Patchwork block: Don't forget to delete temporary file

login
register
mail settings
Submitter dunrong huang
Date Sept. 5, 2012, 1:26 p.m.
Message ID <1346851582-9296-1-git-send-email-riegamaths@gmail.com>
Download mbox | patch
Permalink /patch/181858/
State New
Headers show

Comments

dunrong huang - Sept. 5, 2012, 1:26 p.m.
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 行(-)
Paolo Bonzini - Sept. 5, 2012, 3:40 p.m.
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
dunrong huang - Sept. 5, 2012, 3:51 p.m.
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
Markus Armbruster - Sept. 5, 2012, 4:02 p.m.
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 :)
Paolo Bonzini - Sept. 5, 2012, 4:23 p.m.
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
Eric Blake - Sept. 5, 2012, 4:28 p.m.
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()...).
dunrong huang - Sept. 6, 2012, 3:47 a.m.
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
>
Kevin Wolf - Sept. 11, 2012, 10:23 a.m.
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
Stefan Hajnoczi - Sept. 14, 2012, 7:45 a.m.
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

Patch

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;