[Qemu-trivial] [PATCH] ui/vnc: Convert sasl.mechlist to g_malloc() & friends

[Qemu-trivial] [PATCH] ui/vnc: Convert sasl.mechlist to g_malloc() & friends

[Markus Armbruster]

Fixes protocol_client_auth_sasl_mechname() not to crash when malloc()
fails.  Spotted by Coverity.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 ui/vnc-auth-sasl.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
index 23b1bf5..a88973b 100644
--- a/ui/vnc-auth-sasl.c
+++ b/ui/vnc-auth-sasl.c
@@ -35,7 +35,7 @@ void vnc_sasl_client_cleanup(VncState *vs)
         vs->sasl.encodedLength = vs->sasl.encodedOffset = 0;
         vs->sasl.encoded = NULL;
         g_free(vs->sasl.username);
-        free(vs->sasl.mechlist);
+        g_free(vs->sasl.mechlist);
         vs->sasl.username = vs->sasl.mechlist = NULL;
         sasl_dispose(&vs->sasl.conn);
         vs->sasl.conn = NULL;
@@ -430,7 +430,7 @@ static int protocol_client_auth_sasl_start_len(VncState *vs, uint8_t *data, size
 
 static int protocol_client_auth_sasl_mechname(VncState *vs, uint8_t *data, size_t len)
 {
-    char *mechname = malloc(len + 1);
+    char *mechname = g_malloc(len + 1);
     if (!mechname) {
         VNC_DEBUG("Out of memory reading mechname\n");
         vnc_client_error(vs);
@@ -460,7 +460,7 @@ static int protocol_client_auth_sasl_mechname(VncState *vs, uint8_t *data, size_
         }
     }
 
-    free(vs->sasl.mechlist);
+    g_free(vs->sasl.mechlist);
     vs->sasl.mechlist = mechname;
 
     VNC_DEBUG("Validated mechname '%s'\n", mechname);
@@ -469,7 +469,7 @@ static int protocol_client_auth_sasl_mechname(VncState *vs, uint8_t *data, size_
 
  fail:
     vnc_client_error(vs);
-    free(mechname);
+    g_free(mechname);
     return -1;
 }
 
@@ -608,7 +608,7 @@ void start_auth_sasl(VncState *vs)
     }
     VNC_DEBUG("Available mechanisms for client: '%s'\n", mechlist);
 
-    if (!(vs->sasl.mechlist = strdup(mechlist))) {
+    if (!(vs->sasl.mechlist = g_strdup(mechlist))) {
         VNC_DEBUG("Out of memory");
         sasl_dispose(&vs->sasl.conn);
         vs->sasl.conn = NULL;
-- 
1.7.6.4

Re: [Qemu-trivial] [Qemu-devel] [PATCH] ui/vnc: Convert sasl.mechlist to g_malloc() & friends

[Daniel P. Berrange]

On Tue, Nov 08, 2011 at 10:55:52AM +0100, Markus Armbruster wrote:
> Fixes protocol_client_auth_sasl_mechname() not to crash when malloc()
> fails.  Spotted by Coverity.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  ui/vnc-auth-sasl.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
> index 23b1bf5..a88973b 100644
> --- a/ui/vnc-auth-sasl.c
> +++ b/ui/vnc-auth-sasl.c
> @@ -35,7 +35,7 @@ void vnc_sasl_client_cleanup(VncState *vs)
>          vs->sasl.encodedLength = vs->sasl.encodedOffset = 0;
>          vs->sasl.encoded = NULL;
>          g_free(vs->sasl.username);
> -        free(vs->sasl.mechlist);
> +        g_free(vs->sasl.mechlist);
>          vs->sasl.username = vs->sasl.mechlist = NULL;
>          sasl_dispose(&vs->sasl.conn);
>          vs->sasl.conn = NULL;
> @@ -430,7 +430,7 @@ static int protocol_client_auth_sasl_start_len(VncState *vs, uint8_t *data, size
>  
>  static int protocol_client_auth_sasl_mechname(VncState *vs, uint8_t *data, size_t len)
>  {
> -    char *mechname = malloc(len + 1);
> +    char *mechname = g_malloc(len + 1);
>      if (!mechname) {
>          VNC_DEBUG("Out of memory reading mechname\n");
>          vnc_client_error(vs);

You can delete the   if (!mechname) block now you have g_malloc

The reason for the crash on OOM is here, but the diff context doesn't show it:

Notice the missing 'return -1'  statement following vnc_client_error(vs);

    char *mechname = malloc(len + 1);
    if (!mechname) {
        VNC_DEBUG("Out of memory reading mechname\n");
        vnc_client_error(vs);
    }
    strncpy(mechname, (char*)data, len);
    mechname[len] = '\0';



> @@ -460,7 +460,7 @@ static int protocol_client_auth_sasl_mechname(VncState *vs, uint8_t *data, size_
>          }
>      }
>  
> -    free(vs->sasl.mechlist);
> +    g_free(vs->sasl.mechlist);
>      vs->sasl.mechlist = mechname;
>  
>      VNC_DEBUG("Validated mechname '%s'\n", mechname);
> @@ -469,7 +469,7 @@ static int protocol_client_auth_sasl_mechname(VncState *vs, uint8_t *data, size_
>  
>   fail:
>      vnc_client_error(vs);
> -    free(mechname);
> +    g_free(mechname);
>      return -1;
>  }
>  
> @@ -608,7 +608,7 @@ void start_auth_sasl(VncState *vs)
>      }
>      VNC_DEBUG("Available mechanisms for client: '%s'\n", mechlist);
>  
> -    if (!(vs->sasl.mechlist = strdup(mechlist))) {
> +    if (!(vs->sasl.mechlist = g_strdup(mechlist))) {
>          VNC_DEBUG("Out of memory");
>          sasl_dispose(&vs->sasl.conn);
>          vs->sasl.conn = NULL;

Again, you can delete the conditional here with g_strdup

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-trivial] [Qemu-devel] [PATCH] ui/vnc: Convert sasl.mechlist to g_malloc() & friends

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Tue, Nov 08, 2011 at 10:55:52AM +0100, Markus Armbruster wrote:
>> Fixes protocol_client_auth_sasl_mechname() not to crash when malloc()
>> fails.  Spotted by Coverity.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  ui/vnc-auth-sasl.c |   10 +++++-----
>>  1 files changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
>> index 23b1bf5..a88973b 100644
>> --- a/ui/vnc-auth-sasl.c
>> +++ b/ui/vnc-auth-sasl.c
>> @@ -35,7 +35,7 @@ void vnc_sasl_client_cleanup(VncState *vs)
>>          vs->sasl.encodedLength = vs->sasl.encodedOffset = 0;
>>          vs->sasl.encoded = NULL;
>>          g_free(vs->sasl.username);
>> -        free(vs->sasl.mechlist);
>> +        g_free(vs->sasl.mechlist);
>>          vs->sasl.username = vs->sasl.mechlist = NULL;
>>          sasl_dispose(&vs->sasl.conn);
>>          vs->sasl.conn = NULL;
>> @@ -430,7 +430,7 @@ static int protocol_client_auth_sasl_start_len(VncState *vs, uint8_t *data, size
>>  
>>  static int protocol_client_auth_sasl_mechname(VncState *vs, uint8_t *data, size_t len)
>>  {
>> -    char *mechname = malloc(len + 1);
>> +    char *mechname = g_malloc(len + 1);
>>      if (!mechname) {
>>          VNC_DEBUG("Out of memory reading mechname\n");
>>          vnc_client_error(vs);
>
> You can delete the   if (!mechname) block now you have g_malloc

Should've seen that myself.  Guess I stared at Coverity reports for too
long.  I'll respin.

> The reason for the crash on OOM is here, but the diff context doesn't show it:
>
> Notice the missing 'return -1'  statement following vnc_client_error(vs);
>
>     char *mechname = malloc(len + 1);
>     if (!mechname) {
>         VNC_DEBUG("Out of memory reading mechname\n");
>         vnc_client_error(vs);
>     }
>     strncpy(mechname, (char*)data, len);
>     mechname[len] = '\0';

Correct.

>> @@ -460,7 +460,7 @@ static int protocol_client_auth_sasl_mechname(VncState *vs, uint8_t *data, size_
>>          }
>>      }
>>  
>> -    free(vs->sasl.mechlist);
>> +    g_free(vs->sasl.mechlist);
>>      vs->sasl.mechlist = mechname;
>>  
>>      VNC_DEBUG("Validated mechname '%s'\n", mechname);
>> @@ -469,7 +469,7 @@ static int protocol_client_auth_sasl_mechname(VncState *vs, uint8_t *data, size_
>>  
>>   fail:
>>      vnc_client_error(vs);
>> -    free(mechname);
>> +    g_free(mechname);
>>      return -1;
>>  }
>>  
>> @@ -608,7 +608,7 @@ void start_auth_sasl(VncState *vs)
>>      }
>>      VNC_DEBUG("Available mechanisms for client: '%s'\n", mechlist);
>>  
>> -    if (!(vs->sasl.mechlist = strdup(mechlist))) {
>> +    if (!(vs->sasl.mechlist = g_strdup(mechlist))) {
>>          VNC_DEBUG("Out of memory");
>>          sasl_dispose(&vs->sasl.conn);
>>          vs->sasl.conn = NULL;
>
> Again, you can delete the conditional here with g_strdup

Yes.

Thanks!

Re: [Qemu-trivial] [Qemu-devel] [PATCH] ui/vnc: Convert sasl.mechlist to g_malloc() & friends

[Stefan Hajnoczi]

On Tue, Nov 08, 2011 at 10:55:52AM +0100, Markus Armbruster wrote:
> Fixes protocol_client_auth_sasl_mechname() not to crash when malloc()
> fails.  Spotted by Coverity.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  ui/vnc-auth-sasl.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)

g_malloc(), the allocator the never* fails.

* Or if it does you won't be around to care about it ;-)

Thanks, merged into the trivial-patches tree:

http://repo.or.cz/w/qemu/stefanha.git/shortlog/refs/heads/trivial-patches

Stefan

Re: [Qemu-trivial] [Qemu-devel] [PATCH] ui/vnc: Convert sasl.mechlist to g_malloc() & friends

[Stefan Hajnoczi]

On Tue, Nov 8, 2011 at 10:49 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Nov 08, 2011 at 10:55:52AM +0100, Markus Armbruster wrote:
>> Fixes protocol_client_auth_sasl_mechname() not to crash when malloc()
>> fails.  Spotted by Coverity.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  ui/vnc-auth-sasl.c |   10 +++++-----
>>  1 files changed, 5 insertions(+), 5 deletions(-)
> Thanks, merged into the trivial-patches tree:

I'll grab the new version when it comes out.

Stefan