Critical: check for Size overflow when using palloc by sfc-gh-dachristensen · Pull Request #2408 · apache/age

sfc-gh-dachristensen · 2026-04-22T21:19:43Z

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.

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. Signed-off-by: David Christensen <david.christensen@snowflake.com>

Copilot

Pull request overview

This PR aims to harden agtype construction/iteration utilities against integer-overflow during allocation size computations (e.g., sizeof(T) * count), preventing potential under-allocation and subsequent buffer overruns.

Changes:

Adds overflow guards before palloc() of agtype_value/agtype_pair backing arrays in push_agtype_value_scalar().
Adds overflow guards around dynamic growth (size *= 2 + repalloc()) in append_key() and append_element().
Adds an overflow guard before allocating a temporary container array in agtype_deep_contains().

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-27T21:54:32Z

+        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);


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.

Copilot · 2026-04-27T21:54:32Z

    if (object->val.object.num_pairs >= pstate->size)
    {
        pstate->size *= 2;
+        if ((*pstate)->size > SIZE_MAX / sizeof(agtype_pair))


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).

Suggested change

if ((*pstate)->size > SIZE_MAX / sizeof(agtype_pair))

if (pstate->size > SIZE_MAX / sizeof(agtype_pair))

Copilot · 2026-04-27T21:54:33Z

        pstate->size *= 2;
+        if ((*pstate)->size > SIZE_MAX / sizeof(agtype_pair))
+            ereport(ERROR, "allocation request exceeds size limits");


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.

Suggested change

pstate->size *= 2;

if ((*pstate)->size > SIZE_MAX / sizeof(agtype_pair))

ereport(ERROR, "allocation request exceeds size limits");

if (pstate->size > SIZE_MAX / 2 ||

pstate->size > (SIZE_MAX / sizeof(agtype_pair)) / 2)

ereport(ERROR, "allocation request exceeds size limits");

pstate->size *= 2;

Copilot · 2026-04-27T21:54:33Z

    if (array->val.array.num_elems >= pstate->size)
    {
        pstate->size *= 2;
+        if ((*pstate)->size > SIZE_MAX / sizeof(agtype_value))


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).

Suggested change

if ((*pstate)->size > SIZE_MAX / sizeof(agtype_value))

if (pstate->size > SIZE_MAX / sizeof(agtype_value))

Copilot · 2026-04-27T21:54:33Z

        pstate->size *= 2;
+        if ((*pstate)->size > SIZE_MAX / sizeof(agtype_value))
+            ereport(ERROR, "allocation request exceeds size limits");


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.

Suggested change

pstate->size *= 2;

if ((*pstate)->size > SIZE_MAX / sizeof(agtype_value))

ereport(ERROR, "allocation request exceeds size limits");

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;

Copilot · 2026-04-27T21:54:33Z

    {
        pstate->size *= 2;
+        if ((*pstate)->size > SIZE_MAX / sizeof(agtype_value))
+            ereport(ERROR, "allocation request exceeds size limits");


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.

Suggested change

ereport(ERROR, "allocation request exceeds size limits");

ereport(ERROR, (errmsg("allocation request exceeds size limits")));

Copilot · 2026-04-27T21:54:34Z

+        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);


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).

Copilot · 2026-04-27T21:54:34Z


                    /* Make room for all possible values */
+                    if (num_lhs_elems > SIZE_MAX / sizeof(agtype_value))
+                        ereport(ERROR, "allocation request exceeds size limits");


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.

Suggested change

ereport(ERROR, "allocation request exceeds size limits");

ereport(ERROR,

(errmsg("allocation request exceeds size limits")));

Copilot · 2026-04-27T21:54:34Z

    {
        pstate->size *= 2;
+        if ((*pstate)->size > SIZE_MAX / sizeof(agtype_pair))
+            ereport(ERROR, "allocation request exceeds size limits");


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.

Suggested change

ereport(ERROR, "allocation request exceeds size limits");

ereport(ERROR,

(errmsg("allocation request exceeds size limits")));

jrgemignani · 2026-04-27T22:02:29Z

@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:

The vulnerabilities described are not reachable — pre-existing guards block them.
The patch as posted does not compile.
Some checks the patch adds are dead code.
Several sites the description mentions are not actually patched in the diff.
Where the patch is correct, it is hardening, not a fix.

jrgemignani requested a review from Copilot April 27, 2026 21:42

Copilot AI reviewed Apr 27, 2026

View reviewed changes

jrgemignani closed this Apr 27, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Critical: check for Size overflow when using palloc#2408

Critical: check for Size overflow when using palloc#2408
sfc-gh-dachristensen wants to merge 1 commit intoapache:masterfrom
sfc-gh-dachristensen:fix/critical-palloc-size-overflow

sfc-gh-dachristensen commented Apr 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

Copilot AI Apr 27, 2026

Uh oh!

jrgemignani commented Apr 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if ((*pstate)->size > SIZE_MAX / sizeof(agtype_pair))
	if (pstate->size > SIZE_MAX / sizeof(agtype_pair))

	ereport(ERROR, "allocation request exceeds size limits");
	ereport(ERROR, (errmsg("allocation request exceeds size limits")));

	ereport(ERROR, "allocation request exceeds size limits");
	ereport(ERROR,
	(errmsg("allocation request exceeds size limits")));

Conversation

sfc-gh-dachristensen commented Apr 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

Uh oh!

jrgemignani commented Apr 27, 2026

Summary

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants