[libvirt] [PATCH 4/5] phyp: Fix too small buffer allocation in phypAttachDevice

[Daniel Veillard]

On Sat, Apr 09, 2011 at 11:59:10AM +0200, Matthias Bolte wrote:
> sizeof(domain->name) is the wrong thing. Instead of using strdup here
> rewrite escape_specialcharacters to allocate the buffer itself.
> 
> Add a contains_specialcharacters to be used in phypOpen, as phypOpen is
> not interested in the escaped version.
> ---
>  src/phyp/phyp_driver.c |   90 ++++++++++++++++++++++++++++++++---------------
>  1 files changed, 61 insertions(+), 29 deletions(-)
> 
> diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
> index 24165fb..226946d 100644
> --- a/src/phyp/phyp_driver.c
> +++ b/src/phyp/phyp_driver.c
> @@ -891,14 +891,63 @@ phypUUIDTable_Free(uuid_tablePtr uuid_table)
>      VIR_FREE(uuid_table);
>  }
>  
> -static int
> -escape_specialcharacters(char *src, char *dst, size_t dstlen)
> +static bool
> +contains_specialcharacters(const char *src)
>  {
>      size_t len = strlen(src);
> -    char temp_buffer[len];
> -    unsigned int i = 0, j = 0;
> +    size_t i = 0;
> +
>      if (len == 0)
> -        return -1;
> +        return false;
> +
> +    for (i = 0; i < len; i++) {
> +        switch (src[i]) {
> +            case '&':
> +            case ';':
> +            case '`':
> +            case '@':
> +            case '"':
> +            case '|':
> +            case '*':
> +            case '?':
> +            case '~':
> +            case '<':
> +            case '>':
> +            case '^':
> +            case '(':
> +            case ')':
> +            case '[':
> +            case ']':
> +            case '{':
> +            case '}':
> +            case '$':
> +            case '%':
> +            case '#':
> +            case '\\':
> +            case '\n':
> +            case '\r':
> +            case '\t':
> +                return true;
> +        }
> +    }
> +
> +    return false;
> +}
> +
> +static char *
> +escape_specialcharacters(const char *src)
> +{
> +    size_t len = strlen(src);
> +    size_t i = 0, j = 0;
> +    char *dst;
> +
> +    if (len == 0)
> +        return NULL;
> +
> +    if (VIR_ALLOC_N(dst, len + 1) < 0) {
> +        virReportOOMError();
> +        return NULL;
> +    }
>  
>      for (i = 0; i < len; i++) {
>          switch (src[i]) {
> @@ -929,16 +978,14 @@ escape_specialcharacters(char *src, char *dst, size_t dstlen)
>              case '\t':
>                  continue;
>              default:
> -                temp_buffer[j] = src[i];
> +                dst[j] = src[i];
>                  j++;
>          }
>      }
> -    temp_buffer[j] = '\0';
>  
> -    if (virStrcpy(dst, temp_buffer, dstlen) == NULL)
> -        return -1;
> +    dst[j] = '\0';
>  
> -    return 0;
> +    return dst;
>  }
>  
>  static LIBSSH2_SESSION *
> @@ -1121,8 +1168,6 @@ phypOpen(virConnectPtr conn,
>  {
>      LIBSSH2_SESSION *session = NULL;
>      ConnectionData *connection_data = NULL;
> -    char *string = NULL;
> -    size_t len = 0;
>      int internal_socket;
>      uuid_tablePtr uuid_table = NULL;
>      phyp_driverPtr phyp_driver = NULL;
> @@ -1157,13 +1202,6 @@ phypOpen(virConnectPtr conn,
>      }
>  
>      if (conn->uri->path) {
> -        len = strlen(conn->uri->path) + 1;
> -
> -        if (VIR_ALLOC_N(string, len) < 0) {
> -            virReportOOMError();
> -            goto failure;
> -        }
> -
>          /* need to shift one byte in order to remove the first "/" of URI component */
>          if (conn->uri->path[0] == '/')
>              managed_system = strdup(conn->uri->path + 1);
> @@ -1183,7 +1221,7 @@ phypOpen(virConnectPtr conn,
>          if (char_ptr)
>              *char_ptr = '\0';
>  
> -        if (escape_specialcharacters(conn->uri->path, string, len) == -1) {
> +        if (contains_specialcharacters(conn->uri->path)) {
>              PHYP_ERROR(VIR_ERR_INTERNAL_ERROR,
>                         "%s",
>                         _("Error parsing 'path'. Invalid characters."));
> @@ -1242,7 +1280,6 @@ phypOpen(virConnectPtr conn,
>      }
>  
>      VIR_FREE(connection_data);
> -    VIR_FREE(string);
>  
>      return VIR_DRV_OPEN_ERROR;
>  }
> @@ -1947,15 +1984,10 @@ phypAttachDevice(virDomainPtr domain, const char *xml)
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>      char *domain_name = NULL;
>  
> -    if (VIR_ALLOC_N(domain_name, sizeof(domain->name)) < 0) {
> -        virReportOOMError();
> -        goto err;
> -    }
> +    domain_name = escape_specialcharacters(domain->name);
>  
> -    if (escape_specialcharacters
> -        (domain->name, domain_name, strlen(domain->name)) == -1) {
> -        virReportOOMError();
> -        goto err;
> +    if (domain_name == NULL) {
> +        goto cleanup;
>      }
>  
>      def->os.type = strdup("aix");

  ACK, we just need to make sure contains_specialcharacters() and
escape_specialcharacters() don't diverge on the charater set. Maybe
add a comment in escape_specialcharacters() to this effect.

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/