From: Christopher Faulet <cfaulet@haproxy.com>
Date: Tue, 31 May 2022 16:07:59 +0000 (+0200)
Subject: BUG/MINOR: ssl_ckch: Don't duplicate path when replacing a cert entry
X-Git-Tag: v2.7-dev1~127
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=e2ef4dd3c579869d223534f3108a405774b351aa;p=thirdparty%2Fhaproxy.git

BUG/MINOR: ssl_ckch: Don't duplicate path when replacing a cert entry

When a certificate entry is replaced (via 'set ssl cert' command), the path
is duplicated and used to identify the ongoing transaction. However, if the
same command is repeated, the path is still duplicated but the transaction
is not changed and the duplicated path is not released. Thus there is a
memory leak.

By reviewing the code, it appears there is no reason to duplicate the
path. It is always the path of the old entry. So, a reference on it is now
used. This simplifies the code and this fixes the memory leak.

This patch must be backported as far as 2.2.
---

diff --git a/src/ssl_ckch.c b/src/ssl_ckch.c
index ad84eeacc1..fb85da8ddd 100644
--- a/src/ssl_ckch.c
+++ b/src/ssl_ckch.c
@@ -101,7 +101,6 @@ struct commit_cert_ctx {
 struct set_cert_ctx {
 	struct ckch_store *old_ckchs;
 	struct ckch_store *new_ckchs;
-	char *path;
 };
 
 /* CLI context used by "set ca-file" */
@@ -2126,9 +2125,9 @@ static int cli_io_handler_commit_cert(struct appctx *appctx)
 				/* fallthrough */
 			case CERT_ST_FIN:
 				/* we achieved the transaction, we can set everything to NULL */
-				ha_free(&ckchs_transaction.path);
 				ckchs_transaction.new_ckchs = NULL;
 				ckchs_transaction.old_ckchs = NULL;
+				ckchs_transaction.path = NULL;
 				goto end;
 		}
 	}
@@ -2326,16 +2325,6 @@ static int cli_parse_set_cert(char **args, char *payload, struct appctx *appctx,
 		goto end;
 	}
 
-	if (!ctx->path) {
-	/* this is a new transaction, set the path of the transaction */
-		ctx->path = strdup(ctx->old_ckchs->path);
-		if (!ctx->path) {
-			memprintf(&err, "%sCan't allocate memory\n", err ? err : "");
-			errcode |= ERR_ALERT | ERR_FATAL;
-			goto end;
-		}
-	}
-
 	old_ckchs = ctx->old_ckchs;
 
 	/* duplicate the ckch store */
@@ -2363,7 +2352,7 @@ static int cli_parse_set_cert(char **args, char *payload, struct appctx *appctx,
 	/* if there wasn't a transaction, update the old ckchs */
 	if (!ckchs_transaction.old_ckchs) {
 		ckchs_transaction.old_ckchs = ctx->old_ckchs;
-		ckchs_transaction.path = ctx->path;
+		ckchs_transaction.path = ctx->old_ckchs->path;
 		err = memprintf(&err, "Transaction created for certificate %s!\n", ckchs_transaction.path);
 	} else {
 		err = memprintf(&err, "Transaction updated for certificate %s!\n", ckchs_transaction.path);
@@ -2385,8 +2374,6 @@ end:
 		ckch_store_free(ctx->new_ckchs);
 		ctx->new_ckchs = NULL;
 		ctx->old_ckchs = NULL;
-		ha_free(&ctx->path);
-
 		HA_SPIN_UNLOCK(CKCH_LOCK, &ckch_lock);
 		return cli_dynerr(appctx, memprintf(&err, "%sCan't update %s!\n", err ? err : "", args[3]));
 	} else {
@@ -2426,7 +2413,7 @@ static int cli_parse_abort_cert(char **args, char *payload, struct appctx *appct
 	ckch_store_free(ckchs_transaction.new_ckchs);
 	ckchs_transaction.new_ckchs = NULL;
 	ckchs_transaction.old_ckchs = NULL;
-	ha_free(&ckchs_transaction.path);
+	ckchs_transaction.path = NULL;
 
 	HA_SPIN_UNLOCK(CKCH_LOCK, &ckch_lock);