block: Don't forget to delete temporary file
PATCH 1/1 block: Don't forget to delete temporary file
From: Dunrong Huang <[email protected]>
The caller would not delete temporary file after failed get_tmp_filename().
--- block.c | 6 +++++-
1 个文件被修改，插入 5 行(+)，删除 1 行(-)
block.c +5 -1
@@ -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 ;
Signature · 1 line
1.7.12
Il 05/09/2012 15:26, [email protected] ha scritto:
Quoted 17 lines
From: Dunrong Huang <[email protected]>
The caller would not delete temporary file after failed
get_tmp_filename().Signed-off-by Dunrong Huang <[email protected]>--- block.c | 6 +++++-1 个文件被修改，插入 5 行(+)，删除 1 行(-)
block.c +0
-0@@ -433,7 +433,11 @@ int get_tmp_filename(char *filename, int size)return -EOVERFLOW ; }
block.c int get_tmp_filename(char *filename, int size) · line 443
Quoted 10 lines
block.c +5
-1fd =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
Hi, thanks for you reply.
2012/9/5 Paolo Bonzini <[email protected]>:
Quoted 22 lines
Il 05/09/2012 15:26, [email protected] ha scritto:
Quoted 21 lines
From: Dunrong Huang <[email protected]>
The caller would not delete temporary file after failed
get_tmp_filename().Signed-off-by Dunrong Huang <[email protected]>--- block.c | 6 +++++-1 个文件被修改，插入 5 行(+)，删除 1 行(-)
block.c +3
-1@@ -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; + }
Quoted 9 lines
block.c int get_tmp_filename(char *filename, int size) · line 443
Quoted 6 lines
block.c +2
-0+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.
Quoted 1 line
Paolo
Signature · 3 lines
Best Regards, Dunrong Huang
Quoted 22 lines · Paolo Bonzini
Il 05/09/2012 15:26, [email protected] ha scritto:
Quoted 21 lines
From: Dunrong Huang <[email protected]>
The caller would not delete temporary file after failed
get_tmp_filename().Signed-off-by Dunrong Huang <[email protected]>--- block.c | 6 +++++-1 个文件被修改，插入 5 行(+)，删除 1 行(-)
block.c +3
-1@@ -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; + }
Quoted 8 lines
block.c int get_tmp_filename(char *filename, int size) · line 443
Quoted 6 lines
block.c +2
-0+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 :)
Il 05/09/2012 18:02, Markus Armbruster ha scritto:
Quoted 33 lines
Quoted 22 lines · Paolo Bonzini
Il 05/09/2012 15:26, [email protected] ha scritto:
Quoted 21 lines
From: Dunrong Huang <[email protected]>
The caller would not delete temporary file after failed
get_tmp_filename().Signed-off-by Dunrong Huang <[email protected]>--- block.c | 6 +++++-1 个文件被修改，插入 5 行(+)，删除 1 行(-)
block.c +3
-1@@ -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; + }Quoted 8 lines
block.c int get_tmp_filename(char *filename, int size) · line 443
Quoted 6 lines
block.c +2
-0+if (close (fd) !=0 ) { +unlink (filename);return -errno; }return 0 ;Not necessary, mkstemp will not create a file if it returns an error.
Quoted 1 line
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
Quoted 6 lines · Paolo Bonzini
And finally, the whole
get_tmp_filenameis 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()...).
Signature · 2 lines
Eric Blake [email protected] +1-919-301-3266 Libvirt virtualization library http://libvirt.org
2012/9/6 Eric Blake <[email protected]>:
Quoted 11 lines · Paolo Bonzini
Quoted 6 lines
And finally, the whole
get_tmp_filenameis 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.
Quoted 11 lines
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 bymkstemp()...).--
Eric Blake [email protected] +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Signature · 3 lines
Best Regards, Dunrong Huang
Kevin Wolf
AppliedAm 05.09.2012 15:26, schrieb [email protected]:
Quoted 5 lines
From: Dunrong Huang <[email protected]>
The caller would not delete temporary file after failed
get_tmp_filename().Signed-off-by Dunrong Huang <[email protected]>
Thanks, applied to the block branch.
Kevin
Quoted 8 lines · Kevin Wolf
Am 05.09.2012 15:26, schrieb [email protected]:
Quoted 5 lines
From: Dunrong Huang <[email protected]>
The caller would not delete temporary file after failed
get_tmp_filename().Signed-off-by Dunrong Huang <[email protected]>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