Download raw body.
acme-client: close json memory leaks
On Tue, 7 Apr 2026 11:56:29 +0200
Theo Buehler <tb@theobuehler.org> wrote:
> 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);
> > }
> > }
> >
>
Thank you Theo, new diff below.
diff --git usr.sbin/acme-client/json.c usr.sbin/acme-client/json.c
index 40812698a05..b4cf914e0e6 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,12 @@ 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;
}
+
return 1;
}
@@ -508,7 +509,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..e200b17196b 100644
--- usr.sbin/acme-client/netproc.c
+++ usr.sbin/acme-client/netproc.c
@@ -412,7 +412,7 @@ donewacc(struct conn *c, const struct capaths *p, const char *contact)
{
struct jsmnn *j = NULL;
int rc = 0;
- char *req, *detail, *error = NULL, *accturi = NULL;
+ char *req, *detail = NULL, *error = NULL, *accturi = NULL;
long lc;
if ((req = json_fmt_newacc(contact)) == NULL)
@@ -430,6 +430,7 @@ donewacc(struct conn *c, const struct capaths *p, const char *contact)
free(error);
}
}
+ free(detail);
} else if (lc != 200 && lc != 201)
warnx("%s: bad HTTP: %ld", p->newaccount, lc);
else if (c->buf.buf == NULL || c->buf.sz == 0)
acme-client: close json memory leaks