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 11:56:29 +0200

On Tue, Apr 07, 2026 at 09:37:07AM +0000, Jan Schreiber wrote:
> Hi,
> 
> json_getstr uses strdup(3) internally and returns a pointer.
> The allocated memory currently leaks in a couple of places.
> 
> Jan
> 
> diff --git usr.sbin/acme-client/json.c usr.sbin/acme-client/json.c
> index 40812698a05..bc7236745df 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);

This bit looks right.

> +	return rc;
>  }
>  
>  /*
> @@ -481,6 +485,7 @@ json_parse_order(struct jsmnn *n, struct order *order)
>  	}
>  	return 1;
>  err:
> +	free(finalize);

I don't think the extra strdup buys us anything. I think it would be
better to remove finalize and to assign to order->finalize directly:

	if ((order->finalize = json_getstr(n, "finalize")) == NULL) {

>  	json_free_order(order);
>  	return 0;
>  }
> @@ -491,9 +496,13 @@ 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) {

Same here. Remove certificate and simplify. And you could add an empty
line before the if while you're here

> -		if ((order->certificate = strdup(certificate)) == NULL)
> +		if ((order->certificate = strdup(certificate)) == NULL) {
> +			free(certificate);
>  			return 0;
> +		}
>  	}
> +
> +	free(certificate);
>  	return 1;
>  }
>  
> @@ -508,7 +517,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..c44ea40839b 100644
> --- usr.sbin/acme-client/netproc.c
> +++ usr.sbin/acme-client/netproc.c
> @@ -427,6 +427,7 @@ donewacc(struct conn *c, const struct capaths *p, const char *contact)
>  			if (detail != NULL && stravis(&error, detail, VIS_SAFE)
>  			    != -1) {
>  				warnx("%s", error);
> +				free(detail);

This still leaks in the unlikely event that stravis fails. This free
should be done outside the if block.

>  				free(error);
>  			}
>  		}
>