[Qemu-devel] [PATCH 0/6] plug memory and file-descriptor leaks

[Qemu-devel] [PATCH 0/6] plug memory and file-descriptor leaks

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

These changes fix most of the legitimate coverity-reported leak warnings.

Jim Meyering (6):
  qcow2: don't leak buffer for unexpected qcow_version in header
  qemu-ga: avoid unconditional lockfile file descriptor leak
  linux-user: do_msgrcv: don't leak host_mb upon TARGET_EFAULT failure
  sheepdog: don't leak socket file descriptor upon connection failure
  arm-semi: don't leak 1kb user string lock buffer upon TARGET_SYS_OPEN
  softmmu-semi: fix lock_user* functions not to deref NULL upon OOM

 arm-semi.c           | 13 +++++++------
 block/qcow2.c        |  1 +
 block/sheepdog.c     |  1 +
 linux-user/syscall.c |  1 +
 qemu-ga.c            | 10 ++++++++--
 softmmu-semi.h       |  6 +++---
 6 files changed, 21 insertions(+), 11 deletions(-)

--
1.7.10.2.520.g6a4a482

[Qemu-devel] [PATCH 1/6] qcow2: don't leak buffer for unexpected qcow_version in header

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>


Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 block/qcow2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 655799c..f3388bf 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -919,6 +919,7 @@ int qcow2_update_header(BlockDriverState *bs)
         ret = sizeof(*header);
         break;
     default:
+        free(buf);
         return -EINVAL;
     }

-- 
1.7.10.2.520.g6a4a482

Re: [Qemu-devel] [PATCH 1/6] qcow2: don't leak buffer for unexpected qcow_version in header

[Kevin Wolf]

Am 16.05.2012 15:07, schrieb Jim Meyering:
> From: Jim Meyering <meyering@redhat.com>
> 
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> ---
>  block/qcow2.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 655799c..f3388bf 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -919,6 +919,7 @@ int qcow2_update_header(BlockDriverState *bs)
>          ret = sizeof(*header);
>          break;
>      default:
> +        free(buf);
>          return -EINVAL;
>      }
> 

buf was allocated with qemu_blockalign(), so it must be freed with
qemu_vfree(). But instead of open-coding it here, this place should work
like all other places in the same function:

ret = -EINVAL;
goto fail;

Kevin

[Qemu-devel] [PATCHv2 1/6] qcow2: don't leak buffer for unexpected qcow_version in header

[Jim Meyering]

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
Thanks to Kevin Wolf for the improvement.

 block/qcow2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 655799c..c2e49cd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -919,7 +919,8 @@ int qcow2_update_header(BlockDriverState *bs)
         ret = sizeof(*header);
         break;
     default:
-        return -EINVAL;
+        ret = -EINVAL;
+        goto fail;
     }

     buf += ret;
--
1.7.10.2.552.gaa3bb87

Re: [Qemu-devel] [PATCHv2 1/6] qcow2: don't leak buffer for unexpected qcow_version in header

[Kevin Wolf]

Am 21.05.2012 13:06, schrieb Jim Meyering:
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> ---
> Thanks to Kevin Wolf for the improvement.
> 
>  block/qcow2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 655799c..c2e49cd 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -919,7 +919,8 @@ int qcow2_update_header(BlockDriverState *bs)
>          ret = sizeof(*header);
>          break;
>      default:
> -        return -EINVAL;
> +        ret = -EINVAL;
> +        goto fail;
>      }
> 
>      buf += ret;
> --
> 1.7.10.2.552.gaa3bb87

Acked-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 1/6] qcow2: don't leak buffer for unexpected qcow_version in header

[Jim Meyering]

Kevin Wolf wrote:
> Am 16.05.2012 15:07, schrieb Jim Meyering:
>> From: Jim Meyering <meyering@redhat.com>
>>
>>
>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>> ---
>>  block/qcow2.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 655799c..f3388bf 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -919,6 +919,7 @@ int qcow2_update_header(BlockDriverState *bs)
>>          ret = sizeof(*header);
>>          break;
>>      default:
>> +        free(buf);
>>          return -EINVAL;
>>      }
>>
>
> buf was allocated with qemu_blockalign(), so it must be freed with
> qemu_vfree(). But instead of open-coding it here, this place should work
> like all other places in the same function:
>
> ret = -EINVAL;
> goto fail;

Hi Kevin,
Thanks.  That is obviously better.
I've just posted a v2.

[Qemu-devel] [PATCH 2/6] qemu-ga: avoid unconditional lockfile file descriptor leak

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

Do not leak a file descriptor.
Also, do not forget to unlink the lockfile upon failed lockf.
Always close the lockfile file descriptor, taking care
to diagnose close, as well as open and write, failure.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 qemu-ga.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/qemu-ga.c b/qemu-ga.c
index 680997e..6c6de55 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -241,12 +241,13 @@ void ga_set_response_delimited(GAState *s)
 static bool ga_open_pidfile(const char *pidfile)
 {
     int pidfd;
+    int write_err;
     char pidstr[32];

     pidfd = open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR);
     if (pidfd == -1 || lockf(pidfd, F_TLOCK, 0)) {
         g_critical("Cannot lock pid file, %s", strerror(errno));
-        return false;
+        goto fail;
     }

     if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) {
@@ -254,7 +255,9 @@ static bool ga_open_pidfile(const char *pidfile)
         goto fail;
     }
     sprintf(pidstr, "%d", getpid());
-    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
+    write_err = write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr);
+    if (close(pidfd) || write_err) {
+        pidfd = -1;
         g_critical("Failed to write pid file");
         goto fail;
     }
@@ -262,6 +265,9 @@ static bool ga_open_pidfile(const char *pidfile)
     return true;

 fail:
+    if (pidfd != -1) {
+        close(pidfd);
+    }
     unlink(pidfile);
     return false;
 }
-- 
1.7.10.2.520.g6a4a482

Re: [Qemu-devel] [PATCH 2/6] qemu-ga: avoid unconditional lockfile file descriptor leak

[Michael Roth]

On Wed, May 16, 2012 at 03:07:57PM +0200, Jim Meyering wrote:
> From: Jim Meyering <meyering@redhat.com>
> 
> Do not leak a file descriptor.
> Also, do not forget to unlink the lockfile upon failed lockf.
> Always close the lockfile file descriptor, taking care
> to diagnose close, as well as open and write, failure.
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> ---
>  qemu-ga.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 680997e..6c6de55 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -241,12 +241,13 @@ void ga_set_response_delimited(GAState *s)
>  static bool ga_open_pidfile(const char *pidfile)
>  {
>      int pidfd;
> +    int write_err;
>      char pidstr[32];
> 
>      pidfd = open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR);
>      if (pidfd == -1 || lockf(pidfd, F_TLOCK, 0)) {
>          g_critical("Cannot lock pid file, %s", strerror(errno));
> -        return false;
> +        goto fail;
>      }
> 
>      if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) {
> @@ -254,7 +255,9 @@ static bool ga_open_pidfile(const char *pidfile)
>          goto fail;
>      }
>      sprintf(pidstr, "%d", getpid());
> -    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
> +    write_err = write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr);
> +    if (close(pidfd) || write_err) {

Closing the fd gives up the lock, which is not what we want in this
case. The intention is to hold it for the life of the process. In the
case of close() failing the error msg will also be inaccurate.

So I think we can leave this chunk out. In the error case the fd will get
cleaned up with the handling you added below.

For the success case, If we want to avoid leaking it, I would suggest
assigning it to a new field in GAState which is close()'d somewhere in
main() after we exit from g_main_loop_run().

> +        pidfd = -1;
>          g_critical("Failed to write pid file");
>          goto fail;
>      }
> @@ -262,6 +265,9 @@ static bool ga_open_pidfile(const char *pidfile)
>      return true;
> 
>  fail:
> +    if (pidfd != -1) {
> +        close(pidfd);

Since the patch wants to report close() errors above, we should do it
here too.

> +    }
>      unlink(pidfile);
>      return false;
>  }
> -- 
> 1.7.10.2.520.g6a4a482
> 

Re: [Qemu-devel] [PATCH 2/6] qemu-ga: avoid unconditional lockfile file descriptor leak

[Jim Meyering]

Michael Roth wrote:
> On Wed, May 16, 2012 at 03:07:57PM +0200, Jim Meyering wrote:
>> From: Jim Meyering <meyering@redhat.com>
>>
>> Do not leak a file descriptor.
>> Also, do not forget to unlink the lockfile upon failed lockf.
>> Always close the lockfile file descriptor, taking care
>> to diagnose close, as well as open and write, failure.
>>
>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>> ---
>>  qemu-ga.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/qemu-ga.c b/qemu-ga.c
>> index 680997e..6c6de55 100644
>> --- a/qemu-ga.c
>> +++ b/qemu-ga.c
>> @@ -241,12 +241,13 @@ void ga_set_response_delimited(GAState *s)
>>  static bool ga_open_pidfile(const char *pidfile)
>>  {
>>      int pidfd;
>> +    int write_err;
>>      char pidstr[32];
>>
>>      pidfd = open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR);
>>      if (pidfd == -1 || lockf(pidfd, F_TLOCK, 0)) {
>>          g_critical("Cannot lock pid file, %s", strerror(errno));
>> -        return false;
>> +        goto fail;
>>      }
>>
>>      if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) {
>> @@ -254,7 +255,9 @@ static bool ga_open_pidfile(const char *pidfile)
>>          goto fail;
>>      }
>>      sprintf(pidstr, "%d", getpid());
>> -    if (write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr)) {
>> +    write_err = write(pidfd, pidstr, strlen(pidstr)) != strlen(pidstr);
>> +    if (close(pidfd) || write_err) {
>
> Closing the fd gives up the lock, which is not what we want in this
> case. The intention is to hold it for the life of the process. In the
> case of close() failing the error msg will also be inaccurate.

Hi Michael,

Thanks for the feedback.  You're right.  I missed the significance
of the lockf call.  For now, I'm about to post a much less ambitious V2
patch that simply avoids the FD leak upon failed lockf.

> So I think we can leave this chunk out. In the error case the fd will get
> cleaned up with the handling you added below.
>
> For the success case, If we want to avoid leaking it, I would suggest
> assigning it to a new field in GAState which is close()'d somewhere in
> main() after we exit from g_main_loop_run().
>
>> +        pidfd = -1;
>>          g_critical("Failed to write pid file");
>>          goto fail;
>>      }
>> @@ -262,6 +265,9 @@ static bool ga_open_pidfile(const char *pidfile)
>>      return true;
>>
>>  fail:
>> +    if (pidfd != -1) {
>> +        close(pidfd);
>
> Since the patch wants to report close() errors above, we should do it
> here too.

Typically not, since if there's a primary failure,
that will be reported, and the only reason to perform
the this sort of close is to avoid an FD leak.  Additional
warning about close (aka write) failure is usually unwelcome.

[Qemu-devel] [PATCHv2 2/6] qemu-ga: don't leak a file descriptor upon failed lockf

[Jim Meyering]

Signed-off-by: Jim Meyering <meyering@redhat.com>
---

 qemu-ga.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qemu-ga.c b/qemu-ga.c
index 680997e..24b236a 100644
--- a/qemu-ga.c
+++ b/qemu-ga.c
@@ -246,6 +246,9 @@ static bool ga_open_pidfile(const char *pidfile)
     pidfd = open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR);
     if (pidfd == -1 || lockf(pidfd, F_TLOCK, 0)) {
         g_critical("Cannot lock pid file, %s", strerror(errno));
+        if (pidfd != -1) {
+            close(pidfd);
+        }
         return false;
     }

--
1.7.10.2.520.g6a4a482

Re: [Qemu-devel] [PATCHv2 2/6] qemu-ga: don't leak a file descriptor upon failed lockf

[Michael Roth]

On Wed, May 16, 2012 at 10:19:55PM +0200, Jim Meyering wrote:
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>

Acked-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
> 
>  qemu-ga.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/qemu-ga.c b/qemu-ga.c
> index 680997e..24b236a 100644
> --- a/qemu-ga.c
> +++ b/qemu-ga.c
> @@ -246,6 +246,9 @@ static bool ga_open_pidfile(const char *pidfile)
>      pidfd = open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR);
>      if (pidfd == -1 || lockf(pidfd, F_TLOCK, 0)) {
>          g_critical("Cannot lock pid file, %s", strerror(errno));
> +        if (pidfd != -1) {
> +            close(pidfd);
> +        }
>          return false;
>      }
> 
> --
> 1.7.10.2.520.g6a4a482
> 

[Qemu-devel] [PATCH 3/6] linux-user: do_msgrcv: don't leak host_mb upon TARGET_EFAULT failure

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>


Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 linux-user/syscall.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 20d2a74..bdf8ce0 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2814,6 +2814,7 @@ static inline abi_long do_msgrcv(int msqid, abi_long msgp,
 end:
     if (target_mb)
         unlock_user_struct(target_mb, msgp, 1);
+    free(host_mb);
     return ret;
 }

-- 
1.7.10.2.520.g6a4a482

Re: [Qemu-devel] [PATCH 3/6] linux-user: do_msgrcv: don't leak host_mb upon TARGET_EFAULT failure

[Peter Maydell]

On 16 May 2012 14:07, Jim Meyering <jim@meyering.net> wrote:
> From: Jim Meyering <meyering@redhat.com>
>
>
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> ---
>  linux-user/syscall.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 20d2a74..bdf8ce0 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -2814,6 +2814,7 @@ static inline abi_long do_msgrcv(int msqid, abi_long msgp,
>  end:
>     if (target_mb)
>         unlock_user_struct(target_mb, msgp, 1);
> +    free(host_mb);
>     return ret;
>  }

This will cause us to free() host_mb twice in the normal-return case.

-- PMM

Re: [Qemu-devel] [PATCH 3/6] linux-user: do_msgrcv: don't leak host_mb upon TARGET_EFAULT failure

[Jim Meyering]

Peter Maydell wrote:

> On 16 May 2012 14:07, Jim Meyering <jim@meyering.net> wrote:
>> From: Jim Meyering <meyering@redhat.com>
>>
>>
>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>> ---
>>  linux-user/syscall.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 20d2a74..bdf8ce0 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -2814,6 +2814,7 @@ static inline abi_long do_msgrcv(int msqid, abi_long msgp,
>>  end:
>>     if (target_mb)
>>         unlock_user_struct(target_mb, msgp, 1);
>> +    free(host_mb);
>>     return ret;
>>  }
>
> This will cause us to free() host_mb twice in the normal-return case.

Good catch.  Thanks.
V2 corrects that.

[Qemu-devel] [PATCHv2 3/6] linux-user: do_msgrcv: don't leak host_mb upon TARGET_EFAULT failure

[Jim Meyering]

Also, use g_malloc to avoid NULL-deref upon OOM.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
There are other, similar NULL-deref risks in this file.
TBD separately.

 linux-user/syscall.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 20d2a74..9bf0b28 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2794,7 +2794,7 @@ static inline abi_long do_msgrcv(int msqid, abi_long msgp,
     if (!lock_user_struct(VERIFY_WRITE, target_mb, msgp, 0))
         return -TARGET_EFAULT;

-    host_mb = malloc(msgsz+sizeof(long));
+    host_mb = g_malloc(msgsz+sizeof(long));
     ret = get_errno(msgrcv(msqid, host_mb, msgsz, tswapal(msgtyp), msgflg));

     if (ret > 0) {
@@ -2809,11 +2809,11 @@ static inline abi_long do_msgrcv(int msqid, abi_long msgp,
     }

     target_mb->mtype = tswapal(host_mb->mtype);
-    free(host_mb);

 end:
     if (target_mb)
         unlock_user_struct(target_mb, msgp, 1);
+    g_free(host_mb);
     return ret;
 }

--
1.7.10.2.520.g6a4a482

[Qemu-devel] [PATCH 4/6] sheepdog: don't leak socket file descriptor upon connection failure

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>


Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 block/sheepdog.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index e01d371..a5c834f 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -489,6 +489,7 @@ static int connect_to_sdog(const char *addr, const char *port)
             if (errno == EINTR) {
                 goto reconnect;
             }
+            close(fd);
             break;
         }

-- 
1.7.10.2.520.g6a4a482

Re: [Qemu-devel] [PATCH 4/6] sheepdog: don't leak socket file descriptor upon connection failure

[Kevin Wolf]

Am 16.05.2012 15:07, schrieb Jim Meyering:
> From: Jim Meyering <meyering@redhat.com>
> 
> 
> Signed-off-by: Jim Meyering <meyering@redhat.com>

Acked-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 4/6] sheepdog: don't leak socket file descriptor upon connection failure

[Jim Meyering]

Kevin Wolf wrote:
> Am 16.05.2012 15:07, schrieb Jim Meyering:
>> From: Jim Meyering <meyering@redhat.com>
>>
>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>
> Acked-by: Kevin Wolf <kwolf@redhat.com>

Hi Kevin,

AFAICS, only one of these 6 patches has been applied.
>From what I recall (it's been nearly 3mo), there was good
feedback and I posted at least one V2 patch.
For reference, here's the start of the series:

    http://marc.info/?l=qemu-devel&m=133717388221635&w=2

Let me know if there's anything I can do to help.

Jim Meyering (5):
      qemu-ga: don't leak a file descriptor upon failed lockf
      linux-user: do_msgrcv: don't leak host_mb upon TARGET_EFAULT failure
      sheepdog: don't leak socket file descriptor upon connection failure
      arm-semi: don't leak 1KB user string lock buffer upon TARGET_SYS_OPEN
      softmmu-semi: fix lock_user* functions not to deref NULL upon OOM

 block/sheepdog.c      |    1 +
 linux-user/syscall.c  |    4 ++--
 qemu-ga.c             |    3 +++
 softmmu-semi.h        |    5 ++++-
 target-arm/arm-semi.c |   13 +++++++------
 5 files changed, 17 insertions(+), 9 deletions(-)

Re: [Qemu-devel] [PATCH 4/6] sheepdog: don't leak socket file descriptor upon connection failure

[Kevin Wolf]

Am 17.08.2012 15:30, schrieb Jim Meyering:
> Kevin Wolf wrote:
>> Am 16.05.2012 15:07, schrieb Jim Meyering:
>>> From: Jim Meyering <meyering@redhat.com>
>>>
>>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>>
>> Acked-by: Kevin Wolf <kwolf@redhat.com>
> 
> Hi Kevin,
> 
> AFAICS, only one of these 6 patches has been applied.
> From what I recall (it's been nearly 3mo), there was good
> feedback and I posted at least one V2 patch.
> For reference, here's the start of the series:
> 
>     http://marc.info/?l=qemu-devel&m=133717388221635&w=2
> 
> Let me know if there's anything I can do to help.

Oh, that's bad. This series is spreads across several subsystems, so by
acking the sheepdog patch (the only block layer one) I was intending to
signal that I'm okay with merging it, but that I expect a "global
maintainer" to actually commit it.

Did all your other series get merged? There were a lot more patches with
small fixes and I can't see them in git master at all. I seem to
remember that they got delayed because you posted them late during the
last freeze, but obviously they should have been long committed now.

Anthony, what happened with these series? I think it makes sense to pull
them into -rc1 because all of them were bug fixes, even though mostly
minor ones.

Kevin

> Jim Meyering (5):
>       qemu-ga: don't leak a file descriptor upon failed lockf
>       linux-user: do_msgrcv: don't leak host_mb upon TARGET_EFAULT failure
>       sheepdog: don't leak socket file descriptor upon connection failure
>       arm-semi: don't leak 1KB user string lock buffer upon TARGET_SYS_OPEN
>       softmmu-semi: fix lock_user* functions not to deref NULL upon OOM
> 
>  block/sheepdog.c      |    1 +
>  linux-user/syscall.c  |    4 ++--
>  qemu-ga.c             |    3 +++
>  softmmu-semi.h        |    5 ++++-
>  target-arm/arm-semi.c |   13 +++++++------
>  5 files changed, 17 insertions(+), 9 deletions(-)
> 

Re: [Qemu-devel] [PATCH 4/6] sheepdog: don't leak socket file descriptor upon connection failure

[Jim Meyering]

Kevin Wolf wrote:

> Am 17.08.2012 15:30, schrieb Jim Meyering:
>> Kevin Wolf wrote:
>>> Am 16.05.2012 15:07, schrieb Jim Meyering:
>>>> From: Jim Meyering <meyering@redhat.com>
>>>>
>>>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>>>
>>> Acked-by: Kevin Wolf <kwolf@redhat.com>
>>
>> Hi Kevin,
>>
>> AFAICS, only one of these 6 patches has been applied.
>> From what I recall (it's been nearly 3mo), there was good
>> feedback and I posted at least one V2 patch.
>> For reference, here's the start of the series:
>>
>>     http://marc.info/?l=qemu-devel&m=133717388221635&w=2
>>
>> Let me know if there's anything I can do to help.
>
> Oh, that's bad. This series is spreads across several subsystems, so by
> acking the sheepdog patch (the only block layer one) I was intending to
> signal that I'm okay with merging it, but that I expect a "global
> maintainer" to actually commit it.
>
> Did all your other series get merged? There were a lot more patches with
> small fixes and I can't see them in git master at all. I seem to
> remember that they got delayed because you posted them late during the
> last freeze, but obviously they should have been long committed now.

I'm going through them now.
So far, it looks like most have been deferred.

[Qemu-devel] [PATCH 5/6] arm-semi: don't leak 1kb user string lock buffer upon TARGET_SYS_OPEN

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

Always call unlock_user before returning.

.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 arm-semi.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/arm-semi.c b/arm-semi.c
index 88ca9bb..5d2a2d2 100644
--- a/arm-semi.c
+++ b/arm-semi.c
@@ -194,18 +194,19 @@ uint32_t do_arm_semihosting(CPUARMState *env)
         if (!(s = lock_user_string(ARG(0))))
             /* FIXME - should this error code be -TARGET_EFAULT ? */
             return (uint32_t)-1;
-        if (ARG(1) >= 12)
+        if (ARG(1) >= 12) {
+            unlock_user(s, ARG(0), 0);
             return (uint32_t)-1;
+        }
         if (strcmp(s, ":tt") == 0) {
-            if (ARG(1) < 4)
-                return STDIN_FILENO;
-            else
-                return STDOUT_FILENO;
+            int result_fileno = ARG(1) < 4 ? STDIN_FILENO : STDOUT_FILENO;
+            unlock_user(s, ARG(0), 0);
+            return result_fileno;
         }
         if (use_gdb_syscalls()) {
             gdb_do_syscall(arm_semi_cb, "open,%s,%x,1a4", ARG(0),
 			   (int)ARG(2)+1, gdb_open_modeflags[ARG(1)]);
-            return env->regs[0];
+            ret = env->regs[0];
         } else {
             ret = set_swi_errno(ts, open(s, open_modeflags[ARG(1)], 0644));
         }
-- 
1.7.10.2.520.g6a4a482

Re: [Qemu-devel] [PATCH 5/6] arm-semi: don't leak 1kb user string lock buffer upon TARGET_SYS_OPEN

[Peter Maydell]

On 16 May 2012 14:08, Jim Meyering <jim@meyering.net> wrote:
> From: Jim Meyering <meyering@redhat.com>
>
> Always call unlock_user before returning.
>
> .

Gratuitous dot in your commit message here, but

>
> Signed-off-by: Jim Meyering <meyering@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

[Qemu-devel] [PATCH 6/6] softmmu-semi: fix lock_user* functions not to deref NULL upon OOM

[Jim Meyering]

From: Jim Meyering <meyering@redhat.com>

Use g_malloc/g_free in place of malloc/free.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
 softmmu-semi.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/softmmu-semi.h b/softmmu-semi.h
index 648cb95..996e0f7 100644
--- a/softmmu-semi.h
+++ b/softmmu-semi.h
@@ -39,7 +39,7 @@ static void *softmmu_lock_user(CPUArchState *env, uint32_t addr, uint32_t len,
 {
     uint8_t *p;
     /* TODO: Make this something that isn't fixed size.  */
-    p = malloc(len);
+    p = g_malloc(len);
     if (copy)
         cpu_memory_rw_debug(env, addr, p, len, 0);
     return p;
@@ -51,7 +51,7 @@ static char *softmmu_lock_user_string(CPUArchState *env, uint32_t addr)
     char *s;
     uint8_t c;
     /* TODO: Make this something that isn't fixed size.  */
-    s = p = malloc(1024);
+    s = p = g_malloc(1024);
     do {
         cpu_memory_rw_debug(env, addr, &c, 1, 0);
         addr++;
@@ -65,6 +65,6 @@ static void softmmu_unlock_user(CPUArchState *env, void *p, target_ulong addr,
 {
     if (len)
         cpu_memory_rw_debug(env, addr, p, len, 1);
-    free(p);
+    g_free(p);
 }
 #define unlock_user(s, args, len) softmmu_unlock_user(env, s, args, len)
-- 
1.7.10.2.520.g6a4a482

Re: [Qemu-devel] [PATCH 6/6] softmmu-semi: fix lock_user* functions not to deref NULL upon OOM

[Matthew Fernandez]

On 16 May 2012 23:08, Jim Meyering <jim@meyering.net> wrote:
> From: Jim Meyering <meyering@redhat.com>
>
> Use g_malloc/g_free in place of malloc/free.
>
> Signed-off-by: Jim Meyering <meyering@redhat.com>

Acked-by: Matthew Fernandez <matthew.fernandez@gmail.com>

> ---
>  softmmu-semi.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/softmmu-semi.h b/softmmu-semi.h
> index 648cb95..996e0f7 100644
> --- a/softmmu-semi.h
> +++ b/softmmu-semi.h
> @@ -39,7 +39,7 @@ static void *softmmu_lock_user(CPUArchState *env, uint32_t addr, uint32_t len,
>  {
>     uint8_t *p;
>     /* TODO: Make this something that isn't fixed size.  */
> -    p = malloc(len);
> +    p = g_malloc(len);
>     if (copy)
>         cpu_memory_rw_debug(env, addr, p, len, 0);
>     return p;
> @@ -51,7 +51,7 @@ static char *softmmu_lock_user_string(CPUArchState *env, uint32_t addr)
>     char *s;
>     uint8_t c;
>     /* TODO: Make this something that isn't fixed size.  */
> -    s = p = malloc(1024);
> +    s = p = g_malloc(1024);
>     do {
>         cpu_memory_rw_debug(env, addr, &c, 1, 0);
>         addr++;
> @@ -65,6 +65,6 @@ static void softmmu_unlock_user(CPUArchState *env, void *p, target_ulong addr,
>  {
>     if (len)
>         cpu_memory_rw_debug(env, addr, p, len, 1);
> -    free(p);
> +    g_free(p);
>  }
>  #define unlock_user(s, args, len) softmmu_unlock_user(env, s, args, len)
> --
> 1.7.10.2.520.g6a4a482
>

Re: [Qemu-devel] [PATCH 6/6] softmmu-semi: fix lock_user* functions not to deref NULL upon OOM

[Peter Maydell]

On 16 May 2012 14:08, Jim Meyering <jim@meyering.net> wrote:
> From: Jim Meyering <meyering@redhat.com>
>
> Use g_malloc/g_free in place of malloc/free.
>
> Signed-off-by: Jim Meyering <meyering@redhat.com>
> ---
>  softmmu-semi.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/softmmu-semi.h b/softmmu-semi.h
> index 648cb95..996e0f7 100644
> --- a/softmmu-semi.h
> +++ b/softmmu-semi.h
> @@ -39,7 +39,7 @@ static void *softmmu_lock_user(CPUArchState *env, uint32_t addr, uint32_t len,
>  {
>     uint8_t *p;
>     /* TODO: Make this something that isn't fixed size.  */
> -    p = malloc(len);
> +    p = g_malloc(len);
>     if (copy)
>         cpu_memory_rw_debug(env, addr, p, len, 0);
>     return p;

Nak. This function is called with a length passed from the guest, so
killing qemu if the length is too large is a bad idea. The callers
should handle it returning NULL on failure. (Most of them do already,
if any do not that's a bug.) The bug in this function is passing
NULL to cpu_memory_rw_debug().

-- PMM

Re: [Qemu-devel] [PATCH 6/6] softmmu-semi: fix lock_user* functions not to deref NULL upon OOM

[Jim Meyering]

Peter Maydell wrote:

> On 16 May 2012 14:08, Jim Meyering <jim@meyering.net> wrote:
>> From: Jim Meyering <meyering@redhat.com>
>>
>> Use g_malloc/g_free in place of malloc/free.
>>
>> Signed-off-by: Jim Meyering <meyering@redhat.com>
>> ---
>>  softmmu-semi.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/softmmu-semi.h b/softmmu-semi.h
>> index 648cb95..996e0f7 100644
>> --- a/softmmu-semi.h
>> +++ b/softmmu-semi.h
>> @@ -39,7 +39,7 @@ static void *softmmu_lock_user(CPUArchState *env,
>> uint32_t addr, uint32_t len,
>>  {
>>     uint8_t *p;
>>     /* TODO: Make this something that isn't fixed size.  */
>> -    p = malloc(len);
>> +    p = g_malloc(len);
>>     if (copy)
>>         cpu_memory_rw_debug(env, addr, p, len, 0);
>>     return p;
>
> Nak. This function is called with a length passed from the guest, so
> killing qemu if the length is too large is a bad idea. The callers
> should handle it returning NULL on failure. (Most of them do already,
> if any do not that's a bug.) The bug in this function is passing
> NULL to cpu_memory_rw_debug().

That makes sense.
I've adjusted as you suggest and posted a V2.

[Qemu-devel] [PATCH v2 6/6] softmmu-semi: fix lock_user* functions not to deref NULL upon OOM

[Jim Meyering]

Return NULL upon malloc failure.

Signed-off-by: Jim Meyering <meyering@redhat.com>
---
Improved based on suggestion from Peter Maydell:
Handle malloc failure rather than relying on g_malloc, since we
can't afford to let guest-provided "len" induce g_malloc's abort.

 softmmu-semi.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/softmmu-semi.h b/softmmu-semi.h
index 648cb95..bcb979a 100644
--- a/softmmu-semi.h
+++ b/softmmu-semi.h
@@ -40,7 +40,7 @@ static void *softmmu_lock_user(CPUArchState *env, uint32_t addr, uint32_t len,
     uint8_t *p;
     /* TODO: Make this something that isn't fixed size.  */
     p = malloc(len);
-    if (copy)
+    if (p && copy)
         cpu_memory_rw_debug(env, addr, p, len, 0);
     return p;
 }
@@ -52,6 +52,9 @@ static char *softmmu_lock_user_string(CPUArchState *env, uint32_t addr)
     uint8_t c;
     /* TODO: Make this something that isn't fixed size.  */
     s = p = malloc(1024);
+    if (!s) {
+        return NULL;
+    }
     do {
         cpu_memory_rw_debug(env, addr, &c, 1, 0);
         addr++;
--
1.7.10.2.565.gbd578b5