| Message ID | 1320746152-31620-1-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
"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!
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
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
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;
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(-)