[PATCH 1/2] Cygwin: lockf: Fix access violation in lf_clearlock().

[Takashi Yano]

On Tue, 19 Nov 2024 10:49:39 +0100
Corinna Vinschen wrote:
> On Nov 19 17:39, Takashi Yano wrote:
> > On Mon, 18 Nov 2024 16:55:14 +0100
> > Corinna Vinschen wrote:
> > > On Nov 15 22:14, Takashi Yano wrote:
> > > > The commit ae181b0ff122 has a bug that the pointer is referred bofore
> > > > NULL check in the function lf_clearlock(). This patch fixes that.
> > > > 
> > > > Addresses: https://cygwin.com/pipermail/cygwin/2024-November/256750.html
> > > > Fixes: ae181b0ff122 ("Cygwin: lockf: Make lockf() return ENOLCK when too many locks")
> > > > Reported-by: Sebastian Feld <sebastian.n.feld@gmail.com>
> > > > Reviewed-by:
> > > > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> > > > ---
> > > >  winsup/cygwin/flock.cc | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/winsup/cygwin/flock.cc b/winsup/cygwin/flock.cc
> > > > index 3821bddd6..794e66bd7 100644
> > > > --- a/winsup/cygwin/flock.cc
> > > > +++ b/winsup/cygwin/flock.cc
> > > > @@ -1524,6 +1524,10 @@ lf_clearlock (lockf_t *unlock, lockf_t **clean, HANDLE fhdl)
> > > >    lockf_t *lf = *head;
> > > >    lockf_t *overlap, **prev;
> > > >    int ovcase;
> > > > +
> > > > +  if (lf == NOLOCKF)
> > > > +    return 0;
> > > > +
> > > >    inode_t *node = lf->lf_inode;
> > > >    tmp_pathbuf tp;
> > > >    node->i_all_lf = (lockf_t *) tp.w_get ();
> > > > @@ -1531,8 +1535,6 @@ lf_clearlock (lockf_t *unlock, lockf_t **clean, HANDLE fhdl)
> > > >    uint32_t lock_cnt = node->get_lock_count ();
> > > >    bool first_loop = true;
> > > >  
> > > > -  if (lf == NOLOCKF)
> > > > -    return 0;
> > > >    prev = head;
> > > >    while ((ovcase = lf_findoverlap (lf, unlock, SELF, &prev, &overlap)))
> > > >      {
> > > > -- 
> > > > 2.45.1
> > > 
> > > LGTM, please push.
> > 
> > Thanks for reviewing this patch. Could you please review
> >  [PATCH v2] Cygwin: flock: Fix overlap handling in lf_setlock() and lf_clearlock()
> > as well?
> 
> Give me a bit of time.  While the patch might fix the problem, what
> bugs me is the deviation from upstream code.  We will at least need
> a few comments to explain why we don't follow the upstream behaviour.

I've got it. Does this code come from 'upstream'? From what code?
Essentially, the ovcase 1 can be a part of ovcase 3. I guess the
'upstream' does not add lock entry having same lock range unlike
current cygwin (lf_ver related). So, ovcase 1 can break after
handling 1 overlap. However, we need find overlap repeatedly
because we have lf_ver.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>