Re: [Chicken-hackers] [PATCH] Two minor file-related patches

[Peter Bex]

On Fri, Sep 15, 2017 at 10:07:18PM +1200, Evan Hanson wrote:
> Hi hackers,
> 
> Here are two small patches. The first just removes a helper procedure
> that I think is no longer necessary, `##sys#platform-fixup-pathname`,
> which only affects MinGW and doesn't seem to be needed anymore (see the
> commit message for details about that).

It seems that this patch is necessary after all.  I tested with the
"regular" MingW with MSYS (for some reason csi just hangs on MSYS2...)
and I see the following:

#;1> (use (chicken file))
#;2> (directory-exists? "c:/cygwin")
"c:/cygwin"
#;3> (directory-exists? "c:/cygwin/")
#f

In the old implementation it returned the path in both calls.  So, as
you can see it seems this functionality is required after all.

I do agree that fixup-pathname should probably remove all trailing
path separators rather than just the last one; that's just broken.

> The second patch fixes a TOCTTOU
> problem with `delete-file*`, brings its return value in line with the
> documentation, and also speeds it up by about 2x judging by my testing.

This also changes the semantics drastically.  When using delete-file*,
you generally do so because you're not interested if it doesn't exist;
you're just interested in the end effect, that the file no longer exists
after having called the procedure.

However, if the file *can't* be deleted for whatever reason, the original
implementation would raise an exception, whereas the version after this
patch will merrily return #f and continue the program.

In other words, this will hide *all* errors, which can result in the program
to fail specatularly because its preconditions don't hold.  I believe
we need to check errno in case the return value is nonzero, and only
ignore ENOENT, or am I missing some other return codes?  I don't know
what to do with EAGAIN/EWOULDBLOCK: POSIX doesn't mention these codes
in remove(), but maybe some implementations could return them anyway?

This page seems to imply that unlink() (and by extension remove()) may
return EAGAIN:
https://www.kernel.org/doc/htmldocs/filesystems/API-vfs-unlink.html

Cheers,
Peter

signature.asc
Description: PGP signature