Re: Dereference before NULL check (src/backend/storage/ipc/latch.c)

At Mon, 02 Nov 2020 14:33:40 +0900 (JST), Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> wrote in
> At Mon, 2 Nov 2020 16:22:09 +1300, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote in
> > On Mon, Nov 2, 2020 at 1:49 PM Kyotaro Horiguchi
> > <horikyota(dot)ntt(at)gmail(dot)com> wrote:
> > > At Sat, 31 Oct 2020 11:40:53 -0300, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote in
> > > > Per Coverity.
> > > >
> > > > If test set->latch against NULL, is why it can be NULL.
> > > > ResetEvent can dereference NULL.
> > >
> > > If the returned event is WL_LATCH_SET, set->latch cannot be NULL. We
> > > shouldn't inadvertently ignore the unexpected or broken situation.We
> > > could put Assert instead, but I think that we don't need do something
> > > here at all since SIGSEGV would be raised at the right location.
> >
> > Hmm. I changed that to support set->latch == NULL, so that you can
> > use the long lived WES in the rare code paths that call WaitLatch()
> > without a latch (for example the code I proposed at [1]). The Windows
>
> Ooo. We don't update epoll events in that case. Ok, I understand
> WL_LATCH_SET can fire while set->latch == NULL.
>
> (I was confused by WaitEventAdjust* asserts set->latch != NULL for
> WL_LATCH_SET. Isn't it better if we moved the check on latch from
> ModifyWaitEvent() to WaitEventAdjust*()?))
>
> > version leaves the event handle of the most recently used latch in
> > set->handles[n] (because AFAICS there is no way to have a "hole" in
> > the handles array). The event can fire while you are waiting on "no
> > latch". Perhaps it should be changed to
> > ResetEvent(set->handles[cur_event->pos + 1])?
> >
> > [1] https://www.postgresql.org/message-id/flat/CA%2BhUKGK1607VmtrDUHQXrsooU%3Dap4g4R2yaoByWOOA3m8xevUQ%40mail.gmail.com
>
> Seems right. Just doing that *seems* to work fine, but somehow I
> cannot build on Windows for now...

That was caused by a leftover change on config_default.pl I made when
I tried to enable NLS.

I called SetLatch() during WaitLatch(NULL, ) but that doesn't fire
WL_LATCH_SET event for me on Windows. (I got it fired on Linux..) On
Windows, the latch is detected after exiting the WaitLatch()
call. Seems like MyLatch of waiter is different from
peerPGPROC->procLatch. And... an update for Visual Studio broke my
environment... I will investigate this further but everything feel
cumbersome on Windows...

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center