pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #17839: Heap-buffer overflow on float8_to_char with invalid template
5+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2023-03-14 b081fe4199 6d3a9a60f7 8e33fb9ef1 386a26023a a67c75f825 7cac191057 Fix corner case bug in numeric to_char() some more.
* BUG #17839: Heap-buffer overflow on float8_to_char with invalid template
@ 2023-03-14 01:51 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 5+ messages in thread
From: PG Bug reporting form @ 2023-03-14 01:51 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: thiagotnunes@gmail.com
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.
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: BUG #17839: Heap-buffer overflow on float8_to_char with invalid template
@ 2023-03-14 22:49 Tom Lane <tgl@sss.pgh.pa.us>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2023-03-14 22:49 UTC (permalink / raw)
To: thiagotnunes@gmail.com; +Cc: pgsql-bugs@lists.postgresql.org
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
Attachments:
fix-wild-read-in-to_char.patch (text/x-diff, 2.1K) download
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
@ 2023-03-15 01:17 Thiago Nunes <thiagotnunes@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 5+ messages in thread
From: Thiago Nunes @ 2023-03-15 01:17 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org
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
>
>
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: BUG #17839: Heap-buffer overflow on float8_to_char with invalid template
@ 2023-03-15 01:45 Tom Lane <tgl@sss.pgh.pa.us>
parent: Thiago Nunes <thiagotnunes@gmail.com>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2023-03-15 01:45 UTC (permalink / raw)
To: Thiago Nunes <thiagotnunes@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: BUG #17839: Heap-buffer overflow on float8_to_char with invalid template
@ 2023-03-15 02:02 Thiago Nunes <thiagotnunes@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 5+ messages in thread
From: Thiago Nunes @ 2023-03-15 02:02 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org
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
>
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-03-15 02:02 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-03-14 01:51 BUG #17839: Heap-buffer overflow on float8_to_char with invalid template PG Bug reporting form <noreply@postgresql.org>
2023-03-14 22:49 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-03-15 01:17 ` Thiago Nunes <thiagotnunes@gmail.com>
2023-03-15 01:45 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-03-15 02:02 ` Thiago Nunes <thiagotnunes@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox