You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@age.apache.org by "sfc-gh-dachristensen (via GitHub)" <gi...@apache.org> on 2026/04/22 21:19:46 UTC
[PR] Critical: check for Size overflow when using palloc [age]
sfc-gh-dachristensen opened a new pull request, #2408:
URL: https://github.com/apache/age/pull/2408
Multiple locations compute allocation sizes via unchecked integer multiplication:
Line 1027: palloc(sizeof(agtype_value) * (*pstate)->size) — size doubles repeatedly (lines 1122, 1169); near 0x40000000, multiplication overflows to a small value, leading to heap buffer overflow on subsequent writes. Line 1038: palloc(sizeof(agtype_pair) * (*pstate)->size) — same pattern. Lines 2267, 2366: sizeof(agtentry) * num_elems and sizeof(agtentry) * num_pairs * 2 — chained multiplication without overflow check. Line 1397: it->num_elems * sizeof(agtentry) * 2
Fix this by making explicit checks.
--
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: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Critical: check for Size overflow when using palloc [age]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Critical: check for Size overflow when using palloc [age]
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani closed pull request #2408: Critical: check for Size overflow when using palloc
URL: https://github.com/apache/age/pull/2408
--
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: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Critical: check for Size overflow when using palloc [age]
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on PR #2408:
URL: https://github.com/apache/age/pull/2408#issuecomment-4330728606
@sfc-gh-dachristensen I'm going to reject and close this PR as it isn't necessary -
Below is from Opus 4.7, but I have also reviewed the work. Additionally, it doesn't follow our coding standards: all blocks are always wrapped with braces. While not all of our code is this way, originally, all new code is.
## Summary
**Verdict: the PR is misdiagnosed and the patch is broken. Should be rejected as written.**
The PR claims integer-overflow → heap-buffer-overflow via palloc size-multiplication in `agtype_util.c`. After checking each cited site against the source, the conclusion is:
1. **The vulnerabilities described are not reachable** — pre-existing guards block them.
2. **The patch as posted does not compile.**
3. **Some checks the patch adds are dead code.**
4. **Several sites the description mentions are not actually patched in the diff.**
5. **Where the patch is correct, it is hardening, not a fix.**
--
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: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org