Thread: BUG #17839: Heap-buffer overflow on float8_to_char with invalid template
BUG #17839: Heap-buffer overflow on float8_to_char with invalid template
From
PG Bug reporting form
Date:
The following bug has been logged on the website: Bug reference: 17839 Logged by: Thiago Nunes Email address: thiagotnunes@gmail.com PostgreSQL version: 15.2 Operating system: Linux Description: Heap-buffer overflow on float8_to_char when format exceeds max double digits. I noticed this when running tests with memory sanitiser (msan). The following example triggers the failure (considering max double digits `DBL_DIG` is 15): ``` float8_to_char(12345678901, "FM9999999999D999990") ``` Explanation below: 1. After parsing the format, `Num.pre` will be 10, `Num.post` will be 6 `Num.zero_end` will be 16 (https://github.com/postgres/postgres/blob/REL_15_2/src/backend/utils/adt/formatting.c#L1196-L1228) 2. The template size is greater than the `DBL_DIG`, `Num.post` will be moved back here (https://github.com/postgres/postgres/blob/REL_15_2/src/backend/utils/adt/formatting.c#L6688-L6689). 3. The shortened template with the max `DBL_DIG` will be "stringfied" out here (https://github.com/postgres/postgres/blob/REL_15_2/src/backend/utils/adt/formatting.c#L6712-L6717). The result will be "##########.####" (10 significant digits + '.' + 4 decimal digits). 4. `Np->last_relevant` will be lesser than `Num->zero_end`, so it is updated to an invalid position in the result above (pointer + 16) here (https://github.com/postgres/postgres/blob/REL_15_2/src/backend/utils/adt/formatting.c#L5740-L5743). 5. When applying FILLMODE here (https://github.com/postgres/postgres/blob/REL_15_2/src/backend/utils/adt/formatting.c#L5563), it will try to get the character at Np->last_relevant, which is out of bounds.
PG Bug reporting form <noreply@postgresql.org> writes: > Heap-buffer overflow on float8_to_char when format exceeds max double > digits. I noticed this when running tests with memory sanitiser (msan). > The following example triggers the failure (considering max double digits > `DBL_DIG` is 15): > float8_to_char(12345678901, "FM9999999999D999990") Thanks for the report! For reasons that aren't entirely clear to me, valgrind didn't whine about this until I added a few more zeroes to the format string, like SELECT to_char('12345678901'::float8, 'FM9999999999D9999900000000000000000'); Nonetheless, it did see the issue at that point. I'm inclined to fix it by making the code that adjusts Np->last_relevant more paranoid, as attached. regards, tom lane diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c index f3f4db5ef6..e6246dc44b 100644 --- a/src/backend/utils/adt/formatting.c +++ b/src/backend/utils/adt/formatting.c @@ -5695,13 +5695,20 @@ NUM_processor(FormatNode *node, NUMDesc *Num, char *inout, /* * If any '0' specifiers are present, make sure we don't strip - * those digits. + * those digits. But don't advance last_relevant beyond the last + * character of the Np->number string, which is a hazard if the + * number got shortened due to precision limitations. */ if (Np->last_relevant && Np->Num->zero_end > Np->out_pre_spaces) { + int last_zero_pos; char *last_zero; - last_zero = Np->number + (Np->Num->zero_end - Np->out_pre_spaces); + /* note that Np->number cannot be zero-length here */ + last_zero_pos = strlen(Np->number) - 1; + last_zero_pos = Min(last_zero_pos, + Np->Num->zero_end - Np->out_pre_spaces); + last_zero = Np->number + last_zero_pos; if (Np->last_relevant < last_zero) Np->last_relevant = last_zero; } diff --git a/src/test/regress/expected/numeric.out b/src/test/regress/expected/numeric.out index 2ac2c99b19..26930f4db4 100644 --- a/src/test/regress/expected/numeric.out +++ b/src/test/regress/expected/numeric.out @@ -1929,6 +1929,12 @@ SELECT to_char('100'::numeric, 'FM999'); 100 (1 row) +SELECT to_char('12345678901'::float8, 'FM9999999999D9999900000000000000000'); + to_char +----------------- + ##########.#### +(1 row) + -- Check parsing of literal text in a format string SELECT to_char('100'::numeric, 'foo999'); to_char diff --git a/src/test/regress/sql/numeric.sql b/src/test/regress/sql/numeric.sql index 6e9e6145d0..2dddb58625 100644 --- a/src/test/regress/sql/numeric.sql +++ b/src/test/regress/sql/numeric.sql @@ -979,6 +979,7 @@ FROM v; SELECT to_char('100'::numeric, 'FM999.9'); SELECT to_char('100'::numeric, 'FM999.'); SELECT to_char('100'::numeric, 'FM999'); +SELECT to_char('12345678901'::float8, 'FM9999999999D9999900000000000000000'); -- Check parsing of literal text in a format string SELECT to_char('100'::numeric, 'foo999');
Re: BUG #17839: Heap-buffer overflow on float8_to_char with invalid template
From
Thiago Nunes
Date:
Thanks Tom,
I think your solution deals with all the cases, but I would like to point out how I fixed it locally. I recalculated Num.zero_end after this line (https://github.com/postgres/postgres/blob/REL_15_2/src/backend/utils/adt/formatting.c#L6716):
```
Num.zero_end = Num.pre + Num.post;
```
Cheers,
On Wed, Mar 15, 2023 at 9:49 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
PG Bug reporting form <noreply@postgresql.org> writes:
> Heap-buffer overflow on float8_to_char when format exceeds max double
> digits. I noticed this when running tests with memory sanitiser (msan).
> The following example triggers the failure (considering max double digits
> `DBL_DIG` is 15):
> float8_to_char(12345678901, "FM9999999999D999990")
Thanks for the report! For reasons that aren't entirely clear to me,
valgrind didn't whine about this until I added a few more zeroes to the
format string, like
SELECT to_char('12345678901'::float8, 'FM9999999999D9999900000000000000000');
Nonetheless, it did see the issue at that point. I'm inclined to fix
it by making the code that adjusts Np->last_relevant more paranoid,
as attached.
regards, tom lane
Thiago Nunes <thiagotnunes@gmail.com> writes: > I think your solution deals with all the cases, but I would like to point > out how I fixed it locally. I recalculated Num.zero_end after this line ( > https://github.com/postgres/postgres/blob/REL_15_2/src/backend/utils/adt/formatting.c#L6716 > ): > ``` > Num.zero_end = Num.pre + Num.post; > ``` Hmm ... that seems a bit ad-hoc, because as far as I understand this code, zero_end is supposed to track where is the last '0' format character. That shouldn't change just because we decided that the data value overflowed. regards, tom lane
Re: BUG #17839: Heap-buffer overflow on float8_to_char with invalid template
From
Thiago Nunes
Date:
Got it, I thought that since we were filling the string with #s there, we should reposition the zero_end.
I will wait for your patch then, thanks for looking into it!
Cheers,
On Wed, Mar 15, 2023 at 12:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thiago Nunes <thiagotnunes@gmail.com> writes:
> I think your solution deals with all the cases, but I would like to point
> out how I fixed it locally. I recalculated Num.zero_end after this line (
> https://github.com/postgres/postgres/blob/REL_15_2/src/backend/utils/adt/formatting.c#L6716
> ):
> ```
> Num.zero_end = Num.pre + Num.post;
> ```
Hmm ... that seems a bit ad-hoc, because as far as I understand this
code, zero_end is supposed to track where is the last '0' format
character. That shouldn't change just because we decided that the
data value overflowed.
regards, tom lane