On Thu, Feb 10, 2022 at 11:45:06AM +0100, Theo Buehler wrote: > > > Index: rrdp.c > > > =================================================================== > > > RCS file: /cvs/src/usr.sbin/rpki-client/rrdp.c,v > > > retrieving revision 1.21 > > > diff -u -p -r1.21 rrdp.c > > > --- rrdp.c 23 Jan 2022 12:09:24 -0000 1.21 > > > +++ rrdp.c 10 Feb 2022 07:41:54 -0000 > > > @@ -429,6 +429,7 @@ rrdp_input_handler(int fd) > > > errx(1, "%s: bad internal state", s->local); > > > > > > s->res = res; > > > + free(s->last_mod); > > > s->last_mod = last_mod; > > > s->state |= RRDP_STATE_HTTP_DONE; > > > rrdp_finished(s); > > > > > > > This one needs more thought. rrdp_finished() -> notification_done() moves > > the last_mod into nxml->current->last_mod and so you can end up with > > either a double free or use after free. > > I saw that and thought ownership was transferred to > s->nxml->current->last_mod since s->last_mod is nulled out right after > > s->task = notification_done(s->nxml, s->last_mod); > s->last_mod = NULL; > > I can't see where that's freed though.
Indeed. I missed that somehow. So your diff is indeed OK. The free happens in rrdp_free() which is OK. There is only one call to notification_done() so that is fine. So your diff is indeed OK and should go in. OK claudio@ > > That code needs a proper refactor. > > That may well be... > > > I think it would be best to remove last_mod from the rrdp state and just > > pass it as char * to rrdp_finished. Still need to look when and how to > > free the value reliably. Actually that does not really work because the RRDP_HTTP_FIN could arrive before all data has been handled by rrdp_data_handler(). That is why it is in the state. As said, OK on the original diff. -- :wq Claudio
