BUG/MINOR: ssl/cli: fix "show ssl ca-file/crl-file" not to mix cli+ssl contexts

The "show ca-file" and "show crl-file" commands mix some generic pointers
from the "ctx.cli" struct and context-specific ones from "ctx.ssl" while
both are in a union. It's fortunate that the p0 pointer in use is located
immediately before the first one used (it overlaps with next_ckchi_link,
and old_cafile_entry is safe). But should these fields be reordered or
slightly updated this will break.

Comments were added on top of the affected functions to indicate what they
use.

This needs to be backported to 2.5.

diff --git a/include/haproxy/applet-t.h b/include/haproxy/applet-t.h
index 6bcabec667642735d9bbf4f0903c3cb1dbc2e4fa..715cb69ce7be5a492015d6def3675e1ce48769ba 100644 (file)
--- a/include/haproxy/applet-t.h
+++ b/include/haproxy/applet-t.h
@@ -183,6 +183,7 @@ struct appctx {
                        struct ckch_inst_link *next_ckchi_link;
                        struct cafile_entry *old_cafile_entry;
                        struct cafile_entry *new_cafile_entry;
+                       struct cafile_entry *cur_cafile_entry;
 
                        struct cafile_entry *old_crlfile_entry;
                        struct cafile_entry *new_crlfile_entry;

diff --git a/src/ssl_ckch.c b/src/ssl_ckch.c
index aff35ebcd79fe736e852954b06f137c941c80f27..30d81686df5f4c28f7e15f66013dfcb12bbceb2d 100644 (file)
--- a/src/ssl_ckch.c
+++ b/src/ssl_ckch.c
@@ -2908,7 +2908,9 @@ error:
        return cli_dynerr(appctx, err);
 }
 
-/* release function of the `commit ssl ca-file' command, free things and unlock the spinlock */
+/* release function of the `commit ssl ca-file' command, free things and unlock the spinlock.
+ * It uses ctx.ssl.new_cafile_entry.
+ */
 static void cli_release_commit_cafile(struct appctx *appctx)
 {
        if (appctx->st2 != SETCERT_ST_FIN) {
@@ -2922,11 +2924,14 @@ static void cli_release_commit_cafile(struct appctx *appctx)
 }
 
 
-/* IO handler of details "show ssl ca-file <filename[:index]>" */
+/* IO handler of details "show ssl ca-file <filename[:index]>".
+ * It uses ctx.ssl.cur_cafile_entry, ctx.cli.i0, ctx.cli.i1, and
+ * the global cafile_transaction.new_cafile_entry in read-only.
+ */
 static int cli_io_handler_show_cafile_detail(struct appctx *appctx)
 {
        struct conn_stream *cs = appctx->owner;
-       struct cafile_entry *cafile_entry = appctx->ctx.cli.p0;
+       struct cafile_entry *cafile_entry = appctx->ctx.ssl.cur_cafile_entry;
        struct buffer *out = alloc_trash_chunk();
        int i = 0;
        X509 *cert;
@@ -3046,7 +3051,7 @@ static int cli_parse_show_cafile(char **args, char *payload, struct appctx *appc
                                goto error;
                }
 
-               appctx->ctx.cli.p0 = cafile_entry;
+               appctx->ctx.ssl.cur_cafile_entry = cafile_entry;
                /* use the IO handler that shows details */
                appctx->io_handler = cli_io_handler_show_cafile_detail;
        }
@@ -3083,7 +3088,10 @@ static int get_certificate_count(struct cafile_entry *cafile_entry)
 }
 
 /* IO handler of "show ssl ca-file". The command taking a specific CA file name
- * is managed in cli_io_handler_show_cafile_detail. */
+ * is managed in cli_io_handler_show_cafile_detail.
+ * It uses ctx.ssl.cur_cafile_entry, and the global
+ * cafile_transaction.new_cafile_entry in read-only.
+ */
 static int cli_io_handler_show_cafile(struct appctx *appctx)
 {
        struct buffer *trash = alloc_trash_chunk();
@@ -3104,12 +3112,12 @@ static int cli_io_handler_show_cafile(struct appctx *appctx)
        }
 
        /* First time in this io_handler. */
-       if (!appctx->ctx.cli.p0) {
+       if (!appctx->ctx.ssl.cur_cafile_entry) {
                chunk_appendf(trash, "# filename\n");
                node = ebmb_first(&cafile_tree);
        } else {
                /* We yielded during a previous call. */
-               node = &((struct cafile_entry*)appctx->ctx.cli.p0)->node;
+               node = &appctx->ctx.ssl.cur_cafile_entry->node;
        }
 
        while (node) {
@@ -3127,13 +3135,13 @@ static int cli_io_handler_show_cafile(struct appctx *appctx)
                }
        }
 
-       appctx->ctx.cli.p0 = NULL;
+       appctx->ctx.ssl.cur_cafile_entry = NULL;
        free_trash_chunk(trash);
        return 1;
 yield:
 
        free_trash_chunk(trash);
-       appctx->ctx.cli.p0 = cafile_entry;
+       appctx->ctx.ssl.cur_cafile_entry = cafile_entry;
        return 0; /* should come back */
 }
 
@@ -3591,11 +3599,14 @@ end:
        return 0;
 }
 
-/* IO handler of details "show ssl crl-file <filename[:index]>" */
+/* IO handler of details "show ssl crl-file <filename[:index]>".
+ * It uses ctx.ssl.cur_cafile_entry, ctx.cli.p1, ctx.cli.i1, and
+ * the global crlfile_transaction.new_cafile_entry in read-only.
+ */
 static int cli_io_handler_show_crlfile_detail(struct appctx *appctx)
 {
        struct conn_stream *cs = appctx->owner;
-       struct cafile_entry *cafile_entry = appctx->ctx.cli.p0;
+       struct cafile_entry *cafile_entry = appctx->ctx.ssl.cur_cafile_entry;
        struct buffer *out = alloc_trash_chunk();
        int i;
        X509_CRL *crl;
@@ -3654,7 +3665,10 @@ yield:
        return 0; /* should come back */
 }
 
-/* parsing function for 'show ssl crl-file [crlfile[:index]]' */
+/* parsing function for 'show ssl crl-file [crlfile[:index]]'.
+ * It sets ctx.ssl.cur_cafile_entry, ctx.cli.p1, and the global
+ * cafile_transaction.new_crlfile_entry under the ckch_lock.
+ */
 static int cli_parse_show_crlfile(char **args, char *payload, struct appctx *appctx, void *private)
 {
        struct cafile_entry *cafile_entry;
@@ -3703,7 +3717,7 @@ static int cli_parse_show_crlfile(char **args, char *payload, struct appctx *app
                                goto error;
                }
 
-               appctx->ctx.cli.p0 = cafile_entry;
+               appctx->ctx.ssl.cur_cafile_entry = cafile_entry;
                appctx->ctx.cli.p1 = (void*)index;
                /* use the IO handler that shows details */
                appctx->io_handler = cli_io_handler_show_crlfile_detail;
@@ -3738,12 +3752,12 @@ static int cli_io_handler_show_crlfile(struct appctx *appctx)
        }
 
        /* First time in this io_handler. */
-       if (!appctx->ctx.cli.p0) {
+       if (!appctx->ctx.ssl.cur_cafile_entry) {
                chunk_appendf(trash, "# filename\n");
                node = ebmb_first(&cafile_tree);
        } else {
                /* We yielded during a previous call. */
-               node = &((struct cafile_entry*)appctx->ctx.cli.p0)->node;
+               node = &appctx->ctx.ssl.cur_cafile_entry->node;
        }
 
        while (node) {
@@ -3759,13 +3773,13 @@ static int cli_io_handler_show_crlfile(struct appctx *appctx)
                }
        }
 
-       appctx->ctx.cli.p0 = NULL;
+       appctx->ctx.ssl.cur_cafile_entry = NULL;
        free_trash_chunk(trash);
        return 1;
 yield:
 
        free_trash_chunk(trash);
-       appctx->ctx.cli.p0 = cafile_entry;
+       appctx->ctx.ssl.cur_cafile_entry = cafile_entry;
        return 0; /* should come back */
 }