BUG/MINOR: cli/payload: do not search for args inside payload

The CLI's payload parser is over-complicated and as such contains more
bugs than needed. One of them is that it uses strstr() to find the
ending tag, ignoring spaces before it, while the argument locator
creates a new arg on each space, without checking if the end of the
word appears past the previously found end. This results in "<<" being
considered as the start of a new argument if preceeded by more than
one space, and the payload being damaged with a \0 inserted at the
first space or tab.

Let's make an easily backportable fix for now. This fix makes sure that
the trailing zero from the first line is properly kept after '<<' and
that the end tag is looked for only as an isolated argument and nothing
else. This also gets rid of the unsuitable strstr() call and now makes
sure that strcspn() will not return elements that are found in the
payload.

For the long term the loop must be rewritten to get rid of those
unsuitable strcspn() and strstr() calls which work past each other, and
the cli_parse_request() function should be split into a tokenizer and
an executor that are used from the caller instead of letting the caller
play games with what it finds there.

This should be backported wherever CLI payload is supported, i.e. 2.0+.

diff --git a/src/cli.c b/src/cli.c
index 9d6354cc63afcb3494c5251acb7134680ac6416c..a3ee4c442e93ddd271b1b8e657c1fca262379a3c 100644 (file)
--- a/src/cli.c
+++ b/src/cli.c
@@ -677,22 +677,6 @@ static int cli_parse_request(struct appctx *appctx)
        p = appctx->chunk->area;
        end = p + appctx->chunk->data;
 
-       /*
-        * Get the payload start if there is one.
-        * For the sake of simplicity, the payload pattern is looked up
-        * everywhere from the start of the input but it can only be found
-        * at the end of the first line if APPCTX_CLI_ST1_PAYLOAD is set.
-        *
-        * The input string was zero terminated so it is safe to use
-        * the str*() functions throughout the parsing
-        */
-       if (appctx->st1 & APPCTX_CLI_ST1_PAYLOAD) {
-               payload = strstr(p, PAYLOAD_PATTERN);
-               end = payload;
-               /* skip the pattern */
-               payload += strlen(PAYLOAD_PATTERN);
-       }
-
        /*
         * Get pointers on words.
         * One extra slot is reserved to store a pointer on a null byte.
@@ -705,6 +689,15 @@ static int cli_parse_request(struct appctx *appctx)
                if (!*p)
                        break;
 
+               if (strcmp(p, PAYLOAD_PATTERN) == 0) {
+                       /* payload pattern recognized here, this is not an arg anymore,
+                        * the payload starts at the first byte that follows the zero
+                        * after the pattern.
+                        */
+                       payload = p + strlen(PAYLOAD_PATTERN) + 1;
+                       break;
+               }
+
                args[i] = p;
                while (1) {
                        p += strcspn(p, " \t\\");
@@ -955,8 +948,10 @@ static void cli_io_handler(struct appctx *appctx)
                                 * Its location is not remembered here, this is just to switch
                                 * to a gathering mode.
                                 */
-                               if (strcmp(appctx->chunk->area + appctx->chunk->data - strlen(PAYLOAD_PATTERN), PAYLOAD_PATTERN) == 0)
+                               if (strcmp(appctx->chunk->area + appctx->chunk->data - strlen(PAYLOAD_PATTERN), PAYLOAD_PATTERN) == 0) {
                                        appctx->st1 |= APPCTX_CLI_ST1_PAYLOAD;
+                                       appctx->chunk->data++; // keep the trailing \0 after '<<'
+                               }
                                else {
                                        /* no payload, the command is complete: parse the request */
                                        cli_parse_request(appctx);