connection: Detect overflows in length field.

[Jann Horn]

On Thu, Aug 2, 2018 at 8:37 PM Michal Srb <msrb at suse.com> wrote:
>
> The length field can be any uint32 value. Two kinds of overflows may
> happen on 32 bit systems:
>
> 1) If the value is in range [UINT32_MAX-3, UINT32_MAX], the DIV_ROUNDUP
> will turn it into 0. Then `next` equals `p` and so the big `length` is not
> detected. But the wl_array will contain the original big value. Probably
> leading to crashes it is used later.
>
> 2) The pointer may overflow if the `length` is sufficiently big. In that
> case `next` will point to some memory before the beginning of the buffer
> and the the check `next > end` is not triggered. Using `next` later can
> crash.
>
> Signed-off-by: Michal Srb <msrb at suse.com>
> ---
>
> Note that the problem happens only on 32bit systems.
>
>  src/connection.c        | 19 +++++++++++++++++--
>  tests/connection-test.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/src/connection.c b/src/connection.c
> index 294c521..b48f3a4 100644
> --- a/src/connection.c
> +++ b/src/connection.c
> @@ -46,6 +46,21 @@
>
>  #define DIV_ROUNDUP(n, a) ( ((n) + ((a) - 1)) / (a) )
>
> +inline uint32_t *
> +ptr_add_saturating(uint32_t *p, uint32_t length) {
> +       // Make sure that rounding length up will not overflow.
> +       if (length > UINT32_MAX - sizeof(uint32_t))
> +               return (uint32_t*) INTPTR_MAX;
> +
> +       length = DIV_ROUNDUP(length, sizeof(uint32_t));
> +
> +       // Make sure that adding length to p will not overflow.
> +       if (length * sizeof(uint32_t) > UINTPTR_MAX - (uintptr_t) p)
> +               return (uint32_t*) INTPTR_MAX;
> +
> +       return p + length;
> +}

This patch probably fixes the problem in practice, but AFAICS the
patched version still, in theory, contains undefined behavior. The C99
standard says: "If both the pointer operand and the result point to
elements of the same array object, or one past the last element of the
array object, the evaluation shall not produce an overflow; otherwise,
the behavior is undefined."
Can you please change the patch so that it compares lengths instead of
comparing potentially out-of-bounds pointers, thereby avoiding
undefined behavior?