Skip to content

Fix segfault in Record.get() with invalid positional argument count#1333

Open
apoorvdarshan wants to merge 1 commit into
MagicStack:masterfrom
apoorvdarshan:fix-record-get-segfault-1328
Open

Fix segfault in Record.get() with invalid positional argument count#1333
apoorvdarshan wants to merge 1 commit into
MagicStack:masterfrom
apoorvdarshan:fix-record-get-segfault-1328

Conversation

@apoorvdarshan

Copy link
Copy Markdown

Summary

asyncpg.Record.get() could crash the interpreter (SEGV) — or, depending on stack contents, silently return the default instead of raising TypeError — when called with an invalid number of positional arguments, e.g. r.get() or r.get("a", 2, 3).

In record_get() (asyncpg/protocol/record/recordobj.c), the wrong-argument-count else branch sets a TypeError via PyErr_Format() but does not return. Execution falls through to:

res = record_item_by_name((ApgRecordObject *)self, key, &val);

with key left uninitialized, which is undefined behavior — a native crash on release builds. The release build also emitted warning: 'key' may be used uninitialized [-Wmaybe-uninitialized].

Fix

Return NULL immediately after setting the argument-count error:

} else {
    PyErr_Format(PyExc_TypeError,
                 "Record.get() expected 1 or 2 arguments, got %zd",
                 nargs);
    return NULL;
}

Tests / Verification

  • Added TestRecord::test_record_get_invalid_args in tests/test_record.py asserting TypeError for r.get(), r.get("a", 2, 3), and r.get(default=2).
  • Confirmed the new test fails on pristine master (AssertionError: TypeError not raised) and passes after the fix.
  • Built the C extension locally (python setup.py build_ext --inplace) and ran the full tests/test_record.py module: 26 passed. The rebuild no longer emits the 'key' may be used uninitialized warning.
  • Verified the issue's reproducer now raises the expected error:
    Record.get() expected 1 or 2 arguments, got 0
    Record.get() expected 1 or 2 arguments, got 3
    
    while valid calls (r.get("a"), r.get("a", default)) are unchanged.

Fixes #1328.

Disclosure: prepared with AI assistance; reviewed and verified locally.

record_get() set a TypeError with PyErr_Format() when given the wrong
number of positional arguments but did not return, falling through to
record_item_by_name() with an uninitialized `key` pointer. Depending on
the stack contents this either crashed the interpreter (SEGV) or silently
returned the default instead of raising TypeError. The release build also
emitted a `'key' may be used uninitialized` warning.

Return NULL immediately after setting the error, and add regression
coverage for the invalid argument-count cases.

Fixes MagicStack#1328.
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.

Record.get() with invalid positional argument count segfaults

1 participant