Re: Safeguards against incorrect fd flags for fsync()

On 10/8/19 11:26 PM, Michael Paquier wrote:
> Hi all,
> 
> After the set of issues discussed here, it seems to me that it would
> be a good thing to have some safeguards against incorrect flags when
> opening a fd which would be used for fsync():
> https://www.postgresql.org/message-id/16039-196fc97cc05e141c@postgresql.org
> 
> Attached is a patch aimed at doing that.  Historically O_RDONLY is 0,
> so when looking at a directory we just need to make sure that no write
> flags are used.  For files, that's the contrary, a write flag has to
> be used.
> 
> Thoughts or better ideas?

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.

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 */

because it takes slightly less time for somebody reading the code when 
they don't have to think about the negation of x.

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.

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


-- 
Mark Dilger