Copilot commented on code in PR #2410:
URL: https://github.com/apache/age/pull/2410#discussion_r3150328550
##########
src/backend/utils/adt/agtype.c:
##########
@@ -6556,7 +6556,7 @@ Datum age_tofloatlist(PG_FUNCTION_ARGS)
int i;
bool is_valid = false;
float8 float_num;
- char buffer[64];
+ char buffer[400];
Review Comment:
`char buffer[400];` is a new magic number on the stack. Since the required
size is derived from `DBL_MAX_10_EXP` and the chosen formatting/precision,
consider defining a named constant (or computing the bound) so it's clear why
400 is sufficient and future format changes don't accidentally reintroduce
truncation/overflow risks.
##########
src/backend/utils/adt/agtype.c:
##########
@@ -6618,7 +6618,7 @@ Datum age_tofloatlist(PG_FUNCTION_ARGS)
float_elem.type = AGTV_FLOAT;
float_num = elem->val.float_value;
- sprintf(buffer, "%f", float_num);
+ snprintf(buffer, sizeof(buffer), "%f", float_num);
string = buffer;
float_elem.val.float_value = float8in_internal_null(string, NULL,
"double precision", string, &is_valid);
Review Comment:
The PR description mentions increasing the buffer to "allow full precision",
but this code still formats with "%f" (default precision 6), which both loses
precision and expands large values (e.g., DBL_MAX) into hundreds of digits.
Consider avoiding fixed-point formatting here by using Postgres' float8out
output (or a "%.*g" with DBL_DECIMAL_DIG) so the conversion is round-trippable
without requiring such a large stack buffer.
##########
src/backend/utils/adt/agtype.c:
##########
@@ -6618,7 +6618,7 @@ Datum age_tofloatlist(PG_FUNCTION_ARGS)
float_elem.type = AGTV_FLOAT;
float_num = elem->val.float_value;
- sprintf(buffer, "%f", float_num);
+ snprintf(buffer, sizeof(buffer), "%f", float_num);
string = buffer;
Review Comment:
This change fixes a security-sensitive crash vector (very large float
formatted via `%f`). There are existing regression tests for `toFloatList()` in
`regress/sql/expr.sql`, but none appear to cover extremely large float values
(e.g., something near `DBL_MAX`). Adding a regression case that exercises
`toFloatList([<very large float>])` would help prevent reintroducing the
overflow/truncation issue.
##########
src/backend/utils/adt/agtype.c:
##########
@@ -7585,7 +7585,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);
Review Comment:
`elem->val.int_value` is an `int64` (see `struct agtype_value`), but the
format string uses `%ld`, which is not portable (e.g., `long` is 32-bit on some
platforms) and can lead to incorrect output/UB. Use `INT64_FORMAT` (or
`pg_lltoa` / `int8out`) when formatting `int64` values.
```suggestion
snprintf(buffer, sizeof(buffer), INT64_FORMAT,
elem->val.int_value);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
