Re: BUG #14929: Unchecked AllocateDir() return value in restoreTwoPhaseData()

On Mon, Nov 27, 2017 at 8:09 PM, Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> On 2017/11/27 19:53, Michael Paquier wrote:
>> On Mon, Nov 27, 2017 at 6:31 PM, <bianpan2016(at)163(dot)com> wrote:
>>> AllocateDir() will return a NULL pointer if it fails to open the specified
>>> directory. However, in function restoreTwoPhaseData(), its return value is
>>> not checked. This may result in a NULL pointer dereference when trying to
>>> free it (see line 1759).
>>
>> You are missing the fact that ReadDir goes through ReadDirExtended,
>> which drops an ERROR log if the folder allocated is NULL.
>
> I noticed that too, but isn't possible that elevel might be such that we
> end up returning to restoreTwoPhaseData() after all and hit the line in it
> that will then dereference the NULL cldir? Maybe, that never happens
> because, elevel is never less than ERROR in that code path?

I don't quite follow. ReadDir is called at least once via
ReadDirExtended with elevel = ERROR. So if the input folder is NULL,
the call to ReadDir directly fails so you would not face a pointer
dereference. Something that can turn wrong though is the error message
generated if errno is changed in LWLockAcquire between the calls of
AllocateDir and ReadDir. For that the current code pattern is actually
incorrect.
--
Michael