BUG/MEDIUM: ssl_ckch: Rework 'commit ssl cert' to handle full buffer cases

When changes on a certificate are commited, a trash buffer is used to create
the response. Once done, the message is copied in the response buffer.
However, if the buffer is full, there is no way to retry and the message is
lost. The same issue may happen with the error message. It is a design issue
of cli_io_handler_commit_cert() function.

To fix it, the function was reworked. First, the error message is now part
of the service context. This way, if we cannot push the error message in the
reponse buffer, we may retry later. To do so, a dedicated state was created
(CERT_ST_ERROR). Then, the success message is also handled in a dedicated
state (CERT_ST_SUCCESS). This way we are able to retry to push it if
necessary. Finally, the dot displayed for each updated CKCH instance is now
immediatly pushed in the response buffer, and before the update. This way,
we are able to retry too if necessary.

This patch should fix the issue #1725. It must be backported as far as
2.2. But massive refactoring was performed in 2.6. So, for the 2.5 and
below, the patch must be adapted.

diff --git a/src/ssl_ckch.c b/src/ssl_ckch.c
index fc6d114405f72f3907be2c008b5904324016ad45..e2113f2a0e8729ce70c1460b7acb8731b40276d8 100644 (file)
--- a/src/ssl_ckch.c
+++ b/src/ssl_ckch.c
@@ -89,11 +89,14 @@ struct commit_cert_ctx {
        struct ckch_store *old_ckchs;
        struct ckch_store *new_ckchs;
        struct ckch_inst *next_ckchi;
+       char *err;
        enum {
                CERT_ST_INIT = 0,
                CERT_ST_GEN,
                CERT_ST_INSERT,
+               CERT_ST_SUCCESS,
                CERT_ST_FIN,
+               CERT_ST_ERROR,
        } state;
 };
 
@@ -1868,17 +1871,12 @@ error:
 static void cli_release_commit_cert(struct appctx *appctx)
 {
        struct commit_cert_ctx *ctx = appctx->svcctx;
-       struct ckch_store *new_ckchs;
 
        HA_SPIN_UNLOCK(CKCH_LOCK, &ckch_lock);
-
-       if (ctx->state != CERT_ST_FIN) {
-               /* free every new sni_ctx and the new store, which are not in the trees so no spinlock there */
-               new_ckchs = ctx->new_ckchs;
-
-               /* if the allocation failed, we need to free everything from the temporary list */
-               ckch_store_free(new_ckchs);
-       }
+       /* free every new sni_ctx and the new store, which are not in the trees so no spinlock there */
+       if (ctx->new_ckchs)
+               ckch_store_free(ctx->new_ckchs);
+       ha_free(&ctx->err);
 }
 
 
@@ -2043,23 +2041,18 @@ static int cli_io_handler_commit_cert(struct appctx *appctx)
        struct commit_cert_ctx *ctx = appctx->svcctx;
        struct stconn *sc = appctx_sc(appctx);
        int y = 0;
-       char *err = NULL;
        struct ckch_store *old_ckchs, *new_ckchs = NULL;
        struct ckch_inst *ckchi;
-       struct buffer *trash = alloc_trash_chunk();
-
-       if (trash == NULL)
-               goto error;
 
        if (unlikely(sc_ic(sc)->flags & (CF_WRITE_ERROR|CF_SHUTW)))
-               goto error;
+               goto end;
 
        while (1) {
                switch (ctx->state) {
                        case CERT_ST_INIT:
                                /* This state just print the update message */
-                               chunk_printf(trash, "Committing %s", ckchs_transaction.path);
-                               if (applet_putchk(appctx, trash) == -1)
+                               chunk_printf(&trash, "Committing %s", ckchs_transaction.path);
+                               if (applet_putchk(appctx, &trash) == -1)
                                        goto yield;
 
                                ctx->state = CERT_ST_GEN;
@@ -2076,9 +2069,6 @@ static int cli_io_handler_commit_cert(struct appctx *appctx)
                                old_ckchs = ctx->old_ckchs;
                                new_ckchs = ctx->new_ckchs;
 
-                               if (!new_ckchs)
-                                       continue;
-
                                /* get the next ckchi to regenerate */
                                ckchi = ctx->next_ckchi;
                                /* we didn't start yet, set it to the first elem */
@@ -2089,18 +2079,25 @@ static int cli_io_handler_commit_cert(struct appctx *appctx)
                                list_for_each_entry_from(ckchi, &old_ckchs->ckch_inst, by_ckchs) {
                                        struct ckch_inst *new_inst;
 
+                                       /* save the next ckchi to compute in case of yield */
+                                       ctx->next_ckchi = ckchi;
+
                                        /* it takes a lot of CPU to creates SSL_CTXs, so we yield every 10 CKCH instances */
                                        if (y >= 10) {
-                                               /* save the next ckchi to compute */
-                                               ctx->next_ckchi = ckchi;
+                                               applet_have_more_data(appctx); /* let's come back later */
                                                goto yield;
                                        }
 
-                                       if (ckch_inst_rebuild(new_ckchs, ckchi, &new_inst, &err))
+                                       /* display one dot per new instance */
+                                       if (applet_putstr(appctx, ".") == -1)
+                                               goto yield;
+
+                                       ctx->err = NULL;
+                                       if (ckch_inst_rebuild(new_ckchs, ckchi, &new_inst, &ctx->err)) {
+                                               ctx->state = CERT_ST_ERROR;
                                                goto error;
+                                       }
 
-                                       /* display one dot per new instance */
-                                       chunk_appendf(trash, ".");
                                        /* link the new ckch_inst to the duplicate */
                                        LIST_APPEND(&new_ckchs->ckch_inst, &new_inst->by_ckchs);
                                        y++;
@@ -2113,12 +2110,14 @@ static int cli_io_handler_commit_cert(struct appctx *appctx)
                                old_ckchs = ctx->old_ckchs;
                                new_ckchs = ctx->new_ckchs;
 
-                               if (!new_ckchs)
-                                       continue;
-
                                /* insert everything and remove the previous objects */
                                ckch_store_replace(old_ckchs, new_ckchs);
-
+                               ctx->new_ckchs = ctx->old_ckchs = NULL;
+                               ctx->state = CERT_ST_SUCCESS;
+                               /* fallthrough */
+                       case CERT_ST_SUCCESS:
+                               if (applet_putstr(appctx, "\nSuccess!\n") == -1)
+                                       goto yield;
                                ctx->state = CERT_ST_FIN;
                                /* fallthrough */
                        case CERT_ST_FIN:
@@ -2127,33 +2126,22 @@ static int cli_io_handler_commit_cert(struct appctx *appctx)
                                ckchs_transaction.old_ckchs = NULL;
                                ckchs_transaction.path = NULL;
                                goto end;
+
+                       case CERT_ST_ERROR:
+                         error:
+                               chunk_printf(&trash, "\n%sFailed!\n", ctx->err);
+                               if (applet_putchk(appctx, &trash) == -1)
+                                       goto yield;
+                               ctx->state = CERT_ST_FIN;
+                               break;
                }
        }
 end:
-
-       chunk_appendf(trash, "\n");
-       chunk_appendf(trash, "Success!\n");
-       applet_putchk(appctx, trash);
-       free_trash_chunk(trash);
        /* success: call the release function and don't come back */
        return 1;
+
 yield:
-       /* store the state */
-       applet_putchk(appctx, trash);
-       free_trash_chunk(trash);
-       applet_have_more_data(appctx); /* let's come back later */
        return 0; /* should come back */
-
-error:
-       /* spin unlock and free are done in the release  function */
-       if (trash) {
-               chunk_appendf(trash, "\n%sFailed!\n", err);
-               applet_putchk(appctx, trash);
-               free_trash_chunk(trash);
-       }
-       free(err);
-       /* error: call the release function and don't come back */
-       return 1;
 }
 
 /*