[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@redhat.com> 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@redhat.com> --- 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
Worthwhile cleanups, ACK. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
On Wed, Nov 28, 2007 at 02:18:22PM +0100, Jim Meyering wrote:
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.
Yep, good stuff.
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.
I really don't like the macros, particularly when the macro definitions are inline to the function. I'd prefer to see these helpers as separate functions. The compiler is perfectly able to decide whether to inline the code or not & it makes it more friendly to edit & read when it is using separate functions. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
Daniel P. Berrange wrote:
On Wed, Nov 28, 2007 at 02:18:22PM +0100, Jim Meyering wrote:
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.
Yep, good stuff.
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.
I really don't like the macros, particularly when the macro definitions are inline to the function. I'd prefer to see these helpers as separate functions. The compiler is perfectly able to decide whether to inline the code or not & it makes it more friendly to edit & read when it is using separate functions.
The macros were my doing ... I'll leave it up to Jim's judgement as to whether he wants to refactor them out into functions. Rich. -- Emerging Technologies, Red Hat - http://et.redhat.com/~rjones/ Registered Address: Red Hat UK Ltd, Amberley Place, 107-111 Peascod Street, Windsor, Berkshire, SL4 1TE, United Kingdom. Registered in England and Wales under Company Registration No. 03798903
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Nov 28, 2007 at 02:18:22PM +0100, Jim Meyering wrote:
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.
Yep, good stuff.
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.
I really don't like the macros, particularly when the macro definitions are inline to the function. I'd prefer to see these helpers as separate functions. The compiler is perfectly able to decide whether to inline the code or not & it makes it more friendly to edit & read when it is using separate functions.
I've rewritten to use fewer macros. In the process, I did a little testing, and will add an automated test to give this code some coverage in a separate patch. Detect heap allocation failure; factor out some duplication. * qemud/qemud.c (tls_port, tcp_port, mdns_name, tls_allowed_ip_list): (tls_allowed_dn_list): Remove "const", now that we free these. (unix_sock_rw_mask): Rename from unix_sock_rw_perms, so that the latter name can be used as a local string variable, so that the variable name matches the config attribute name. (unix_sock_ro_mask): Rename from unix_sock_ro_perms, likewise. (remoteCheckDN, remoteCheckAccess): Adapt to const removal. (qemudDispatchServer): Check for heap allocation failure. (remoteConfigGetStringList): New function, based on code from Dan Berrangé. (CHECK_TYPE): Remove macro. (checkType): New function. (GET_CONF_INT, GET_CONF_STR): New macros. (remoteReadConfigFile): Use new macros to avoid duplication and to check for allocation failure. * src/conf.h (virConfTypeName): New static inline function. * qemud/libvirtd.conf: Add '=' signs to correct invalid syntax in commented out config lines. Distinguish those lines by having no space between '#' and the following character. --- qemud/libvirtd.conf | 50 ++++---- qemud/qemud.c | 378 ++++++++++++++++++++++++++++++--------------------- src/conf.h | 26 ++++- 3 files changed, 273 insertions(+), 181 deletions(-) diff --git a/qemud/libvirtd.conf b/qemud/libvirtd.conf index 51168b8..b427e14 100644 --- a/qemud/libvirtd.conf +++ b/qemud/libvirtd.conf @@ -11,7 +11,7 @@ # using this capability. # # This is enabled by default, uncomment this to disable it -# listen_tls = 0 +#listen_tls = 0 # Listen for unencrypted TCP connections on the public TCP/IP port. # NB, must pass the --listen flag to the libvirtd process for this to @@ -20,19 +20,19 @@ # NB, this is insecure. Do not use except for development. # # This is disabled by default, uncomment this to enable it. -# listen_tcp = 1 +#listen_tcp = 1 # Override the port for accepting secure TLS connections # This can be a port number, or service name # -# tls_port = "16514" +#tls_port = "16514" # Override the port for accepting insecure TCP connections # This can be a port number, or service name -# -# tcp_port = "16509" +# +#tcp_port = "16509" @@ -42,38 +42,38 @@ # stopping the Avahi daemon # # This is enabled by default, uncomment this to disable it -# mdns_adv = 0 +#mdns_adv = 0 # Override the default mDNS advertizement name. This must be # unique on the immediate broadcast network. -# +# # The default is "Virtualization Host HOSTNAME", where HOSTNAME # is subsituted for the short hostname of the machine (without domain) # -# mdns_name "Virtualization Host Joe Demo" +#mdns_name = "Virtualization Host Joe Demo" # Set the UNIX domain socket group ownership. This can be used to # allow a 'trusted' set of users access to management capabilities # without becoming root. -# -# This is restricted to 'root' by default. -# unix_sock_group "libvirt" +# +# This is restricted to 'root' by default. +#unix_sock_group = "libvirt" # Set the UNIX socket permissions for the R/O socket. This is used # for monitoring VM status only # # Default allows any user. If setting group ownership may want to # restrict this to: -# unix_sock_ro_perms "0777" +#unix_sock_ro_perms = "0777" # Set the UNIX socket permissions for the R/W socket. This is used # for full management of VMs # # Default allows only root. If setting group ownership may want to # relax this to: -# unix_sock_rw_perms "octal-perms" "0770" +#unix_sock_rw_perms = "0770" @@ -85,7 +85,7 @@ # # Default is to always verify. Uncommenting this will disable # verification - make sure an IP whitelist is set -# tls_no_verify_certificate 1 +#tls_no_verify_certificate = 1 # Flag to disable verification of client IP address # @@ -94,27 +94,27 @@ # it is easy to spoof source IP. # # Uncommenting this will disable verification -# tls_no_verify_address 1 +#tls_no_verify_address = 1 # Override the default server key file path # -# key_file "/etc/pki/libvirt/private/serverkey.pem" +#key_file = "/etc/pki/libvirt/private/serverkey.pem" # Override the default server certificate file path # -# cert_file "/etc/pki/libvirt/servercert.pem" +#cert_file = "/etc/pki/libvirt/servercert.pem" # Override the default CA certificate path # -# ca_file "/etc/pki/CA/cacert.pem" +#ca_file = "/etc/pki/CA/cacert.pem" # Specify a certificate revocation list. -# +# # Defaults to not using a CRL, uncomment to enable it -# crl_file "/etc/pki/CA/crl.pem" +#crl_file = "/etc/pki/CA/crl.pem" # A whitelist of allowed x509 Distinguished Names -# This list may contain wildcards such as +# This list may contain wildcards such as # # "C=GB,ST=London,L=London,O=Red Hat,CN=*" # @@ -124,18 +124,16 @@ # entirely rather than using empty list to disable these checks # # By default, no DN's are checked -# tls_allowed_dn_list ["DN1", "DN2"] +#tls_allowed_dn_list = ["DN1", "DN2"] # A whitelist of allowed client IP addresses # -# This list may contain wildcards such as 192.168.* See the POSIX fnmatch +# This list may contain wildcards such as 192.168.* See the POSIX fnmatch # function for the format of the wildcards. # # NB If this is an empty list, no client can connect, so comment out # entirely rather than using empty list to disable these checks # # By default, no IP's are checked. This can be IPv4 or IPv6 addresses -# tls_allowed_ip_list ["ip1", "ip2", "ip3"] - - +#tls_allowed_ip_list = ["ip1", "ip2", "ip3"] diff --git a/qemud/qemud.c b/qemud/qemud.c index 5f76a26..9904e4a 100644 --- a/qemud/qemud.c +++ b/qemud/qemud.c @@ -70,27 +70,27 @@ static int ipsock = 0; /* -l Listen for TCP/IP */ /* Defaults for configuration file elements */ static int listen_tls = 1; static int listen_tcp = 0; -static const char *tls_port = LIBVIRTD_TLS_PORT; -static const char *tcp_port = LIBVIRTD_TCP_PORT; +static char *tls_port = (char *) LIBVIRTD_TLS_PORT; +static char *tcp_port = (char *) LIBVIRTD_TCP_PORT; static gid_t unix_sock_gid = 0; /* Only root by default */ -static int unix_sock_rw_perms = 0700; /* Allow user only */ -static int unix_sock_ro_perms = 0777; /* Allow world */ +static int unix_sock_rw_mask = 0700; /* Allow user only */ +static int unix_sock_ro_mask = 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; -static const char *ca_file = LIBVIRT_CACERT; -static const char *crl_file = ""; +static char *key_file = (char *) LIBVIRT_SERVERKEY; +static char *cert_file = (char *) LIBVIRT_SERVERCERT; +static char *ca_file = (char *) LIBVIRT_CACERT; +static char *crl_file = (char *) ""; static gnutls_certificate_credentials_t x509_cred; static gnutls_dh_params_t dh_params; @@ -407,7 +407,7 @@ static int qemudGoDaemon(void) { status != 0) { return -1; } - + return pid; } } @@ -482,7 +482,7 @@ static int qemudListenUnix(struct qemud_server *server, oldgrp = getgid(); - oldmask = umask(readonly ? ~unix_sock_ro_perms : ~unix_sock_rw_perms); + oldmask = umask(readonly ? ~unix_sock_ro_mask : ~unix_sock_rw_mask); if (getuid() == 0) setgid(unix_sock_gid); @@ -649,7 +649,7 @@ static int qemudInitPaths(struct qemud_server *server, char *base = 0; uid_t uid = geteuid(); - + if (!uid) { if (snprintf (sockname, maxlen, "%s/run/libvirt/libvirt-sock", LOCAL_STATE_DIR) >= maxlen) @@ -764,7 +764,7 @@ static struct qemud_server *qemudInitialize(int sigread) { group = libvirtd_mdns_add_group(server->mdns, mdns_name); } - /* + /* * See if there's a TLS enabled port we can advertise. Cowardly * don't bother to advertise TCP since we don't want people using * them for real world apps @@ -777,7 +777,7 @@ static struct qemud_server *qemudInitialize(int sigread) { } sock = sock->next; } - + /* * Add the primary entry - we choose SSH because its most likely to always * be available @@ -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); @@ -928,13 +928,13 @@ remoteCheckCertificate (gnutls_session_t session) gnutls_x509_crt_deinit (cert); return -1; } - + if (gnutls_x509_crt_get_expiration_time (cert) < now) { qemudLog (QEMUD_ERR, "remoteCheckCertificate: the client certificate has expired"); gnutls_x509_crt_deinit (cert); return -1; } - + if (gnutls_x509_crt_get_activation_time (cert) > now) { qemudLog (QEMUD_ERR, "remoteCheckCertificate: the client certificate is not yet activated"); gnutls_x509_crt_deinit (cert); @@ -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; @@ -1505,6 +1507,131 @@ static void qemudCleanup(struct qemud_server *server) { free(server); } +/* Allocate an array of malloc'd strings from the config file, filename + * (used only in diagnostics), using handle "conf". Upon error, return -1 + * and free any allocated memory. Otherwise, save the array in *list_arg + * and return 0. + */ +static int +remoteConfigGetStringList(virConfPtr conf, const char *key, char ***list_arg, + const char *filename) +{ + char **list; + virConfValuePtr p = virConfGetValue (conf, key); + if (!p) + return 0; + + switch (p->type) { + case VIR_CONF_STRING: + list = malloc (2 * sizeof (*list)); + if (list == NULL) { + qemudLog (QEMUD_ERR, + "failed to allocate memory for %s config list", key); + return -1; + } + list[0] = strdup (p->str); + list[1] = NULL; + if (list[0] == NULL) { + qemudLog (QEMUD_ERR, + "failed to allocate memory for %s config list value", + key); + free (list); + return -1; + } + break; + + case VIR_CONF_LIST: { + int i, len = 0; + virConfValuePtr pp; + for (pp = p->list; pp; pp = pp->next) + len++; + list = calloc (1+len, sizeof (*list)); + if (list == NULL) { + qemudLog (QEMUD_ERR, + "failed to allocate memory for %s config list", key); + return -1; + } + for (i = 0, pp = p->list; pp; ++i, pp = pp->next) { + if (pp->type != VIR_CONF_STRING) { + qemudLog (QEMUD_ERR, "remoteReadConfigFile: %s: %s:" + " must be a string or list of strings\n", + filename, key); + free (list); + return -1; + } + list[i] = strdup (pp->str); + if (list[i] == NULL) { + int j; + for (j = 0 ; j < i ; j++) + free (list[j]); + free (list); + qemudLog (QEMUD_ERR, "failed to allocate memory" + " for %s config list value", key); + return -1; + } + + } + list[i] = NULL; + break; + } + + default: + qemudLog (QEMUD_ERR, "remoteReadConfigFile: %s: %s:" + " must be a string or list of strings\n", + filename, key); + return -1; + } + + *list_arg = list; + return 0; +} + +/* A helper function used by each of the following macros. */ +static int +checkType (virConfValuePtr p, const char *filename, + const char *key, virConfType required_type) +{ + if (p->type != required_type) { + qemudLog (QEMUD_ERR, + "remoteReadConfigFile: %s: %s: invalid type:" + " got %s; expected %s\n", filename, key, + virConfTypeName (p->type), + virConfTypeName (required_type)); + return -1; + } + return 0; +} + +/* If there is no config data for the key, #var_name, then do nothing. + If there is valid data of type VIR_CONF_STRING, and strdup succeeds, + store the result in var_name. Otherwise, (i.e. invalid type, or strdup + failure), give a diagnostic and "goto" the cleanup-and-fail label. */ +#define GET_CONF_STR(conf, filename, var_name) \ + do { \ + virConfValuePtr p = virConfGetValue (conf, #var_name); \ + if (p) { \ + if (checkType (p, filename, #var_name, VIR_CONF_STRING) < 0) \ + goto free_and_fail; \ + (var_name) = strdup (p->str); \ + if ((var_name) == NULL) { \ + qemudLog (QEMUD_ERR, "remoteReadConfigFile: %s\n", \ + strerror (errno)); \ + goto free_and_fail; \ + } \ + } \ + } while (0) + +/* Like GET_CONF_STR, but for integral values. */ +#define GET_CONF_INT(conf, filename, var_name) \ + do { \ + virConfValuePtr p = virConfGetValue (conf, #var_name); \ + if (p) { \ + if (checkType (p, filename, #var_name, VIR_CONF_LONG) < 0) \ + goto free_and_fail; \ + (var_name) = p->l; \ + } \ + } while (0) + /* Read the config file if it exists. * Only used in the remote case, hence the name. */ @@ -1513,6 +1640,12 @@ remoteReadConfigFile (const char *filename) { virConfPtr conf; + /* The following variable names must match the corresponding + configuration strings. */ + char *unix_sock_ro_perms = NULL; + char *unix_sock_rw_perms = NULL; + char *unix_sock_group = NULL; + /* Just check the file is readable before opening it, otherwise * libvirt emits an error. */ @@ -1521,166 +1654,103 @@ remoteReadConfigFile (const char *filename) conf = virConfReadFile (filename); if (!conf) return 0; - 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; + GET_CONF_STR (conf, filename, tls_port); + GET_CONF_STR (conf, filename, tcp_port); - p = virConfGetValue (conf, "unix_sock_group"); - CHECK_TYPE ("unix_sock_group", VIR_CONF_STRING); - if (p && p->str) { + GET_CONF_STR (conf, filename, unix_sock_group); + if (unix_sock_group) { if (getuid() != 0) { qemudLog (QEMUD_WARN, "Cannot set group when not running as root"); } else { - struct group *grp = getgrnam(p->str); + struct group *grp = getgrnam(unix_sock_group); if (!grp) { - qemudLog (QEMUD_ERR, "Failed to lookup group '%s'", p->str); - return -1; + qemudLog (QEMUD_ERR, "Failed to lookup group '%s'", + unix_sock_group); + goto free_and_fail; } unix_sock_gid = grp->gr_gid; } + free (unix_sock_group); + unix_sock_group = NULL; } - p = virConfGetValue (conf, "unix_sock_ro_perms"); - CHECK_TYPE ("unix_sock_ro_perms", VIR_CONF_STRING); - 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); - return -1; + GET_CONF_STR (conf, filename, unix_sock_ro_perms); + if (unix_sock_ro_perms) { + if (xstrtol_i (unix_sock_ro_perms, NULL, 8, &unix_sock_ro_mask) != 0) { + qemudLog (QEMUD_ERR, "Failed to parse mode '%s'", + unix_sock_ro_perms); + goto free_and_fail; } + free (unix_sock_ro_perms); + unix_sock_ro_perms = NULL; } - p = virConfGetValue (conf, "unix_sock_rw_perms"); - CHECK_TYPE ("unix_sock_rw_perms", VIR_CONF_STRING); - 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); - return -1; + GET_CONF_STR (conf, filename, unix_sock_rw_perms); + if (unix_sock_rw_perms) { + if (xstrtol_i (unix_sock_rw_perms, NULL, 8, &unix_sock_rw_mask) != 0) { + qemudLog (QEMUD_ERR, "Failed to parse mode '%s'", + unix_sock_rw_perms); + goto free_and_fail; } + free (unix_sock_rw_perms); + unix_sock_rw_perms = NULL; } #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 (conf, filename, mdns_adv); + GET_CONF_STR (conf, filename, 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 (conf, filename, tls_no_verify_certificate); + GET_CONF_INT (conf, filename, tls_no_verify_address); - 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; - } + GET_CONF_STR (conf, filename, key_file); + GET_CONF_STR (conf, filename, cert_file); + GET_CONF_STR (conf, filename, ca_file); + GET_CONF_STR (conf, filename, crl_file); - default: - qemudLog (QEMUD_ERR, "remoteReadConfigFile: %s: tls_allowed_dn_list: should be a string or list of strings\n", filename); - return -1; - } - } + if (remoteConfigGetStringList (conf, "tls_allowed_dn_list", + &tls_allowed_dn_list, filename) < 0) + goto free_and_fail; - 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; + if (remoteConfigGetStringList (conf, "tls_allowed_ip_list", + &tls_allowed_ip_list, filename) < 0) + goto free_and_fail; - 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; - } + virConfFree (conf); + return 0; - default: - qemudLog (QEMUD_ERR, "remoteReadConfigFile: %s: tls_allowed_ip_list: should be a string or list of strings\n", filename); - return -1; - } + free_and_fail: + virConfFree (conf); + free (mdns_name); + mdns_name = NULL; + free (unix_sock_ro_perms); + free (unix_sock_rw_perms); + free (unix_sock_group); + + /* 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. */ + + if (tls_allowed_dn_list) { + int i; + for (i = 0; tls_allowed_dn_list[i]; i++) + free (tls_allowed_dn_list[i]); + free (tls_allowed_dn_list); + tls_allowed_dn_list = NULL; + } + + if (tls_allowed_ip_list) { + int i; + for (i = 0; tls_allowed_ip_list[i]; i++) + free (tls_allowed_ip_list[i]); + free (tls_allowed_ip_list); + tls_allowed_ip_list = NULL; } - virConfFree (conf); - return 0; + return -1; } /* Print command-line usage. */ diff --git a/src/conf.h b/src/conf.h index 7a7ad25..53c9456 100644 --- a/src/conf.h +++ b/src/conf.h @@ -1,7 +1,7 @@ /** * conf.h: parser for a subset of the Python encoded Xen configuration files * - * Copyright (C) 2006 Red Hat, Inc. + * Copyright (C) 2006, 2007 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -28,6 +28,21 @@ typedef enum { VIR_CONF_LIST = 3 /* a list */ } virConfType; +static inline const char * +virConfTypeName (virConfType t) +{ + switch (t) { + case VIR_CONF_LONG: + return "long"; + case VIR_CONF_STRING: + return "string"; + case VIR_CONF_LIST: + return "list"; + default: + return "*unexpected*"; + } +} + /** * virConfValue: * a value from the configuration file @@ -80,3 +95,12 @@ int __virConfWriteMem (char *memory, } #endif #endif /* __VIR_CONF_H__ */ + +/* + * Local variables: + * indent-tabs-mode: nil + * c-indent-level: 4 + * c-basic-offset: 4 + * tab-width: 4 + * End: + */ -- 1.5.3.6.961.gecf4
On Thu, Nov 29, 2007 at 07:35:47PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Nov 28, 2007 at 02:18:22PM +0100, Jim Meyering wrote:
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.
Yep, good stuff.
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.
I really don't like the macros, particularly when the macro definitions are inline to the function. I'd prefer to see these helpers as separate functions. The compiler is perfectly able to decide whether to inline the code or not & it makes it more friendly to edit & read when it is using separate functions.
I've rewritten to use fewer macros. In the process, I did a little testing, and will add an automated test to give this code some coverage in a separate patch.
Ok, this looks good to me now. Can you commit the code changes, but leave out the config file changes - I've re-written/structured major parts of the config file in my SASL patches, so I'll just include the few typos you found in my updated patches. Regards, Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
"Daniel P. Berrange" <berrange@redhat.com> wrote: ...
Can you commit the code changes, but leave out the config file changes - I've re-written/structured major parts of the config file in my SASL patches, so I'll just include the few typos you found in my updated patches.
Sure. I've just done it.
Just discovered an accidental ommission in this patch - removed the listen_tls and listen_tcp config param handling completely. So I've commited the attached trivial patch to re-add them. On Thu, Nov 29, 2007 at 07:35:47PM +0100, Jim Meyering wrote:
@@ -1521,166 +1654,103 @@ remoteReadConfigFile (const char *filename) conf = virConfReadFile (filename); if (!conf) return 0;
- 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; + GET_CONF_STR (conf, filename, tls_port); + GET_CONF_STR (conf, filename, tcp_port);
Dan. -- |=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=| |=- Perl modules: http://search.cpan.org/~danberr/ -=| |=- Projects: http://freshmeat.net/~danielpb/ -=| |=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
Just discovered an accidental ommission in this patch - removed the listen_tls and listen_tcp config param handling completely. So I've commited the attached trivial patch to re-add them.
Thanks. I'll adjust my upcoming config-test addition so that it ensures each option is parsed.
participants (3)
-
Daniel P. Berrange -
Jim Meyering -
Richard W.M. Jones