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 08:17:27 +0100

On Fri, Nov 22, 2024 at 08:11:09AM +0100, Theo Buehler wrote:
> On Fri, Nov 22, 2024 at 07:58:58AM +0100, Claudio Jeker wrote:
> > 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);
> > 	...
> 
> I was thinking the same.
> 
> Index: imsg-buffer.c
> ===================================================================
> RCS file: /cvs/src/lib/libutil/imsg-buffer.c,v
> diff -u -p -r1.29 imsg-buffer.c
> --- imsg-buffer.c	22 Nov 2024 02:11:09 -0000	1.29
> +++ imsg-buffer.c	22 Nov 2024 07:08:17 -0000
> @@ -605,10 +605,10 @@ msgbuf_new_reader(size_t hdrsz, ssize_t 
>  void
>  msgbuf_free(struct msgbuf *msgbuf)
>  {
> -	if (msgbuf != NULL) {
> -		msgbuf_clear(msgbuf);
> -		free(msgbuf->rbuf);
> -	}
> +	if (msgbuf == NULL)
> +		return;
> +	msgbuf_clear(msgbuf);
> +	free(msgbuf->rbuf);
>  	free(msgbuf);
>  }
>  
> 

I have the same diff. OK claudio@

-- 
:wq Claudio