| Message ID | 1346851582-9296-1-git-send-email-riegamaths@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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 :)
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
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()...).
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 >
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
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
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;