Re: Race condition in vim_tempname() on Windows
On 20/02/09 05:45, Bram Moolenaar wrote: Matt Wozniski wrote: src/fileio.c : vim_tempname() contains these lines: if (GetTempFileName(szTempFile, buf4, 0, itmp) == 0) return NULL; /* GetTempFileName() will create the file, we don't want that */ (void)DeleteFile(itmp); Is this really right? Is there any reason to call DeleteFile() here? On a quick glance, it seems that everyone who gets a name back is just doing an mch_open(tempname, w) on it, which doesn't care whether or not the file exists already. I ask because this seems to be causing a race condition: GetTempFileName() checks if a file exists, finds a name that doesn't exist, creates it, and returns the name back to the caller. It creates it so that future calls to GetTempFileName() won't choose the same file, since it now exists. But, deleting it before being done with it introduces a race condition: vim calls GetTempFileName(), then DeleteFile(), then another process calls GetTempFileName(), and now vim has the path to a file that didn't exist when it asked for a unique, non-existant file, but exists now, and is no longer safe for vim to use. I have never seen the situation that another process (or Vim itself) gets the same file name. I don't have access to a windows system right now, but if I remember correctly the names look like a random sequence of characters. For Unix we create our own directory, which is a safe way to avoid others to take over our file unexpectedly. Not all code that invokes vim_tempname() starts writing to the file. Most notable is the tempname() function. Changing this behavior will not be easy, so you will have to come up with some kind of proof that the current mechanism may fail. Also, what about the case when a script might use tempname() in order to get a unique name for a directory? If you (Matt) leave the file lying around, directory creation will IIUC be impossible, which breaks compatibility with existing behaviour. Best regards, Tony. -- Somebody ought to cross ball point pens with coat hangers so that the pens will multiply instead of disappear. --~--~-~--~~~---~--~~ You received this message from the vim_dev maillist. For more information, visit http://www.vim.org/maillist.php -~--~~~~--~~--~--~---
Re: Race condition in vim_tempname() on Windows
On Fri, Feb 20, 2009 at 10:16, Tony Mechelynck antoine.mechely...@gmail.com wrote: On 20/02/09 05:45, Bram Moolenaar wrote: Changing this behavior will not be easy, so you will have to come up with some kind of proof that the current mechanism may fail. Also, what about the case when a script might use tempname() in order to get a unique name for a directory? If you (Matt) leave the file lying around, directory creation will IIUC be impossible, which breaks compatibility with existing behaviour. We recently discussed the security issues of temporary files on this list. Having functions that only return the name of a temporary files is insufficient. You need functions that return the actual file handle to be secure (along with a bunch of other constraints). I realize that this isn't something that can be fixed in a simple manner in VimL. --~--~-~--~~~---~--~~ You received this message from the vim_dev maillist. For more information, visit http://www.vim.org/maillist.php -~--~~~~--~~--~--~---
Re: Race condition in vim_tempname() on Windows
Nikolai Weibull wrote: On Fri, Feb 20, 2009 at 10:16, Tony Mechelynck antoine.mechely...@gmail.com wrote: On 20/02/09 05:45, Bram Moolenaar wrote: Changing this behavior will not be easy, so you will have to come up with some kind of proof that the current mechanism may fail. Also, what about the case when a script might use tempname() in order to get a unique name for a directory? If you (Matt) leave the file lying around, directory creation will IIUC be impossible, which breaks compatibility with existing behaviour. We recently discussed the security issues of temporary files on this list. Having functions that only return the name of a temporary files is insufficient. You need functions that return the actual file handle to be secure (along with a bunch of other constraints). I realize that this isn't something that can be fixed in a simple manner in VimL. It's secure if the directory where the file (or directory) is located is only writable by the user. What you talk about is when using a temp directory that is shared between users. In that case someone else may be able to rename the file you created and put another one in its place. -- hundred-and-one symptoms of being an internet addict: 95. Only communication in your household is through email. /// Bram Moolenaar -- b...@moolenaar.net -- http://www.Moolenaar.net \\\ ///sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\download, build and distribute -- http://www.A-A-P.org/// \\\help me help AIDS victims -- http://ICCF-Holland.org/// --~--~-~--~~~---~--~~ You received this message from the vim_dev maillist. For more information, visit http://www.vim.org/maillist.php -~--~~~~--~~--~--~---
Re: Race condition in vim_tempname() on Windows
Matt Wozniski wrote: src/fileio.c : vim_tempname() contains these lines: if (GetTempFileName(szTempFile, buf4, 0, itmp) == 0) return NULL; /* GetTempFileName() will create the file, we don't want that */ (void)DeleteFile(itmp); Is this really right? Is there any reason to call DeleteFile() here? On a quick glance, it seems that everyone who gets a name back is just doing an mch_open(tempname, w) on it, which doesn't care whether or not the file exists already. I ask because this seems to be causing a race condition: GetTempFileName() checks if a file exists, finds a name that doesn't exist, creates it, and returns the name back to the caller. It creates it so that future calls to GetTempFileName() won't choose the same file, since it now exists. But, deleting it before being done with it introduces a race condition: vim calls GetTempFileName(), then DeleteFile(), then another process calls GetTempFileName(), and now vim has the path to a file that didn't exist when it asked for a unique, non-existant file, but exists now, and is no longer safe for vim to use. I have never seen the situation that another process (or Vim itself) gets the same file name. I don't have access to a windows system right now, but if I remember correctly the names look like a random sequence of characters. For Unix we create our own directory, which is a safe way to avoid others to take over our file unexpectedly. Not all code that invokes vim_tempname() starts writing to the file. Most notable is the tempname() function. Changing this behavior will not be easy, so you will have to come up with some kind of proof that the current mechanism may fail. -- From know your smileys: |-) Chinese |-( Chinese and doesn't like these kind of jokes /// Bram Moolenaar -- b...@moolenaar.net -- http://www.Moolenaar.net \\\ ///sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\ \\\download, build and distribute -- http://www.A-A-P.org/// \\\help me help AIDS victims -- http://ICCF-Holland.org/// --~--~-~--~~~---~--~~ You received this message from the vim_dev maillist. For more information, visit http://www.vim.org/maillist.php -~--~~~~--~~--~--~---
Race condition in vim_tempname() on Windows
src/fileio.c : vim_tempname() contains these lines: if (GetTempFileName(szTempFile, buf4, 0, itmp) == 0) return NULL; /* GetTempFileName() will create the file, we don't want that */ (void)DeleteFile(itmp); Is this really right? Is there any reason to call DeleteFile() here? On a quick glance, it seems that everyone who gets a name back is just doing an mch_open(tempname, w) on it, which doesn't care whether or not the file exists already. I ask because this seems to be causing a race condition: GetTempFileName() checks if a file exists, finds a name that doesn't exist, creates it, and returns the name back to the caller. It creates it so that future calls to GetTempFileName() won't choose the same file, since it now exists. But, deleting it before being done with it introduces a race condition: vim calls GetTempFileName(), then DeleteFile(), then another process calls GetTempFileName(), and now vim has the path to a file that didn't exist when it asked for a unique, non-existant file, but exists now, and is no longer safe for vim to use. ~Matt --~--~-~--~~~---~--~~ You received this message from the vim_dev maillist. For more information, visit http://www.vim.org/maillist.php -~--~~~~--~~--~--~---
