From: Jan Schreiber Subject: Re: acme-client: close json memory leaks To: Theo Buehler Cc: tech@openbsd.org Date: Tue, 07 Apr 2026 10:21:27 +0000 On Tue, 7 Apr 2026 11:56:29 +0200 Theo Buehler 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)