[Libvir] [PATCH] Detect heap allocation failure; factor out some duplication.

I spotted a few unchecked heap allocations (strdup, malloc, calloc) in qemud.c and have fixed it so such failure evokes a proper diagnostic rather than e.g., a segfault. I've also arranged to free some of the memory upon failure, but not all (see comments for why). In spite of those additions, this patch factors out enough duplication that the net change is to remove a few lines from the file. Although in general I prefer to factor things out into separate functions, in this case, it seemed better to use macros. Wed Nov 28 14:16:17 CET 2007 Jim Meyering <meyering(a)redhat.com&gt; Detect heap allocation failure; factor out some duplication. * qemud/qemud.c (mdns_name, tls_allowed_ip_list, tls_allowed_dn_list): Remove "const", now that we free these. (remoteCheckDN, remoteCheckAccess): Adapt to const removal. (qemudDispatchServer): Check for heap allocation failure. CHECK_TYPE, GET_CONF_INT, GET_CONF_STR, GET_CONF_STR_LIST): New and changed macros. Signed-off-by: Jim Meyering <meyering(a)redhat.com&gt; --- qemud/qemud.c | 247 ++++++++++++++++++++++++++++----------------------------- 1 files changed, 121 insertions(+), 126 deletions(-) diff --git a/qemud/qemud.c b/qemud/qemud.c index 5f76a26..34ab815 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -79,13 +79,13 @@ static int unix_sock_ro_perms = 0777; /* Allow world */ #ifdef HAVE_AVAHI static int mdns_adv = 1; -static const char *mdns_name = NULL; +static char *mdns_name = NULL; #endif static int tls_no_verify_certificate = 0; static int tls_no_verify_address = 0; -static const char **tls_allowed_ip_list = 0; -static const char **tls_allowed_dn_list = 0; +static char **tls_allowed_ip_list = NULL; +static char **tls_allowed_dn_list = NULL; static const char *key_file = LIBVIRT_SERVERKEY; static const char *cert_file = LIBVIRT_SERVERCERT; @@ -840,7 +840,7 @@ remoteCheckDN (gnutls_x509_crt_t cert) { char name[256]; size_t namesize = sizeof name; - const char **wildcards; + char **wildcards; int err; err = gnutls_x509_crt_get_dn (cert, name, &namesize); @@ -959,7 +959,7 @@ static int remoteCheckAccess (struct qemud_client *client) { char addr[NI_MAXHOST]; - const char **wildcards; + char **wildcards; int found, err; /* Verify client certificate. */ @@ -1044,6 +1044,8 @@ static int qemudDispatchServer(struct qemud_server *server, struct qemud_socket } client = calloc(1, sizeof(struct qemud_client)); + if (client == NULL) + goto cleanup; client->magic = QEMUD_CLIENT_MAGIC; client->fd = fd; client->readonly = sock->readonly; @@ -1523,31 +1525,38 @@ remoteReadConfigFile (const char *filename) virConfValuePtr p; -#define CHECK_TYPE(name,typ) if (p && p->type != (typ)) { \ - qemudLog (QEMUD_ERR, \ - "remoteReadConfigFile: %s: %s: expected type " #typ "\n", \ - filename, (name)); \ - return -1; \ - } - - p = virConfGetValue (conf, "listen_tls"); - CHECK_TYPE ("listen_tls", VIR_CONF_LONG); - listen_tls = p ? p->l : listen_tls; - - p = virConfGetValue (conf, "listen_tcp"); - CHECK_TYPE ("listen_tcp", VIR_CONF_LONG); - listen_tcp = p ? p->l : listen_tcp; - - p = virConfGetValue (conf, "tls_port"); - CHECK_TYPE ("tls_port", VIR_CONF_STRING); - tls_port = p ? strdup (p->str) : tls_port; - - p = virConfGetValue (conf, "tcp_port"); - CHECK_TYPE ("tcp_port", VIR_CONF_STRING); - tcp_port = p ? strdup (p->str) : tcp_port; - - p = virConfGetValue (conf, "unix_sock_group"); - CHECK_TYPE ("unix_sock_group", VIR_CONF_STRING); +#define CHECK_TYPE(p, filename, conf, var_name, Type) \ + do { \ + (p) = virConfGetValue ((conf), #Type); \ + if ((p) && (p)->type != (Type)) { \ + qemudLog (QEMUD_ERR, \ + "remoteReadConfigFile: %s: %s:" \ + " expected type " #Type "\n", \ + filename, #var_name); \ + goto free_and_fail; \ + } \ + } while (0) + +#define GET_CONF_INT(p, filename, conf, var_name) \ + do { \ + CHECK_TYPE(p, filename, conf, var_name, VIR_CONF_LONG); \ + if (p) \ + (var_name) = p->l; \ + } while (0) + +#define GET_CONF_STR(p, filename, conf, var_name) \ + do { \ + CHECK_TYPE(p, filename, conf, var_name, VIR_CONF_STRING); \ + if (p) { \ + if (!((var_name) = strdup ((p)->str))) \ + goto diagnose_alloc_failure_and_fail; \ + } \ + } while (0) + + GET_CONF_STR (p, filename, conf, tls_port); + GET_CONF_STR (p, filename, conf, tcp_port); + + CHECK_TYPE (p, filename, conf, unix_sock_group, VIR_CONF_STRING); if (p && p->str) { if (getuid() != 0) { qemudLog (QEMUD_WARN, "Cannot set group when not running as root"); @@ -1561,8 +1570,7 @@ remoteReadConfigFile (const char *filename) } } - p = virConfGetValue (conf, "unix_sock_ro_perms"); - CHECK_TYPE ("unix_sock_ro_perms", VIR_CONF_STRING); + GET_CONF_INT (p, filename, conf, unix_sock_ro_perms); if (p && p->str) { if (xstrtol_i(p->str, NULL, 8, &unix_sock_ro_perms) != 0) { qemudLog (QEMUD_ERR, "Failed to parse mode '%s'", p->str); @@ -1570,8 +1578,7 @@ remoteReadConfigFile (const char *filename) } } - p = virConfGetValue (conf, "unix_sock_rw_perms"); - CHECK_TYPE ("unix_sock_rw_perms", VIR_CONF_STRING); + GET_CONF_INT (p, filename, conf, unix_sock_rw_perms); if (p && p->str) { if (xstrtol_i(p->str, NULL, 8, &unix_sock_rw_perms) != 0) { qemudLog (QEMUD_ERR, "Failed to parse mode '%s'", p->str); @@ -1580,107 +1587,95 @@ remoteReadConfigFile (const char *filename) } #ifdef HAVE_AVAHI - p = virConfGetValue (conf, "mdns_adv"); - CHECK_TYPE ("mdns_adv", VIR_CONF_LONG); - mdns_adv = p ? p->l : mdns_adv; - - p = virConfGetValue (conf, "mdns_name"); - CHECK_TYPE ("mdns_name", VIR_CONF_STRING); - mdns_name = p ? strdup (p->str) : NULL; + GET_CONF_INT (p, filename, conf, mdns_adv); + GET_CONF_STR (p, filename, conf, mdns_name); #endif - p = virConfGetValue (conf, "tls_no_verify_certificate"); - CHECK_TYPE ("tls_no_verify_certificate", VIR_CONF_LONG); - tls_no_verify_certificate = p ? p->l : tls_no_verify_certificate; - - p = virConfGetValue (conf, "tls_no_verify_address"); - CHECK_TYPE ("tls_no_verify_address", VIR_CONF_LONG); - tls_no_verify_address = p ? p->l : tls_no_verify_address; - - p = virConfGetValue (conf, "key_file"); - CHECK_TYPE ("key_file", VIR_CONF_STRING); - key_file = p ? strdup (p->str) : key_file; - - p = virConfGetValue (conf, "cert_file"); - CHECK_TYPE ("cert_file", VIR_CONF_STRING); - cert_file = p ? strdup (p->str) : cert_file; - - p = virConfGetValue (conf, "ca_file"); - CHECK_TYPE ("ca_file", VIR_CONF_STRING); - ca_file = p ? strdup (p->str) : ca_file; - - p = virConfGetValue (conf, "crl_file"); - CHECK_TYPE ("crl_file", VIR_CONF_STRING); - crl_file = p ? strdup (p->str) : crl_file; - - p = virConfGetValue (conf, "tls_allowed_dn_list"); - if (p) { - switch (p->type) { - case VIR_CONF_STRING: - tls_allowed_dn_list = malloc (2 * sizeof (char *)); - tls_allowed_dn_list[0] = strdup (p->str); - tls_allowed_dn_list[1] = 0; - break; + GET_CONF_INT (p, filename, conf, tls_no_verify_certificate); + GET_CONF_INT (p, filename, conf, tls_no_verify_address); + + GET_CONF_STR (p, filename, conf, key_file); + GET_CONF_STR (p, filename, conf, cert_file); + GET_CONF_STR (p, filename, conf, ca_file); + GET_CONF_STR (p, filename, conf, crl_file); + +#define GET_CONF_STR_LIST(List_var) \ + do { \ + p = virConfGetValue (conf, #List_var); \ + if (p) { \ + switch (p->type) { \ + case VIR_CONF_STRING: \ + if (!((List_var) = malloc (2 * sizeof (char *)))) \ + goto free_and_fail; \ + if (!((List_var)[0] = strdup (p->str))) \ + goto free_and_fail; \ + (List_var)[1] = NULL; \ + break; \ + \ + case VIR_CONF_LIST: { \ + int i, len = 0; \ + virConfValuePtr pp; \ + for (pp = p->list; pp; pp = p->next) \ + len++; \ + if (!((List_var) = malloc ((1+len) * sizeof (char *)))) \ + goto free_and_fail; \ + for (i = 0, pp = p->list; pp; ++i, pp = p->next) { \ + if (pp->type != VIR_CONF_STRING) { \ + qemudLog (QEMUD_ERR, "remoteReadConfigFile: %s: %s: " \ + "must be a string or list of strings\n", \ + filename, #List_var); \ + goto free_and_fail; \ + } \ + if (!((List_var)[i] = strdup (pp->str))) \ + goto free_and_fail; \ + } \ + (List_var)[i] = NULL; \ + break; \ + } \ + \ + default: \ + qemudLog (QEMUD_ERR, "remoteReadConfigFile: %s: %s: " \ + "must be a string or list of strings\n", \ + filename, #List_var); \ + goto free_and_fail; \ + } \ + } \ + } while (0) + + GET_CONF_STR_LIST (tls_allowed_dn_list); + GET_CONF_STR_LIST (tls_allowed_ip_list); - case VIR_CONF_LIST: { - int i, len = 0; - virConfValuePtr pp; - for (pp = p->list; pp; pp = p->next) - len++; - tls_allowed_dn_list = - malloc ((1+len) * sizeof (char *)); - for (i = 0, pp = p->list; pp; ++i, pp = p->next) { - if (pp->type != VIR_CONF_STRING) { - qemudLog (QEMUD_ERR, "remoteReadConfigFile: %s: tls_allowed_dn_list: should be a string or list of strings\n", filename); - return -1; - } - tls_allowed_dn_list[i] = strdup (pp->str); - } - tls_allowed_dn_list[i] = 0; - break; - } + virConfFree (conf); + return 0; - default: - qemudLog (QEMUD_ERR, "remoteReadConfigFile: %s: tls_allowed_dn_list: should be a string or list of strings\n", filename); - return -1; - } - } + diagnose_alloc_failure_and_fail: + qemudLog (QEMUD_ERR, "remoteReadConfigFile: %s\n", strerror (errno)); - p = virConfGetValue (conf, "tls_allowed_ip_list"); - if (p) { - switch (p->type) { - case VIR_CONF_STRING: - tls_allowed_ip_list = malloc (2 * sizeof (char *)); - tls_allowed_ip_list[0] = strdup (p->str); - tls_allowed_ip_list[1] = 0; - break; + free_and_fail: + free (mdns_name); + mdns_name = NULL; - case VIR_CONF_LIST: { - int i, len = 0; - virConfValuePtr pp; - for (pp = p->list; pp; pp = p->next) - len++; - tls_allowed_ip_list = - malloc ((1+len) * sizeof (char *)); - for (i = 0, pp = p->list; pp; ++i, pp = p->next) { - if (pp->type != VIR_CONF_STRING) { - qemudLog (QEMUD_ERR, "remoteReadConfigFile: %s: tls_allowed_ip_list: should be a string or list of strings\n", filename); - return -1; - } - tls_allowed_ip_list[i] = strdup (pp->str); - } - tls_allowed_ip_list[i] = 0; - break; - } + /* Don't bother trying to free tcp_port, tls_port, key_file, cert_file, + ca_file, or crl_file, since they are initialized to non-malloc'd + strings. Besides, these are static variables, and callers are + unlikely to call this function more than once, so there wouldn't + even be a real leak. */ - default: - qemudLog (QEMUD_ERR, "remoteReadConfigFile: %s: tls_allowed_ip_list: should be a string or list of strings\n", filename); - return -1; - } + if (tls_allowed_ip_list) { + char *t; + for (t = *tls_allowed_ip_list; t; t++) + free (t); + tls_allowed_ip_list = NULL; } - virConfFree (conf); - return 0; + if (tls_allowed_dn_list) { + char *t; + for (t = *tls_allowed_dn_list; t; t++) + free (t); + tls_allowed_dn_list = NULL; + } + + return -1; } /* Print command-line usage. */ -- 1.5.3.6.961.gecf4