From ad63582eb9bc57de6d74674025451780067a56a5 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Mon, 8 Aug 2016 19:21:09 +0200 Subject: [PATCH] BUG/MEDIUM: samples: make smp_dup() always duplicate the sample Vedran Furac reported a strange problem where the "base" sample fetch would not always work for tracking purposes. In fact, it happens that commit bc8c404 ("MAJOR: stick-tables: use sample types in place of dedicated types") merged in 1.6 exposed a fundamental bug related to the way samples use chunks as strings. The problem is that chunks convey a base pointer, a length and an optional size, which may be zero when unknown or when the chunk is allocated from a read-only location. The sole purpose of this size is to know whether or not the chunk may be appended new data. This size cause some semantics issue in the sample, which has its own SMP_F_CONST flag to indicate read-only contents. The problem was emphasized by the commit above because it made use of new calls to smp_dup() to convert a sample to a table key. And since smp_dup() would only check the SMP_F_CONST flag, it would happily return read-write samples indicating size=0. So some tests were added upon smp_dup() return to ensure that the actual length is smaller than size, but this in fact made things even worse. For example, the "sni" server directive does some bad stuff on many occasions because it limits len to size-1 and effectively sets it to -1 and writes the zero byte before the beginning of the string! It is therefore obvious that smp_dup() needs to be modified to take this nature of the chunks into account. It's not enough but is needed. The core of the problem comes from the fact that smp_dup() is called for 5 distinct needs which are not always fulfilled : 1) duplicate a sample to keep a copy of it during some operations 2) ensure that the sample is rewritable for a converter like upper() 3) ensure that the sample is terminated with a \0 4) set a correct size on the sample 5) grow the sample in case it was extracted from a partial chunk Case 1 is not used for now, so we can ignore it. Case 2 indicates the wish to modify the sample, so its R/O status must be removed if any, but there's no implied requirement that the chunk becomes larger. Case 3 is used when the sample has to be made compatible with libc's str* functions. There's no need to make it R/W nor to duplicate it if it is already correct. Case 4 can happen when the sample's size is required (eg: before performing some changes that must fit in the buffer). Case 5 is more or less similar but will happen when the sample by be grown but we want to ensure we're not bound by the current small size. So the proposal is to have different functions for various operations. One will ensure a sample is safe for use with str* functions. Another one will ensure it may be rewritten in place. And smp_dup() will have to perform an inconditional duplication to guarantee at least #5 above, and implicitly all other ones. This patch only modifies smp_dup() to make the duplication inconditional. It is enough to fix both the "base" sample fetch and the "sni" server directive, and all use cases in general though not always optimally. More patches will follow to address them more optimally and even better than the current situation (eg: avoid a dup just to add a \0 when possible). The bug comes from an ambiguous design, so its roots are old. 1.6 is affected and a backport is needed. In 1.5, the function already existed but was only used by two converters modifying the data in place, so the bug has no effect there. --- include/types/sample.h | 6 ++++++ src/sample.c | 26 +++++++++++++++++++++----- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/include/types/sample.h b/include/types/sample.h index 955f9bda43..cf0528edc2 100644 --- a/include/types/sample.h +++ b/include/types/sample.h @@ -236,6 +236,12 @@ union smp_ctx { void *a[8]; /* any array of up to 8 pointers */ }; +/* Note: the strings below make use of chunks. Chunks may carry an allocated + * size in addition to the length. The size counts from the beginning (str) + * to the end. If the size is unknown, it MUST be zero, in which case the + * sample will automatically be duplicated when a change larger than has + * to be performed. Thus it is safe to always set size to zero. + */ struct meth { enum http_meth_t meth; struct chunk str; diff --git a/src/sample.c b/src/sample.c index 645cb81217..e0a880ec6f 100644 --- a/src/sample.c +++ b/src/sample.c @@ -637,7 +637,12 @@ static int c_int2str(struct sample *smp) return 1; } -/* This function duplicates data and removes the flag "const". */ +/* This function inconditionally duplicates data and removes the "const" flag. + * For strings and binary blocks, it also provides a known allocated size with + * a length that is capped to the size, and ensures a trailing zero is always + * appended for strings. This is necessary for some operations which may + * require to extend the length. It returns 0 if it fails, 1 on success. + */ int smp_dup(struct sample *smp) { struct chunk *trash; @@ -645,8 +650,6 @@ int smp_dup(struct sample *smp) /* If the const flag is not set, we don't need to duplicate the * pattern as it can be modified in place. */ - if (!(smp->flags & SMP_F_CONST)) - return 1; switch (smp->data.type) { case SMP_T_BOOL: @@ -656,11 +659,24 @@ int smp_dup(struct sample *smp) case SMP_T_IPV6: /* These type are not const. */ break; + case SMP_T_STR: + trash = get_trash_chunk(); + trash->len = smp->data.u.str.len; + if (trash->len > trash->size - 1) + trash->len = trash->size - 1; + + memcpy(trash->str, smp->data.u.str.str, trash->len); + trash->str[trash->len] = 0; + smp->data.u.str = *trash; + break; + case SMP_T_BIN: - /* Duplicate data. */ trash = get_trash_chunk(); - trash->len = smp->data.u.str.len < trash->size ? smp->data.u.str.len : trash->size; + trash->len = smp->data.u.str.len; + if (trash->len > trash->size) + trash->len = trash->size; + memcpy(trash->str, smp->data.u.str.str, trash->len); smp->data.u.str = *trash; break; -- 2.39.5