]> git.ipfire.org Git - thirdparty/haproxy.git/commit
BUG/MINOR: tools/log: invalid encode_{chunk,string} usage
authorAurelien DARRAGON <adarragon@haproxy.com>
Tue, 9 Apr 2024 09:44:54 +0000 (11:44 +0200)
committerAurelien DARRAGON <adarragon@haproxy.com>
Tue, 9 Apr 2024 15:35:45 +0000 (17:35 +0200)
commit8226e92eb07cc1a2ed8d30f4bd33d54c8e5328b8
tree1f023afb23a25205640ec08fe0563a560be12ffc
parentb15f6dfae871f6ebfefa8a4e89958c1c70078e09
BUG/MINOR: tools/log: invalid encode_{chunk,string} usage

encode_{chunk,string}() is often found to be used this way:

  ret = encode_{chunk,string}(start, stop...)
  if (ret == NULL || *ret != '\0') {
//error
  }
  //success

Indeed, encode_{chunk,string} will always try to add terminating NULL byte
to the output string, unless no space is available for even 1 byte.
However, it means that for the caller to be able to spot an error, then it
must provide a buffer (here: start) which is already initialized.

But this is wrong: not only this is very tricky to use, but since those
functions don't return NULL on failure, then if the output buffer was not
properly initialized prior to calling the function, the caller will
perform invalid reads when checking for failure this way. Moreover, even
if the buffer is initialized, we cannot reliably tell if the function
actually failed this way because if the buffer was previously initialized
with NULL byte, then the caller might think that the call actually
succeeded (since the function didn't return NULL and didn't update the
buffer).

Also, sess_build_logline() relies lf_encode_{chunk,string}() functions
which are in fact wrappers for encode_{chunk,string}() functions and thus
exhibit the same error handling mechanism. It turns out that
sess_build_logline() makes unsafe use of those functions because it uses
the error-checking logic mentionned above while buffer (tmplog) is not
guaranteed to be initialized when entering the function. This may
ultimately cause malfunctions or invalid reads if the output buffer is
lacking space.

To fix the issue once and for all and prevent similar bugs from being
introduced, we make it so encode_{string, chunk} and escape_string()
(based on encode_string()) now explicitly return NULL on failure
(when the function failed to write at least the ending NULL byte)

lf_encode_{string,chunk}() helpers had to be patched as well due to code
duplication.

This should be backported to all stable versions.

[ada: for 2.4 and 2.6 the patch won't apply as-is, it might be helpful to
 backport ae1e14d65 ("CLEANUP: tools: removing escape_chunk() function")
 first, considering it's not very relevant to maintain a dead function]
include/haproxy/tools.h
src/log.c
src/tools.c