Skip to content

Fix backend crash in age_create_barbell_graph with a null node label - #2521

Open
NotHimmel wants to merge 1 commit into
apache:masterfrom
NotHimmel:fix/barbell-null-node-label
Open

Fix backend crash in age_create_barbell_graph with a null node label#2521
NotHimmel wants to merge 1 commit into
apache:masterfrom
NotHimmel:fix/barbell-null-node-label

Conversation

@NotHimmel

Copy link
Copy Markdown

Fixes #2519

Problem

node_label is declared name = NULL in the SQL signature of
ag_catalog.age_create_barbell_graph(), so leaving it out — or passing NULL
explicitly — is a supported call. Both crashed the backend with SIGSEGV, which
makes the postmaster reinitialize and drops every other session on the
instance.

SELECT create_graph('barbell_test');
SELECT age_create_barbell_graph('barbell_test', 5, 0);
-- server closed the connection unexpectedly
LOG:  client backend (PID 312566) was terminated by signal 11: Segmentation fault
LOG:  all server processes terminated; reinitializing

Root cause

There are two separate faults on the null-node_label path.

1. The default label was copied into a null pointer.

Name node_label_name = NULL;
...
if (PG_ARGISNULL(3))
    namestrcpy(node_label_name, AG_DEFAULT_LABEL_VERTEX);

namestrcpy() writes through the pointer it is given and does not allocate, so
this writes to address 0. The fix gives the default its own NameData. As a
side effect the default now actually takes effect, which it never did.

Note that the immediate segfault only happens on PostgreSQL 14 and later.
namestrcpy() used to start with if (!name || !str) return -1;, removed by
PostgreSQL commit 1784f278a638 ("Replace remaining StrNCpy() by strlcpy()",
first released in 14). On PG 13 and earlier the call silently did nothing, so
node_label_str simply ended up NULL and the failure moved downstream.

2. The node label was forwarded as the raw argument Datum.

DirectFunctionCall4(create_complete_graph, ..., arguments->args[3].value);

DirectFunctionCall4() marks every argument as not null, so a null node label
reached create_complete_graph() as a non-null NULL pointer and was
dereferenced by its vertex/edge label comparison. Fixing only the namestrcpy()
call is therefore not enough — verified: with just that change,
age_create_barbell_graph('g', 5, 0, NULL, NULL, 'E') still segfaults. The
resolved label is forwarded instead.

create_complete_graph() already handles its own null node label correctly;
this change makes the barbell function follow the same approach.

How the bug was introduced

By the original barbell implementation, 0c79370 ("Barbell graph generation",
#648, 2023-02-17). Present on master, PG16, PG17, PG18 and PG19, so
the fix likely wants backporting to those branches.

Testing

Extended regress/sql/graph_generation.sql with the previously untested cases.
The existing barbell tests always passed a node label, except for the
all-arguments-null case, which errors out on the graph name before ever
reaching this code — which is why this was never caught.

SELECT * FROM age_create_barbell_graph('gp7',5,0,NULL,NULL,'edges',NULL);
SELECT COUNT(*) FROM gp7."_ag_label_vertex";
SELECT COUNT(*) FROM gp7."edges";
SELECT * FROM cypher('gp7', $$MATCH (a)-[e]->(b) RETURN e$$) as (n agtype);

-- SHOULD FAIL, but with an error rather than a crash
SELECT * FROM age_create_barbell_graph('gp8',5,0);

On PostgreSQL 18.4, built from source:

  • Without the code change, graph_generation fails with
    server closed the connection unexpectedly, and because the crash takes the
    temporary instance down with it, the ten tests that follow fail as well.
  • With the code change, # All 43 tests passed.

The new cases also confirm the default label is applied: gp7 gets 10 vertices
under _ag_label_vertex (two K5s) and 21 edges (2 × 10 plus the bridge).

Out of scope

Three other defects in the same function, left alone to keep this change
focused. Happy to open separate issues:

  • if (PG_ARGISNULL(1) && PG_GETARG_INT32(1) < 3) should use ||. As written,
    graph_size of 1 or 2 passes the check silently.
  • node_properties and edge_properties are accepted but ignored;
    properties is hardcoded to create_empty_agtype().
  • bridge_size is validated but unused, as the in-code comment notes.

node_label is declared "name = NULL" in the SQL signature, so leaving it
out - or passing NULL explicitly - is a supported call. Both crashed the
backend with SIGSEGV, which makes the postmaster reinitialize and drops
every other session on the instance.

There were two separate faults on that path.

First, the default label was copied into a null pointer:

    Name node_label_name = NULL;
    ...
    if (PG_ARGISNULL(3))
        namestrcpy(node_label_name, AG_DEFAULT_LABEL_VERTEX);

namestrcpy() writes through the pointer it is given and does not
allocate, so give the default its own NameData. This also makes the
default actually take effect, which it never did.

Second, the node label was forwarded to create_complete_graph() as
args[3].value. DirectFunctionCall4() marks every argument as not null, so
a null node label arrived there as a non-null NULL pointer and was
dereferenced by the vertex/edge label comparison. Forward the resolved
label instead.

create_complete_graph() already handles its own null node label correctly;
follow the same approach here.

Extend the graph_generation test with the previously untested cases. The
existing barbell tests always passed a node label, except for the
all-arguments-null case which errors out on the graph name before ever
reaching this code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

age_create_barbell_graph() crashes the backend (SIGSEGV) when node_label is left at its default

1 participant