[Qemu-devel] [PATCH] block: Don't forget to delete temporary file

[Qemu-devel] [PATCH] block: Don't forget to delete temporary file

[riegamaths]

From: Dunrong Huang <riegamaths@gmail.com>

The caller would not delete temporary file after failed get_tmp_filename().

Signed-off-by: Dunrong Huang <riegamaths@gmail.com>
---
 block.c | 6 +++++-
 1 个文件被修改，插入 5 行(+)，删除 1 行(-)

diff --git a/block.c b/block.c
index 074987e..2bc9f75 100644
--- a/block.c
+++ b/block.c
@@ -433,7 +433,11 @@ int get_tmp_filename(char *filename, int size)
         return -EOVERFLOW;
     }
     fd = mkstemp(filename);
-    if (fd < 0 || close(fd)) {
+    if (fd < 0) {
+        return -errno;
+    }
+    if (close(fd) != 0) {
+        unlink(filename);
         return -errno;
     }
     return 0;
-- 
1.7.12

Re: [Qemu-devel] [PATCH] block: Don't forget to delete temporary file

[Paolo Bonzini]

Il 05/09/2012 15:26, riegamaths@gmail.com ha scritto:
> From: Dunrong Huang <riegamaths@gmail.com>
> 
> The caller would not delete temporary file after failed get_tmp_filename().
> 
> Signed-off-by: Dunrong Huang <riegamaths@gmail.com>
> ---
>  block.c | 6 +++++-
>  1 个文件被修改，插入 5 行(+)，删除 1 行(-)
> 
> diff --git a/block.c b/block.c
> index 074987e..2bc9f75 100644
> --- a/block.c
> +++ b/block.c
> @@ -433,7 +433,11 @@ int get_tmp_filename(char *filename, int size)
>          return -EOVERFLOW;
>      }
>      fd = mkstemp(filename);
> -    if (fd < 0 || close(fd)) {
> +    if (fd < 0) {
> +        return -errno;
> +    }
> +    if (close(fd) != 0) {
> +        unlink(filename);
>          return -errno;
>      }
>      return 0;
> 

Not necessary, mkstemp will not create a file if it returns an error.

Paolo

Re: [Qemu-devel] [PATCH] block: Don't forget to delete temporary file

[Dunrong Huang]

Hi, thanks for you reply.
2012/9/5 Paolo Bonzini <pbonzini@redhat.com>:
> Il 05/09/2012 15:26, riegamaths@gmail.com ha scritto:
>> From: Dunrong Huang <riegamaths@gmail.com>
>>
>> The caller would not delete temporary file after failed get_tmp_filename().
>>
>> Signed-off-by: Dunrong Huang <riegamaths@gmail.com>
>> ---
>>  block.c | 6 +++++-
>>  1 个文件被修改，插入 5 行(+)，删除 1 行(-)
>>
>> diff --git a/block.c b/block.c
>> index 074987e..2bc9f75 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -433,7 +433,11 @@ int get_tmp_filename(char *filename, int size)
>>          return -EOVERFLOW;
>>      }
>>      fd = mkstemp(filename);
>> -    if (fd < 0 || close(fd)) {
>> +    if (fd < 0) {
>> +        return -errno;
>> +    }
>> +    if (close(fd) != 0) {
>> +        unlink(filename);
>>          return -errno;
>>      }
>>      return 0;
>>
>
> Not necessary, mkstemp will not create a file if it returns an error.
>
If we call mkstemp() successfully, but failed to close(fd),
in this case, the temporafy file will not be deleted even if QEMU exits.
> Paolo



-- 
Best Regards,

Dunrong Huang

Re: [Qemu-devel] [PATCH] block: Don't forget to delete temporary file

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 05/09/2012 15:26, riegamaths@gmail.com ha scritto:
>> From: Dunrong Huang <riegamaths@gmail.com>
>> 
>> The caller would not delete temporary file after failed get_tmp_filename().
>> 
>> Signed-off-by: Dunrong Huang <riegamaths@gmail.com>
>> ---
>>  block.c | 6 +++++-
>>  1 个文件被修改，插入 5 行(+)，删除 1 行(-)
>> 
>> diff --git a/block.c b/block.c
>> index 074987e..2bc9f75 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -433,7 +433,11 @@ int get_tmp_filename(char *filename, int size)
>>          return -EOVERFLOW;
>>      }
>>      fd = mkstemp(filename);
>> -    if (fd < 0 || close(fd)) {
>> +    if (fd < 0) {
>> +        return -errno;
>> +    }
>> +    if (close(fd) != 0) {
>> +        unlink(filename);
>>          return -errno;
>>      }
>>      return 0;
>> 
>
> Not necessary, mkstemp will not create a file if it returns an error.

Read the patch once more :)

Re: [Qemu-devel] [PATCH] block: Don't forget to delete temporary file

[Paolo Bonzini]

Il 05/09/2012 18:02, Markus Armbruster ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> Il 05/09/2012 15:26, riegamaths@gmail.com ha scritto:
>>> From: Dunrong Huang <riegamaths@gmail.com>
>>>
>>> The caller would not delete temporary file after failed get_tmp_filename().
>>>
>>> Signed-off-by: Dunrong Huang <riegamaths@gmail.com>
>>> ---
>>>  block.c | 6 +++++-
>>>  1 个文件被修改，插入 5 行(+)，删除 1 行(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 074987e..2bc9f75 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -433,7 +433,11 @@ int get_tmp_filename(char *filename, int size)
>>>          return -EOVERFLOW;
>>>      }
>>>      fd = mkstemp(filename);
>>> -    if (fd < 0 || close(fd)) {
>>> +    if (fd < 0) {
>>> +        return -errno;
>>> +    }
>>> +    if (close(fd) != 0) {
>>> +        unlink(filename);
>>>          return -errno;
>>>      }
>>>      return 0;
>>>
>>
>> Not necessary, mkstemp will not create a file if it returns an error.
> 
> Read the patch once more :)

Oops. :)

I wondered about that check for close() errors, perhaps an error in
close could just be swallowed?  Since we don't care about the content of
the file after close (it's empty), we also don't care about errors
closing it.

However, perhaps there were errors writing the directory entry...
Should any errors writing the directory entry be reported by open(),
i.e. by mkstemp()?   If not, and the dcache entry is evicted, someone
could create a different file with the same name as ours.  But then, not
even a successful close() guarantees that the data has reached the disk.

And finally, the whole get_tmp_filename is unsafe because there is a
race window between closing and reopening the file, if the directory is
writable and does not have the sticky bit.

So the patch is an improvement, but there is still something unpleasing
in this code...

Paolo

Re: [Qemu-devel] [PATCH] block: Don't forget to delete temporary file

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

On 09/05/2012 10:23 AM, Paolo Bonzini wrote:
> And finally, the whole get_tmp_filename is unsafe because there is a
> race window between closing and reopening the file, if the directory is
> writable and does not have the sticky bit.
> 
> So the patch is an improvement, but there is still something unpleasing
> in this code...

I absolutely agree that there is a nasty race here.  If you aren't going
to use the fd, then mktemp() is sufficient (and just as racy, but then
you are at least honest that you don't care about the race); in all
other situations, if you want a temporary file name but want to avoid a
race, then it feels like you should be returning the fd from mkstemp()
still open (or at a bare minimum, auditing ALL callers to make sure they
only use the temporary name with O_CREAT|O_EXCL, and that they retry in
a loop in case they lose the race, at which point they are reinventing
the loop already done on their behalf by mkstemp()...).

-- 
Eric Blake   eblake@redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 617 bytes --]

Re: [Qemu-devel] [PATCH] block: Don't forget to delete temporary file

[Dunrong Huang]

2012/9/6 Eric Blake <eblake@redhat.com>:
> On 09/05/2012 10:23 AM, Paolo Bonzini wrote:
>> And finally, the whole get_tmp_filename is unsafe because there is a
>> race window between closing and reopening the file, if the directory is
>> writable and does not have the sticky bit.
>>
>> So the patch is an improvement, but there is still something unpleasing
>> in this code...
>
> I absolutely agree that there is a nasty race here.  If you aren't going
> to use the fd, then mktemp() is sufficient (and just as racy, but then
> you are at least honest that you don't care about the race); in all
Yes, using mktemp() in get_tmp_filename() is ok because we dont
care  about race, but for old gcc version, e.g. for version 4.4, we will get
a annoying unsecure warning "warning: the use of `mktemp' is
dangerous, better use `mkstemp'",
which breaks build.

> other situations, if you want a temporary file name but want to avoid a
> race, then it feels like you should be returning the fd from mkstemp()
> still open (or at a bare minimum, auditing ALL callers to make sure they
> only use the temporary name with O_CREAT|O_EXCL, and that they retry in
> a loop in case they lose the race, at which point they are reinventing
> the loop already done on their behalf by mkstemp()...).
>
> --
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>



-- 
Best Regards,

Dunrong Huang

Re: [Qemu-devel] [PATCH] block: Don't forget to delete temporary file

[Kevin Wolf]

Am 05.09.2012 15:26, schrieb riegamaths@gmail.com:
> From: Dunrong Huang <riegamaths@gmail.com>
> 
> The caller would not delete temporary file after failed get_tmp_filename().
> 
> Signed-off-by: Dunrong Huang <riegamaths@gmail.com>

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH] block: Don't forget to delete temporary file

[Stefan Hajnoczi]

On Tue, Sep 11, 2012 at 12:23:37PM +0200, Kevin Wolf wrote:
> Am 05.09.2012 15:26, schrieb riegamaths@gmail.com:
> > From: Dunrong Huang <riegamaths@gmail.com>
> > 
> > The caller would not delete temporary file after failed get_tmp_filename().
> > 
> > Signed-off-by: Dunrong Huang <riegamaths@gmail.com>
> 
> Thanks, applied to the block branch.

For the record, using the close(2) errno after calling unlink(2) isn't
a good idea.  We should follow the docs and preserve errno carefully.

It's the exact issue I pointed out on the original patch.

Stefan