From: | Mark Dilger <hornschnorter(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Safeguards against incorrect fd flags for fsync() |
Date: | 2019-11-25 04:18:38 |
Message-ID: | 724f2e0a-9fd5-6e8a-5188-0fad8eb91bf7@gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 11/24/19 6:53 PM, Mark Dilger wrote:
>
>
> 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....
Ok, it passes all regression tests, and I played around with
intentionally breaking the code to open file descriptors in
the wrong mode. The assertion appears to work as intended.
I'd say this is ready for commit.
--
Mark Dilger
From | Date | Subject | |
---|---|---|---|
Next Message | Surafel Temesgen | 2019-11-25 05:55:52 | Re: Conflict handling for COPY FROM |
Previous Message | Dilip Kumar | 2019-11-25 03:52:49 | Fastpath while arranging the changes in LSN order in logical decoding |