Copilot commented on code in PR #2408:
URL: https://github.com/apache/age/pull/2408#discussion_r3150394358
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -1167,6 +1173,8 @@ static void append_element(agtype_parse_state *pstate,
if (array->val.array.num_elems >= pstate->size)
{
pstate->size *= 2;
+ if ((*pstate)->size > SIZE_MAX / sizeof(agtype_value))
Review Comment:
In `append_element`, `pstate` is an `agtype_parse_state *`, so
`(*pstate)->size` is invalid and will not compile. Use the direct field access
form already used elsewhere in this function (`pstate->size`).
```suggestion
if (pstate->size > SIZE_MAX / sizeof(agtype_value))
```
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -1034,6 +1036,8 @@ static agtype_value
*push_agtype_value_scalar(agtype_parse_state **pstate,
(*pstate)->cont_val.type = AGTV_OBJECT;
(*pstate)->cont_val.val.object.num_pairs = 0;
(*pstate)->size = 4;
+ if ((*pstate)->size > SIZE_MAX / sizeof(agtype_pair))
+ ereport(ERROR, "allocation request exceeds size limits");
(*pstate)->cont_val.val.object.pairs =
palloc(sizeof(agtype_pair) * (*pstate)->size);
Review Comment:
Same issue as above: `ereport(ERROR, "...")` is not a valid `ereport`
invocation in this file (see existing patterns like `ereport(ERROR,
(errmsg(...)))`). This will fail to compile unless rewritten to the standard
`ereport(ERROR, (...))` form.
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -1120,6 +1124,8 @@ static void append_key(agtype_parse_state *pstate,
agtype_value *string)
if (object->val.object.num_pairs >= pstate->size)
{
pstate->size *= 2;
+ if ((*pstate)->size > SIZE_MAX / sizeof(agtype_pair))
+ ereport(ERROR, "allocation request exceeds size limits");
Review Comment:
The overflow check is placed after `pstate->size *= 2`. If `Size` wraps on
multiplication, `pstate->size` can become small and pass the subsequent
`SIZE_MAX / sizeof(...)` check, reintroducing the exact under-allocation risk
this PR is trying to fix. The check needs to happen before doubling (or use a
safe arithmetic helper) so that overflow in the growth step itself is prevented.
```suggestion
if (pstate->size > SIZE_MAX / 2 ||
pstate->size > (SIZE_MAX / sizeof(agtype_pair)) / 2)
ereport(ERROR, "allocation request exceeds size limits");
pstate->size *= 2;
```
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -1167,6 +1173,8 @@ static void append_element(agtype_parse_state *pstate,
if (array->val.array.num_elems >= pstate->size)
{
pstate->size *= 2;
+ if ((*pstate)->size > SIZE_MAX / sizeof(agtype_value))
+ ereport(ERROR, "allocation request exceeds size limits");
Review Comment:
Same pattern as the object growth path: the overflow check comes after
`pstate->size *= 2`, so an overflow in the doubling step itself can wrap `size`
and evade the subsequent `SIZE_MAX / sizeof(...)` guard. Move the guard before
the doubling (or use safe-arithmetic helpers) to ensure capacity growth can’t
wrap and cause an under-sized `repalloc`.
```suggestion
size_t new_size;
if (pstate->size > SIZE_MAX / 2)
ereport(ERROR, "allocation request exceeds size limits");
new_size = pstate->size * 2;
if (new_size > SIZE_MAX / sizeof(agtype_value))
ereport(ERROR, "allocation request exceeds size limits");
pstate->size = new_size;
```
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -1120,6 +1124,8 @@ static void append_key(agtype_parse_state *pstate,
agtype_value *string)
if (object->val.object.num_pairs >= pstate->size)
{
pstate->size *= 2;
+ if ((*pstate)->size > SIZE_MAX / sizeof(agtype_pair))
+ ereport(ERROR, "allocation request exceeds size limits");
Review Comment:
The newly added `ereport` in this block uses a raw string literal. In this
file, `ereport` must be invoked with an error clause list (e.g.,
`errmsg(...)`), otherwise it won’t compile. Please rewrite this `ereport` to
match the existing patterns in the surrounding code.
```suggestion
ereport(ERROR,
(errmsg("allocation request exceeds size limits")));
```
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -1120,6 +1124,8 @@ static void append_key(agtype_parse_state *pstate,
agtype_value *string)
if (object->val.object.num_pairs >= pstate->size)
{
pstate->size *= 2;
+ if ((*pstate)->size > SIZE_MAX / sizeof(agtype_pair))
Review Comment:
In `append_key`, `pstate` is an `agtype_parse_state *`, so `(*pstate)->size`
is invalid (compile error). This should reference the struct directly
(consistent with the surrounding code using `pstate->size`).
```suggestion
if (pstate->size > SIZE_MAX / sizeof(agtype_pair))
```
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -1167,6 +1173,8 @@ static void append_element(agtype_parse_state *pstate,
if (array->val.array.num_elems >= pstate->size)
{
pstate->size *= 2;
+ if ((*pstate)->size > SIZE_MAX / sizeof(agtype_value))
+ ereport(ERROR, "allocation request exceeds size limits");
Review Comment:
The `ereport` added here is using a raw string literal, but `ereport` in
this codebase expects an `errmsg(...)` (and optionally `errcode(...)`) clause
list; as written, it will not compile. Update this call to follow the existing
`ereport(ERROR, (errmsg(...)))` style in the file.
```suggestion
ereport(ERROR, (errmsg("allocation request exceeds size
limits")));
```
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -1023,6 +1023,8 @@ static agtype_value
*push_agtype_value_scalar(agtype_parse_state **pstate,
{
(*pstate)->size = 4;
}
+ if ((*pstate)->size > SIZE_MAX / sizeof(agtype_value))
+ ereport(ERROR, "allocation request exceeds size limits");
(*pstate)->cont_val.val.array.elems =
palloc(sizeof(agtype_value) * (*pstate)->size);
Review Comment:
`ereport` is being called with a raw string here, but in this codebase
`ereport` expects an error-reporting clause (e.g., `errmsg(...)`, optionally
with `errcode(...)`). As written, this won’t compile (and it also deviates from
the nearby `ERRCODE_PROGRAM_LIMIT_EXCEEDED` usage). Update these new overflow
checks to use the standard `ereport(ERROR, (errmsg(...)))` form (and consider
an appropriate `errcode`).
##########
src/backend/utils/adt/agtype_util.c:
##########
@@ -1631,6 +1639,8 @@ bool agtype_deep_contains(agtype_iterator **val,
uint32 j = 0;
/* Make room for all possible values */
+ if (num_lhs_elems > SIZE_MAX / sizeof(agtype_value))
+ ereport(ERROR, "allocation request exceeds size
limits");
Review Comment:
This new guard uses `ereport(ERROR, "...")`, which doesn’t match the valid
`ereport(ERROR, (errmsg(...)))` style used elsewhere in this file and will not
compile as-is. Update it to the standard `ereport(ERROR, (...))` form so the
overflow check is actually effective.
```suggestion
ereport(ERROR,
(errmsg("allocation request exceeds size
limits")));
```
--
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]
