On Sun, Mar 15, 2020 at 11:14:06AM -0400, Okan Demirmen wrote:
> 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.
Right, thanks for the explanation.
-Otto
>
> > >
> > > - 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. */
> >
