From 19d421fb5c68caef8cacd1410b6eae526956c4fc Mon Sep 17 00:00:00 2001 From: Greg Felice Date: Wed, 1 Jul 2026 17:07:32 -0400 Subject: [PATCH] Fix stack overflow and precision loss in toFloatList() conversion 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 --- regress/expected/expr.out | 20 ++++++++++++++++++++ regress/sql/expr.sql | 10 ++++++++++ src/backend/utils/adt/agtype.c | 19 +++++++++++-------- 3 files changed, 41 insertions(+), 8 deletions(-) diff --git a/regress/expected/expr.out b/regress/expected/expr.out index d6b9ac155..806a6f65c 100644 --- a/regress/expected/expr.out +++ b/regress/expected/expr.out @@ -3548,6 +3548,26 @@ $$) AS (toFloatList agtype); [1.20002] (1 row) +-- large magnitudes must not overflow the conversion (regression: unbounded +-- sprintf into a fixed stack buffer overflowed for values like 1.0e308) +SELECT * FROM cypher('expr', $$ + RETURN toFloatList([1.0e308, -1.0e308]) +$$) AS (toFloatList agtype); + tofloatlist +------------------- + [1e+308, -1e+308] +(1 row) + +-- precision must be preserved (regression: "%f" format truncated to 6 digits, +-- so 0.123456789 came back as 0.123457) +SELECT * FROM cypher('expr', $$ + RETURN toFloatList([0.123456789]) +$$) AS (toFloatList agtype); + tofloatlist +--------------- + [0.123456789] +(1 row) + -- should return null SELECT * FROM cypher('expr', $$ RETURN toFloatList(['true']) diff --git a/regress/sql/expr.sql b/regress/sql/expr.sql index 251349cef..d4d900a1c 100644 --- a/regress/sql/expr.sql +++ b/regress/sql/expr.sql @@ -1520,6 +1520,16 @@ $$) AS (toFloatList agtype); SELECT * FROM cypher('expr', $$ RETURN toFloatList([1.20002]) $$) AS (toFloatList agtype); +-- large magnitudes must not overflow the conversion (regression: unbounded +-- sprintf into a fixed stack buffer overflowed for values like 1.0e308) +SELECT * FROM cypher('expr', $$ + RETURN toFloatList([1.0e308, -1.0e308]) +$$) AS (toFloatList agtype); +-- precision must be preserved (regression: "%f" format truncated to 6 digits, +-- so 0.123456789 came back as 0.123457) +SELECT * FROM cypher('expr', $$ + RETURN toFloatList([0.123456789]) +$$) AS (toFloatList agtype); -- should return null SELECT * FROM cypher('expr', $$ RETURN toFloatList(['true']) diff --git a/src/backend/utils/adt/agtype.c b/src/backend/utils/adt/agtype.c index bf69bf1fa..0e1a7963f 100644 --- a/src/backend/utils/adt/agtype.c +++ b/src/backend/utils/adt/agtype.c @@ -7099,8 +7099,6 @@ Datum age_tofloatlist(PG_FUNCTION_ARGS) int count; int i; bool is_valid = false; - float8 float_num; - char buffer[64]; /* check for null */ if (PG_ARGISNULL(0)) @@ -7160,11 +7158,16 @@ Datum age_tofloatlist(PG_FUNCTION_ARGS) case AGTV_FLOAT: + /* + * The element is already a float8, so assign it directly. The + * previous approach formatted it to a string with sprintf() and + * re-parsed it: that both overflowed a fixed 64-byte stack buffer + * for large magnitudes (e.g. 1.0e308 needs ~317 chars) and lost + * precision, since "%f" truncates to 6 fractional digits. Direct + * assignment avoids both problems. + */ float_elem.type = AGTV_FLOAT; - float_num = elem->val.float_value; - sprintf(buffer, "%f", float_num); - string = buffer; - float_elem.val.float_value = float8in_internal_null(string, NULL, "double precision", string, &is_valid); + float_elem.val.float_value = elem->val.float_value; agis_result.res = push_agtype_value(&agis_result.parse_state, WAGT_ELEM, &float_elem); break; @@ -8146,7 +8149,7 @@ Datum age_tostringlist(PG_FUNCTION_ARGS) case AGTV_FLOAT: - sprintf(buffer, "%.*g", DBL_DIG, elem->val.float_value); + snprintf(buffer, sizeof(buffer), "%.*g", DBL_DIG, elem->val.float_value); string_elem.val.string.val = pstrdup(buffer); string_elem.val.string.len = strlen(buffer); @@ -8157,7 +8160,7 @@ Datum age_tostringlist(PG_FUNCTION_ARGS) case AGTV_INTEGER: - sprintf(buffer, "%ld", elem->val.int_value); + snprintf(buffer, sizeof(buffer), "%ld", elem->val.int_value); string_elem.val.string.val = pstrdup(buffer); string_elem.val.string.len = strlen(buffer);