Skip to content

Commit fafdaa1

Browse files
Fix stack overflow and precision loss in toFloatList() conversion (#2451)
toFloatList()'s AGTV_FLOAT branch formatted each element with sprintf(buffer, "%f", ...) into a fixed 64-byte stack buffer and then re-parsed the string back into a float. This had two defects: 1. Stack overflow. "%f" prints the full integer part with no width limit, so a large magnitude overflows the 64-byte buffer. The value is query-reachable: RETURN toFloatList([1.0e308]) needs ~317 bytes (309 integer digits + ".000000") and smashes the stack. This is the issue reported in #2410. 2. Precision loss. "%f" emits only 6 fractional digits, so the format-and-reparse round trip was lossy -- toFloatList([0.123456789]) returned 0.123457. The element is already a float8, so the whole format/reparse step is unnecessary. Assign elem->val.float_value directly. This removes the stack buffer entirely (no magic buffer size to justify) and fixes both the overflow and the precision loss at once. Also harden toStringList(): its "%.*g"/"%ld" conversions use bounded formats and were never overflow-prone, but switch them from sprintf to snprintf as defensive depth. Add regression coverage to regress/sql/expr.sql for both the large magnitude case (no overflow) and precision preservation. This reimplements the fix originally proposed by David Christensen in #2410, whose report identified the sprintf overflow. Co-authored-by: David Christensen <david.christensen@snowflake.com>
1 parent 5137671 commit fafdaa1

3 files changed

Lines changed: 41 additions & 8 deletions

File tree

‎regress/expected/expr.out‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3548,6 +3548,26 @@ $$) AS (toFloatList agtype);
35483548
[1.20002]
35493549
(1 row)
35503550

3551+
-- large magnitudes must not overflow the conversion (regression: unbounded
3552+
-- sprintf into a fixed stack buffer overflowed for values like 1.0e308)
3553+
SELECT * FROM cypher('expr', $$
3554+
RETURN toFloatList([1.0e308, -1.0e308])
3555+
$$) AS (toFloatList agtype);
3556+
tofloatlist
3557+
-------------------
3558+
[1e+308, -1e+308]
3559+
(1 row)
3560+
3561+
-- precision must be preserved (regression: "%f" format truncated to 6 digits,
3562+
-- so 0.123456789 came back as 0.123457)
3563+
SELECT * FROM cypher('expr', $$
3564+
RETURN toFloatList([0.123456789])
3565+
$$) AS (toFloatList agtype);
3566+
tofloatlist
3567+
---------------
3568+
[0.123456789]
3569+
(1 row)
3570+
35513571
-- should return null
35523572
SELECT * FROM cypher('expr', $$
35533573
RETURN toFloatList(['true'])

‎regress/sql/expr.sql‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1520,6 +1520,16 @@ $$) AS (toFloatList agtype);
15201520
SELECT * FROM cypher('expr', $$
15211521
RETURN toFloatList([1.20002])
15221522
$$) AS (toFloatList agtype);
1523+
-- large magnitudes must not overflow the conversion (regression: unbounded
1524+
-- sprintf into a fixed stack buffer overflowed for values like 1.0e308)
1525+
SELECT * FROM cypher('expr', $$
1526+
RETURN toFloatList([1.0e308, -1.0e308])
1527+
$$) AS (toFloatList agtype);
1528+
-- precision must be preserved (regression: "%f" format truncated to 6 digits,
1529+
-- so 0.123456789 came back as 0.123457)
1530+
SELECT * FROM cypher('expr', $$
1531+
RETURN toFloatList([0.123456789])
1532+
$$) AS (toFloatList agtype);
15231533
-- should return null
15241534
SELECT * FROM cypher('expr', $$
15251535
RETURN toFloatList(['true'])

‎src/backend/utils/adt/agtype.c‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7099,8 +7099,6 @@ Datum age_tofloatlist(PG_FUNCTION_ARGS)
70997099
int count;
71007100
int i;
71017101
bool is_valid = false;
7102-
float8 float_num;
7103-
char buffer[64];
71047102

71057103
/* check for null */
71067104
if (PG_ARGISNULL(0))
@@ -7160,11 +7158,16 @@ Datum age_tofloatlist(PG_FUNCTION_ARGS)
71607158

71617159
case AGTV_FLOAT:
71627160

7161+
/*
7162+
* The element is already a float8, so assign it directly. The
7163+
* previous approach formatted it to a string with sprintf() and
7164+
* re-parsed it: that both overflowed a fixed 64-byte stack buffer
7165+
* for large magnitudes (e.g. 1.0e308 needs ~317 chars) and lost
7166+
* precision, since "%f" truncates to 6 fractional digits. Direct
7167+
* assignment avoids both problems.
7168+
*/
71637169
float_elem.type = AGTV_FLOAT;
7164-
float_num = elem->val.float_value;
7165-
sprintf(buffer, "%f", float_num);
7166-
string = buffer;
7167-
float_elem.val.float_value = float8in_internal_null(string, NULL, "double precision", string, &is_valid);
7170+
float_elem.val.float_value = elem->val.float_value;
71687171
agis_result.res = push_agtype_value(&agis_result.parse_state, WAGT_ELEM, &float_elem);
71697172

71707173
break;
@@ -8146,7 +8149,7 @@ Datum age_tostringlist(PG_FUNCTION_ARGS)
81468149

81478150
case AGTV_FLOAT:
81488151

8149-
sprintf(buffer, "%.*g", DBL_DIG, elem->val.float_value);
8152+
snprintf(buffer, sizeof(buffer), "%.*g", DBL_DIG, elem->val.float_value);
81508153
string_elem.val.string.val = pstrdup(buffer);
81518154
string_elem.val.string.len = strlen(buffer);
81528155

@@ -8157,7 +8160,7 @@ Datum age_tostringlist(PG_FUNCTION_ARGS)
81578160

81588161
case AGTV_INTEGER:
81598162

8160-
sprintf(buffer, "%ld", elem->val.int_value);
8163+
snprintf(buffer, sizeof(buffer), "%ld", elem->val.int_value);
81618164
string_elem.val.string.val = pstrdup(buffer);
81628165
string_elem.val.string.len = strlen(buffer);
81638166

0 commit comments

Comments
 (0)