Hello Willy,
On Fri, Apr 20, 2018 at 05:12:22PM +0200, Willy Tarreau wrote:
> Hi Aurélien,
>
> It seems to me that some places in the parser look for "<<" anywhere on the
> line (mainly the strstr() which even skips trailing spaces/tabs), and some
> parts of the logic only expect it at the end.
>
> I'm personally perfectly fine with being strict at the beginning and doing
> as you documented, ie using "<<\n" as the delimiter so that users don't get
> used to start to put it anywhere. I noticed anyway that it's not recognized
> as the delimiter if not at the end, but the logic seems to check for it.
That's right. The logic in cli_io_handler() looks for the pattern at the end of
the first line received. In cli_parse_request(), before the
tokenization, it will
rescan the input to look for the pattern again only if it knows it is there (the
flag APPCTX_CLI_ST1_PAYLOAD is set). I thought about storing its position
to avoid this second lookup but felt using an extra field for that
would not be that
good considering the parsing is not something done millions of times per
second. I don't like doing/storing things multiple times but here I think it's a
fair trade-off.
I will add some comments since I can see how it can be disturbing to see two
different logics.
> > + p = appctx->chunk->str;
> > + end = p + appctx->chunk->len;
> > +
> > + /* look for a payload */
> > + if (appctx->st1 & APPCTX_CLI_ST1_PAYLOAD) {
> > + payload = strstr(p, PAYLOAD_PATTERN);
> > + end = payload;
> > + /* skip the pattern */
> > + payload += strlen(PAYLOAD_PATTERN);
> > + /* skip whitespaces */
> > + payload += strspn(payload, " \t");
> > + }
>
> Be extremely careful with the str* functions in haproxy. Due to being used
> to process buffers in-place, most of the time our strings are *not* zero-
> terminated, which is why they're often put in chunks made of (str,len),
> or the new immediate strings (ist) also made of (ptr,len). The internal
> API is not very rich regarding this so some functions are implemented
> like strnistr() and others using ist like istist() which does the same
> as strstr() but on ist strings. It's among the things we have to do to
> unify the internal strings API as for now many operations are simply
> open-coded. At the very least, wherever the chunk is filled, a comment
> should indicate that the trailing zero is assumed to be present in the
> rest of the code, because the risk of the code being changed without
> keeping it is high.
I took extra care to be sure I was dealing only with C-strings because
even if it's not very fun to parse strings in C, it's even worse when
the are not zero terminated (hello nginx) and you always end up needing
a str-like() function the internal API does not provide. I will also
add comments.
> > + chunk_appendf(appctx->chunk, "%s", trash.str);
>
> Given that this will result in appctx->chunk always being equal to trash.str,
> I think you should take a look at cli_io_handler() to check if it still makes
> sense at all to use the trash there. Maybe you should simply rely on this
> allocated chunk all the time and save this copy and formatting.
Mmh, yes, I think we can read input data into appctx->chunk and drop the trash
variable.
>
> > From 63439f2b089613973f72a37dd5dda17f45f26545 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
> > Date: Wed, 18 Apr 2018 14:04:47 +0200
> > Subject: [PATCH 2/3] MINOR: map: Add payload support to "add map"
> (...)
> > diff --git a/src/map.c b/src/map.c
> > index d02a0255c..64222dc2e 100644
> > --- a/src/map.c
> > +++ b/src/map.c
> > + const char *end = payload + strlen(payload);
> > +
> > + while (payload < end) {
> (...)
> > + /* value */
> > + payload += strspn(payload, " \t");
> > + value = payload;
> > + l = strcspn(value, "\n");
> > + value[l] = 0;
> > + payload += l + 1;
>
> I think this one is not exactly good, as a string ending in "123\0" will
> cause payload to point past the \0. While there's theorically nothing
> wrong with this, and we know that on all supported architecture, ~0 is
> already not a valid pointer so this will not cause payload to become < end,
> it can at least be confusing when debugging. I'd rather do :
>
> payload += l;
> if (*payload)
> payload++;
Yes, 'payload' will go past the end of the string and that is why I
use 'end' and stop
dereferencing 'payload' after the increment but I totally see why it
can confuse people.
>
> > From 1b7f6c40b010521a8b55103c984d3c8f305e818c Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Aur=C3=A9lien=20Nephtali?= <aurelien.nepht...@corp.ovh.com>
> > Date: Wed, 18 Apr 2018 14:04:58 +0200
> > Subject: [PATCH 3/3] MINOR: ssl: Add payload support to "set ssl
> (...)
> > --- a/src/ssl_sock.c
> > +++ b/src/ssl_sock.c
> > @@ -8565,16 +8565,27 @@ static int cli_parse_set_ocspresponse(char **args,
> > char *payload, struct appctx
> > {
> > #if (defined SSL_CTRL_SET_TLSEXT_STATUS_REQ_CB && !defined OPENSSL_NO_OCSP)
> > char *err = NULL;
> > + int i, j;
> > +
> > + if (!payload)
> > + payload = args[3];
> >
> > /* Expect one parameter: the new response in base64 encoding */
> > - if (!*args[3]) {
> > + if (!*payload) {
> > appctx->ctx.cli.severity = LOG_ERR;
> > appctx->ctx.cli.msg = "'set ssl ocsp-response' expects response in base64
> > encoding.\n";
> > appctx->st0 = CLI_ST_PRINT;
> > return 1;
> > }
> >
> > - trash.len = base64dec(args[3], strlen(args[3]), trash.str, trash.size);
> > + /* remove \r and \n from the payload */
> > + for (i = 0, j = 0; payload[i]; i++) {
> > + if (payload[i] == '\r' || payload[i] == '\n')
> > + continue;
> > + payload[j++] = payload[i];
> > + }
> > +
> > + trash.len = base64dec(payload, strlen(payload), trash.str, trash.size);
>
> Here there is an issue, if you have some '\n' or '\r' in the payload, then
> strlen() will be wrong and a few extra chars will be sent :
>
> azert\nyuiop\nqsdfg\nhjklm\n
>
> will become :
>
> azertyuiopqsdfghjklmklm\n
> <------------------> j
> <-----------------------> strlen(payload)
>
> Note that it will not be noticeable on small strings spanning over 2 or 3
> lines as the trailing "=" will simply be replicated at the end, still
> looking like a valid end of string, but it will randomly break for longer
> strings.
>
> So you should use "j" instead of strlen(payload) here.
>
Oops, I indeed forgot to set [j] to 0. For the sake of consistency I
will correctly
zero-terminate 'payload' and use j instead of strlen().
> So please take a look at these points, and once you think they are OK
> (or don't need to be changed), please mention the command changes in the
> management doc for "add map" and "set ssl ocsp-response", and we can merge
> this.
I'll send new patches soon that address your comments and update the
documentation.
Thanks !
--
Aurélien.
