On Sun 2020.03.15 at 15:45 +0100, Otto Moerbeek wrote:
> On Sun, Mar 15, 2020 at 10:09:53AM -0400, Okan Demirmen wrote:
>
> > On Sat 2020.03.14 at 18:33 +0100, Tim van der Molen wrote:
> > > Stefan Sperling (2020-03-14 18:12 +0100):
> > > > On Sat, Mar 14, 2020 at 05:36:46PM +0100, Otto Moerbeek wrote:
> > > > > Looking a the diff, I do see a use-after-free.
> > > >
> > > > The problem is the use of TAILQ_FOREACH(wn, ...) instead of
> > > > TAILQ_FOREACH_SAFE(wn, ...) in combination with free(wn), correct?
> > >
> > > Most likely, yes.
> >
> > Revised diff using TAILQ_FOREACH_SAFE.
>
> Doesn't seem right.
>
> >
> > Index: client.c
> > ===================================================================
> > RCS file: /home/open/cvs/xenocara/app/cwm/client.c,v
> > retrieving revision 1.260
> > diff -u -p -r1.260 client.c
> > --- client.c 14 Mar 2020 16:11:09 -0000 1.260
> > +++ client.c 14 Mar 2020 18:36:49 -0000
> > @@ -667,22 +667,24 @@ client_close(struct client_ctx *cc)
> > void
> > client_set_name(struct client_ctx *cc)
> > {
> > - struct winname *wn;
> > - char *newname;
> > + struct winname *wn, *wmnxt;
> > int i = 0;
> >
> > - if (!xu_get_strprop(cc->win, ewmh[_NET_WM_NAME], &newname))
> > - if (!xu_get_strprop(cc->win, XA_WM_NAME, &newname))
> > - newname = xstrdup("");
> > + free(cc->name);
>
> cc->name is freed
>
> > + if (!xu_get_strprop(cc->win, ewmh[_NET_WM_NAME], &cc->name))
> > + if (!xu_get_strprop(cc->win, XA_WM_NAME, &cc->name))
> > + cc->name = xstrdup("");
>
> And only re-assigned on certain conditions
First it tries to get the name from _NET_WM_NAME Atom, then XA_WM_NAME
Atom, then lastly it gets set to "". xu_get_strprop() returns => 1 on
success and 0 when there is no value to assign. So cc->name should
always be assigned at this point.
> >
> > - TAILQ_FOREACH(wn, &cc->nameq, entry) {
> > - if (strcmp(wn->name, newname) == 0)
> > + TAILQ_FOREACH_SAFE(wn, &cc->nameq, entry, wmnxt) {
> > + if (strcmp(wn->name, cc->name) == 0) {
> > TAILQ_REMOVE(&cc->nameq, wn, entry);
> > + free(wn->name);
> > + free(wn);
> > + }
> > i++;
> > }
> > - cc->name = newname;
> > wn = xmalloc(sizeof(*wn));
> > - wn->name = xstrdup(newname);
> > + wn->name = xstrdup(cc->name);
>
> and then used unconditioanlly
>
> > TAILQ_INSERT_TAIL(&cc->nameq, wn, entry);
> >
> > /* Garbage collection. */
>
