From: Theo Buehler <tb@theobuehler.org>
Subject: Re: msgbuf_free incomplete NULL check
To: Henry Ford <henryfordkjv@gmail.com>, tech@openbsd.org
Date: Fri, 22 Nov 2024 08:11:09 +0100

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);
 }