[Vm-dev] Please review and merge: A commit to fix race conditions is sqFileOpen(), buffer overflows in sqUnixFile.c, and add support for a new primitive

Fabio Niephaus lists at fniephaus.com
Tue Oct 25 08:48:22 UTC 2016


On Tue, Oct 25, 2016 at 2:48 AM monty <monty2 at 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).
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20161025/45f201cf/attachment.htm


More information about the Vm-dev mailing list