From: Mark Dilger <hornschnorter@gmail.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Safeguards against incorrect fd flags for fsync()
Date: Sun, 24 Nov 2019 18:53:35 -0800
Message-ID: <107de5d3-6972-7566-04d1-4776b677b462@gmail.com> (raw)
In-Reply-To: <20191125022858.GE37821@paquier.xyz>
References: <20191009062640.GB21379@paquier.xyz>
<82db297a-74f7-ca23-545b-3c531d692e4d@gmail.com>
<20191125022858.GE37821@paquier.xyz>
On 11/24/19 6:28 PM, Michael Paquier wrote:
> 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?
That looks great, thank you, but I have not tested it yet. I'll go do
that now....
--
Mark Dilger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: hornschnorter@gmail.com, michael@paquier.xyz, pgsql-hackers@lists.postgresql.org
Subject: Re: Safeguards against incorrect fd flags for fsync()
In-Reply-To: <107de5d3-6972-7566-04d1-4776b677b462@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox