Please review and merge: A commit to fix race conditions is sqFileOpen(), buffer overflows in sqUnixFile.c, and add support for a new primitive
This commit fixes race conditions in sqFilePluginBasicPrims.c's sqFileOpen() and buffer overflows in sqUnixFile.c and adds sqFileOpenNew() to support a new primitive to open only new files for IO atomically using open() with O_CREAT|O_EXCL and the equivalent on Windows, failing if they already exist. This will hopefully eventually replace any in-image code that tests for a file's existence before opening it for writing and creation/truncation, a classic race condition.
I don't have a Github account so I attached the commit as a "git format-patch" patch that you can apply with "git am". Details of the issues fixed follow.
sqFileOpen() issues:
Look at the current implementation. It tries fopen() with "r+b", reading/writing of an existing file, then if that fails, with "w+b", reading/writing of a new or truncated file. If the file is created and modified elsewhere between the first and second fopen(), you get unintentional truncation with potential data loss. There's also a minor race condition involving the setting of Mac file characteristics. The fix is replacing use of fopen() with the more general sys call open() and fdopen() after and proper error checking throughout.
sqUnixFile.c issues:
Evaluate this in a Pharo5/6 image with a recent SpurVM: name := String newFrom: ((1 to: 10000) collect: [ :e | $a ]). (name, '/', name) asFileReference writeStream. it should crash with a stack trace with dir_EntryLookup() on top. I fixed it by cleaning up sqUnixFile.c, making it use safer string functions adapted from BSD's strlcpy()/strlcat(), added checks throughout for truncation, removed obsolete includes, and made unexternally referenced global vars static. The iOS branch uses strlcpy()/strlcat() directly, but we sadly can't assume all non-BSDs like Linux have them, so I inserted renamed versions, which is common: http://marc.info/?l=openbsd-tech&m=138733933417096&w=2
The diff is smaller than it looks, because I ran sqUnixFile.c through Lindent to make it easier for me to edit and replaced the function-generating macros in sqUnixCharConv.c with their output ("gcc -E" with minor awk post-processing).
On Tue, Oct 25, 2016 at 2:48 AM monty monty2@programmer.net wrote:
This commit fixes race conditions in sqFilePluginBasicPrims.c's sqFileOpen() and buffer overflows in sqUnixFile.c and adds sqFileOpenNew() to support a new primitive to open only new files for IO atomically using open() with O_CREAT|O_EXCL and the equivalent on Windows, failing if they already exist. This will hopefully eventually replace any in-image code that tests for a file's existence before opening it for writing and creation/truncation, a classic race condition.
I don't have a Github account so I attached the commit as a "git format-patch" patch that you can apply with "git am". Details of the issues fixed follow.
Thanks for submitting the patch. I've opened the following pull request on your behalf: https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/82
I'm not sure who will review this, but I think it's clear that we need to discuss this here on the mailing list. :)
Fabio
sqFileOpen() issues:
Look at the current implementation. It tries fopen() with "r+b", reading/writing of an existing file, then if that fails, with "w+b", reading/writing of a new or truncated file. If the file is created and modified elsewhere between the first and second fopen(), you get unintentional truncation with potential data loss. There's also a minor race condition involving the setting of Mac file characteristics. The fix is replacing use of fopen() with the more general sys call open() and fdopen() after and proper error checking throughout.
sqUnixFile.c issues:
Evaluate this in a Pharo5/6 image with a recent SpurVM: name := String newFrom: ((1 to: 10000) collect: [ :e | $a ]). (name, '/', name) asFileReference writeStream. it should crash with a stack trace with dir_EntryLookup() on top. I fixed it by cleaning up sqUnixFile.c, making it use safer string functions adapted from BSD's strlcpy()/strlcat(), added checks throughout for truncation, removed obsolete includes, and made unexternally referenced global vars static. The iOS branch uses strlcpy()/strlcat() directly, but we sadly can't assume all non-BSDs like Linux have them, so I inserted renamed versions, which is common: http://marc.info/?l=openbsd-tech&m=138733933417096&w=2
The diff is smaller than it looks, because I ran sqUnixFile.c through Lindent to make it easier for me to edit and replaced the function-generating macros in sqUnixCharConv.c with their output ("gcc -E" with minor awk post-processing).
vm-dev@lists.squeakfoundation.org
-
Fabio Niephaus -
monty