From: Theo Buehler <tb@theobuehler.org>
Subject: Re: acme-client: close json memory leaks
To: Jan Schreiber <jes@posteo.de>
Cc: tech@openbsd.org
Date: Tue, 7 Apr 2026 14:36:31 +0200

On Tue, Apr 07, 2026 at 12:23:45PM +0000, Jan Schreiber wrote:
> 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

Maybe. Maybe it's also the warn("strdup") in json_getstr() that is
wrong. In any case, a diff fixing memleaks should not change behavior.

Notice that a missing "certificate" is expected towards the end of
netproc().

> 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!

This version is ok tb if anyone wants to commit.