Обсуждение: pgsql: Fix several one-byte buffer over-reads in to_number

Fix several one-byte buffer over-reads in to_number

Several places in NUM_numpart_from_char(), which is called from the SQL
function to_number(text, text), could accidentally read one byte past
the end of the input buffer (which comes from the input text datum and
is not null-terminated).

1. One leading space character would be skipped, but there was no check
   that the input was at least one byte long.  This does not happen in
   practice, but for defensiveness, add a check anyway.

2. Commit 4a3a1e2cf apparently accidentally doubled that code that skips
   one space character (so that two spaces might be skipped), but there
   was no overflow check before skipping the second byte.  Fix by
   removing that duplicate code.

3. A logic error would allow a one-byte over-read when looking for a
   trailing sign (S) placeholder.

In each case, the extra byte cannot be read out directly, but looking at
it might cause a crash.

The third item was discovered by Piotr Stefaniak, the first two were
found and analyzed by Tom Lane and Peter Eisentraut.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/9a46324fd46506c86b190d3163902ed90072c53c

Modified Files
--------------
src/backend/utils/adt/formatting.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

On 2016-08-08 17:18, Peter Eisentraut wrote:
> Fix several one-byte buffer over-reads in to_number

I've been meaning to update my patch like this, but didn't want to 
bother you before trying to find more issues with formatting.c (still 
haven't found the time for that, sadly):

@@ -4188,13 +4188,10 @@ NUM_numpart_from_char(NUMProc *Np, int id, int 
input_len)
                  (id == NUM_0 || id == NUM_9) ? "NUM_0/9" : id == 
NUM_DEC ? "NUM_DEC" : "???");
  #endif

-       if (*Np->inout_p == ' ')
-               Np->inout_p++;
-
  #define OVERLOAD_TEST  (Np->inout_p >= Np->inout + input_len)
  #define AMOUNT_TEST(_s) (input_len-(Np->inout_p-Np->inout) >= _s)

-       if (*Np->inout_p == ' ')
+       while (!OVERLOAD_TEST && isspace((unsigned char) *Np->inout_p))
                 Np->inout_p++;

         if (OVERLOAD_TEST)

Piotr Stefaniak <postgres@piotr-stefaniak.me> writes:
> I've been meaning to update my patch like this, but didn't want to
> bother you before trying to find more issues with formatting.c (still
> haven't found the time for that, sadly):

> -       if (*Np->inout_p == ' ')
> +       while (!OVERLOAD_TEST && isspace((unsigned char) *Np->inout_p))
>                  Np->inout_p++;

Meh.  I agree that replacing the "== ' '" test with isspace() would be
an improvement, since that seems to be the way it's done elsewhere in
formatting.c.  But changing this into a loop, so that it's willing to
consume any amount of whitespace, is a nontrivial change in the
specification of to_number().  I'm not at all sure it's a good idea;
IMO the point of to_number() is to parse numbers according to a fairly
tightly controlled format.

I'd even argue that unconditionally consuming a single space is the wrong
thing here.  Rather, I think what this is meant to be doing is treating a
space as one of the possible alternatives for a sign character, and so
instead of this what the code ought to be is an alternative on the same
footing as '+' or '-', a few lines down:

            else if (*Np->inout_p == '+')
            {
                *Np->number = '+';        /* set + */
                Np->inout_p++;
            }
+            else if (isspace((unsigned char) *Np->inout_p))
+            {
+                *Np->number = '+';        /* set + */
+                Np->inout_p++;
+            }
        }
    }


            regards, tom lane