Re: Safeguards against incorrect fd flags for fsync()

On Thu, Nov 07, 2019 at 01:57:57PM -0800, Mark Dilger wrote:
> The code and comments don't clearly indicate what you have said in the
> email, that you are verifying directories are opened read-only and files are
> opened either read-write or write-only. I'd recommend changing the comments
> a bit to make that clearer.

Thanks for the suggestions, sounds fine to me.

> I would also rearrange the code a little, as it is slightly clearer to read:
>
> if (x)
> /* directory stuff */
> else
> /* file stuff */
>
> than as you have it:
>
> if (!x)
> /* file stuff */
> else
> /* directory stuff */

The check order in the former patch is consistent with what's done at
the top of fsync_fname_ext(), still I can see your point. So let's do
as you suggest.

> I'm a little uncertain about ignoring fstat errors as you do, but left that
> part of the logic alone. I understand that any fstat error will likely be
> immediately followed by another error when the fsync is attempted, but
> relying on that seems vaguely similar to the security vulnerability of
> checking permissions and then opening a file as two separate operations.
> Not sure the analogy actually holds for fstat before fsync, though.

The only possible error which could be expected here would be a ENOENT
so we could filter after that, but fsync() would most likely complain
about that so it sounds better to let it do its work with its own
logging, which would be more helpful for the user, if of course we
have fsync=on in postgresql.conf.

> Attached is a revised version of the patch. Perhaps you can check what I've
> done and tell me if I've broken it.

Thanks for the review. I was wondering why I did not do that as well
for file_utils.c, just to find out that fsync_fname() is the only
entry point in file_utils.c. Anyway, the patch had a problem
regarding fcntl() which is not available on Windows (see for example
pg_set_noblock in noblock.c). Performing the sanity check will allow
to catch any problems for all platforms we support, so let's just skip
it for Windows. For this reason it is better as well to update errno
to 0 after the fstat() call. Who knows... Attached is an updated
version, with your changes included. How does that look?
--
Michael