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

When a CA or CRL entry is replaced (via 'set ssl ca-file' or 'set ssl
crl-file' commands), 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 filename 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.5.

diff --git a/src/ssl_ckch.c b/src/ssl_ckch.c
index fb85da8dddb635b34ccc4c81030c2d5bba56bf4d..fc6d114405f72f3907be2c008b5904324016ad45 100644 (file)
--- a/src/ssl_ckch.c
+++ b/src/ssl_ckch.c
@@ -107,14 +107,12 @@ struct set_cert_ctx {
 struct set_cafile_ctx {
        struct cafile_entry *old_cafile_entry;
        struct cafile_entry *new_cafile_entry;
-       char *path;
 };
 
 /* CLI context used by "set crl-file" */
 struct set_crlfile_ctx {
        struct cafile_entry *old_crlfile_entry;
        struct cafile_entry *new_crlfile_entry;
-       char *path;
 };
 
 /* CLI context used by "commit cafile" and "commit crlfile" */
@@ -2625,21 +2623,11 @@ static int cli_parse_set_cafile(char **args, char *payload, struct appctx *appct
                goto end;
        }
 
-       if (!ctx->path) {
-               /* this is a new transaction, set the path of the transaction */
-               ctx->path = strdup(ctx->old_cafile_entry->path);
-               if (!ctx->path) {
-                       memprintf(&err, "%sCan't allocate memory\n", err ? err : "");
-                       errcode |= ERR_ALERT | ERR_FATAL;
-                       goto end;
-               }
-       }
-
        if (ctx->new_cafile_entry)
                ssl_store_delete_cafile_entry(ctx->new_cafile_entry);
 
        /* Create a new cafile_entry without adding it to the cafile tree. */
-       ctx->new_cafile_entry = ssl_store_create_cafile_entry(ctx->path, NULL, CAFILE_CERT);
+       ctx->new_cafile_entry = ssl_store_create_cafile_entry(ctx->old_cafile_entry->path, NULL, CAFILE_CERT);
        if (!ctx->new_cafile_entry) {
                memprintf(&err, "%sCannot allocate memory!\n",
                          err ? err : "");
@@ -2659,7 +2647,7 @@ static int cli_parse_set_cafile(char **args, char *payload, struct appctx *appct
        /* if there wasn't a transaction, update the old CA */
        if (!cafile_transaction.old_cafile_entry) {
                cafile_transaction.old_cafile_entry = ctx->old_cafile_entry;
-               cafile_transaction.path = ctx->path;
+               cafile_transaction.path = ctx->old_cafile_entry->path;
                err = memprintf(&err, "transaction created for CA %s!\n", cafile_transaction.path);
        } else {
                err = memprintf(&err, "transaction updated for CA %s!\n", cafile_transaction.path);
@@ -2679,7 +2667,6 @@ end:
                ssl_store_delete_cafile_entry(ctx->new_cafile_entry);
                ctx->new_cafile_entry = NULL;
                ctx->old_cafile_entry = 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 {
@@ -2898,14 +2885,14 @@ static int cli_io_handler_commit_cafile_crlfile(struct appctx *appctx)
                                /* we achieved the transaction, we can set everything to NULL */
                                switch (ctx->cafile_type) {
                                case CAFILE_CERT:
-                                       ha_free(&cafile_transaction.path);
                                        cafile_transaction.old_cafile_entry = NULL;
                                        cafile_transaction.new_cafile_entry = NULL;
+                                       cafile_transaction.path = NULL;
                                        break;
                                case CAFILE_CRL:
-                                       ha_free(&crlfile_transaction.path);
                                        crlfile_transaction.old_crlfile_entry = NULL;
                                        crlfile_transaction.new_crlfile_entry = NULL;
+                                       crlfile_transaction.path = NULL;
                                        break;
                                }
                                goto end;
@@ -2969,7 +2956,7 @@ static int cli_parse_abort_cafile(char **args, char *payload, struct appctx *app
        ssl_store_delete_cafile_entry(cafile_transaction.new_cafile_entry);
        cafile_transaction.new_cafile_entry = NULL;
        cafile_transaction.old_cafile_entry = NULL;
-       ha_free(&cafile_transaction.path);
+       cafile_transaction.path = NULL;
 
        HA_SPIN_UNLOCK(CKCH_LOCK, &ckch_lock);
 
@@ -3372,21 +3359,11 @@ static int cli_parse_set_crlfile(char **args, char *payload, struct appctx *appc
                goto end;
        }
 
-       if (!ctx->path) {
-               /* this is a new transaction, set the path of the transaction */
-               ctx->path = strdup(ctx->old_crlfile_entry->path);
-               if (!ctx->path) {
-                       memprintf(&err, "%sCan't allocate memory\n", err ? err : "");
-                       errcode |= ERR_ALERT | ERR_FATAL;
-                       goto end;
-               }
-       }
-
        if (ctx->new_crlfile_entry)
                ssl_store_delete_cafile_entry(ctx->new_crlfile_entry);
 
        /* Create a new cafile_entry without adding it to the cafile tree. */
-       ctx->new_crlfile_entry = ssl_store_create_cafile_entry(ctx->path, NULL, CAFILE_CRL);
+       ctx->new_crlfile_entry = ssl_store_create_cafile_entry(ctx->old_crlfile_entry->path, NULL, CAFILE_CRL);
        if (!ctx->new_crlfile_entry) {
                memprintf(&err, "%sCannot allocate memory!\n", err ? err : "");
                errcode |= ERR_ALERT | ERR_FATAL;
@@ -3405,7 +3382,7 @@ static int cli_parse_set_crlfile(char **args, char *payload, struct appctx *appc
        /* if there wasn't a transaction, update the old CRL */
        if (!crlfile_transaction.old_crlfile_entry) {
                crlfile_transaction.old_crlfile_entry = ctx->old_crlfile_entry;
-               crlfile_transaction.path = ctx->path;
+               crlfile_transaction.path = ctx->old_crlfile_entry->path;
                err = memprintf(&err, "transaction created for CRL %s!\n", crlfile_transaction.path);
        } else {
                err = memprintf(&err, "transaction updated for CRL %s!\n", crlfile_transaction.path);
@@ -3425,7 +3402,6 @@ end:
                ssl_store_delete_cafile_entry(ctx->new_crlfile_entry);
                ctx->new_crlfile_entry = NULL;
                ctx->old_crlfile_entry = 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 {
@@ -3581,7 +3557,7 @@ static int cli_parse_abort_crlfile(char **args, char *payload, struct appctx *ap
        ssl_store_delete_cafile_entry(crlfile_transaction.new_crlfile_entry);
        crlfile_transaction.new_crlfile_entry = NULL;
        crlfile_transaction.old_crlfile_entry = NULL;
-       ha_free(&crlfile_transaction.path);
+       crlfile_transaction.path = NULL;
 
        HA_SPIN_UNLOCK(CKCH_LOCK, &ckch_lock);