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

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(-)

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
>

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.