From: Jan Schreiber <jes@posteo.de>
Subject: Re: acme-client: close json memory leaks
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Tue, 07 Apr 2026 12:23:45 +0000

On Tue, 7 Apr 2026 14:10:56 +0200
Theo Buehler <tb@theobuehler.org> wrote:

> On Tue, Apr 07, 2026 at 11:57:16AM +0000, Jan Schreiber wrote:
> > On Tue, 7 Apr 2026 13:15:55 +0200
> > Theo Buehler <tb@theobuehler.org> wrote:
> >   
> > > On Tue, Apr 07, 2026 at 11:04:00AM +0000, Jan Schreiber wrote:  
> > > > On Tue, 7 Apr 2026 12:45:49 +0200
> > > > Theo Buehler <tb@theobuehler.org> wrote:
> > > >     
> > > > > Thanks. Yes, that's basically what I had in mind. Two more nits    
> > > > 
> > > > Fixed now, thank you for the review!    
> > >   
> > > > @@ -488,12 +489,11 @@ err:
> > > >  int
> > > >  json_parse_upd_order(struct jsmnn *n, struct order *order)
> > > >  {
> > > > -	char	*certificate;
> > > >  	order->status = json_parse_order_status(n);
> > > > -	if ((certificate = json_getstr(n, "certificate")) != NULL) {
> > > > -		if ((order->certificate = strdup(certificate)) == NULL)
> > > > -			return 0;
> > > > -	}
> > > > +
> > > > +	if ((order->certificate = json_getstr(n, "certificate")) != NULL)
> > > > +		return 0;    
> > > 
> > > The != NULL check is wrong, if anything it should be == NULL. But
> > > previously an error check would only happen if there actually was a
> > > "certificate" present (and strdup didn't fail). I think this should be
> > > 
> > > 	order->status = json_parse_order_status(n);
> > > 	order->certificate = json_getstr(n, "certificate");
> > >   
> > 
> > You mean removing the NULL check entirely?  
> 
> Yes. The point is json_getstr(n, "certificate") == NULL was not an error
> previously. It would still return 1, and some pebble tests rely on that.

It feels wrong not to check for a failed json_getstr to me. I'll have a look at
the regression tests later. (Something went haywire and I can't run them right now.)
> 
> > That would also involve removing
> > the return code check in netproc.c on line 543 and not handling the error.  
> 
> I would not do that. Keeping the return 1 as I suggested seems fine to
> me.
> 

Patch with your suggestions below. Thank you for taking the time!

diff --git usr.sbin/acme-client/json.c usr.sbin/acme-client/json.c
index 40812698a05..014e3555f6f 100644
--- usr.sbin/acme-client/json.c
+++ usr.sbin/acme-client/json.c
@@ -412,7 +412,8 @@ json_parse_challenge(struct jsmnn *n, struct chng *p)
 static enum orderstatus
 json_parse_order_status(struct jsmnn *n)
 {
-	char	*status;
+	char			*status;
+	enum orderstatus	 rc;
 
 	if (n == NULL)
 		return ORDER_INVALID;
@@ -421,17 +422,20 @@ json_parse_order_status(struct jsmnn *n)
 		return ORDER_INVALID;
 
 	if (strcmp(status, "pending") == 0)
-		return ORDER_PENDING;
+		rc = ORDER_PENDING;
 	else if (strcmp(status, "ready") == 0)
-		return ORDER_READY;
+		rc = ORDER_READY;
 	else if (strcmp(status, "processing") == 0)
-		return ORDER_PROCESSING;
+		rc = ORDER_PROCESSING;
 	else if (strcmp(status, "valid") == 0)
-		return ORDER_VALID;
+		rc = ORDER_VALID;
 	else if (strcmp(status, "invalid") == 0)
-		return ORDER_INVALID;
+		rc = ORDER_INVALID;
 	else
-		return ORDER_INVALID;
+		rc = ORDER_INVALID;
+
+	free(status);
+	return rc;
 }
 
 /*
@@ -443,21 +447,18 @@ json_parse_order(struct jsmnn *n, struct order *order)
 {
 	struct jsmnn	*array;
 	size_t		 i;
-	char		*finalize, *str;
+	char		*str;
 
 	order->status = json_parse_order_status(n);
 
 	if (n == NULL)
 		return 0;
 
-	if ((finalize = json_getstr(n, "finalize")) == NULL) {
+	if ((order->finalize = json_getstr(n, "finalize")) == NULL) {
 		warnx("no finalize field in order response");
 		return 0;
 	}
 
-	if ((order->finalize = strdup(finalize)) == NULL)
-		goto err;
-
 	if ((array = json_getarray(n, "authorizations")) == NULL)
 		goto err;
 
@@ -488,12 +489,9 @@ err:
 int
 json_parse_upd_order(struct jsmnn *n, struct order *order)
 {
-	char	*certificate;
 	order->status = json_parse_order_status(n);
-	if ((certificate = json_getstr(n, "certificate")) != NULL) {
-		if ((order->certificate = strdup(certificate)) == NULL)
-			return 0;
-	}
+	order->certificate = json_getstr(n, "certificate");
+
 	return 1;
 }
 
@@ -508,7 +506,6 @@ json_free_order(struct order *order)
 		free(order->auths[i]);
 	free(order->auths);
 
-	order->finalize = NULL;
 	order->auths = NULL;
 	order->authsz = 0;
 }
diff --git usr.sbin/acme-client/netproc.c usr.sbin/acme-client/netproc.c
index 70a069bc095..46f413e110e 100644
--- usr.sbin/acme-client/netproc.c
+++ usr.sbin/acme-client/netproc.c
@@ -429,6 +429,7 @@ donewacc(struct conn *c, const struct capaths *p, const char *contact)
 				warnx("%s", error);
 				free(error);
 			}
+			free(detail);
 		}
 	} else if (lc != 200 && lc != 201)
 		warnx("%s: bad HTTP: %ld", p->newaccount, lc);