From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: msgbuf_free incomplete NULL check
To: Theo Buehler <tb@theobuehler.org>
Cc: Henry Ford <henryfordkjv@gmail.com>, tech@openbsd.org
Date: Fri, 22 Nov 2024 07:58:58 +0100

On Fri, Nov 22, 2024 at 03:17:01AM +0100, Theo Buehler wrote:
> On Thu, Nov 21, 2024 at 08:58:19PM -0500, Henry Ford wrote:
> > msgbuf_free checks if msgbuf is NULL before calling msgbuf_clear on
> > it, but does not perform the same check before freeing its rbuf field.
> > After upgrading to the latest snapshot this causes my ntpd to crash on
> > startup. The following diff guards the call to free with the same check
> > used for msgbuf_clear. After applying this diff ntpd no longer crashes.
> 
> Thanks for that. I committed this as is although it's still a bit weird.
> I'll let claudio bring it into his preferred form (if it should be
> different from this) when he gets up.

Thanks for the report and taking care of this.
The free(msgbuf->rbuf) call was indeed a last minute addition.
Maybe we should change this to:

	if (msgbuf == NULL)
		return;
	msgbuf_clear(msgbuf);
	...
 
> > 
> > diff /usr/src
> > commit - e08605c7f2d4f3a5540bdbbdf70eaa19abe1f819
> > path + /usr/src
> > blob - c43da77f8af85dd91437e0576db867ab7c4defa1
> > file + lib/libutil/imsg-buffer.c
> > --- lib/libutil/imsg-buffer.c
> > +++ lib/libutil/imsg-buffer.c
> > @@ -605,9 +605,10 @@ msgbuf_new_reader(size_t hdrsz, ssize_t (*readhdr)(str
> >  void
> >  msgbuf_free(struct msgbuf *msgbuf)
> >  {
> > -	if (msgbuf != NULL)
> > +	if (msgbuf != NULL) {
> >  		msgbuf_clear(msgbuf);
> > -	free(msgbuf->rbuf);
> > +		free(msgbuf->rbuf);
> > +	}
> >  	free(msgbuf);
> >  }
> >  
> > 
> 

-- 
:wq Claudio