Skip to content

Fix nine ways a pljs function can take down the backend - #34

Open
sfc-gh-okalaci wants to merge 11 commits into
up/02-error-reportingfrom
up/03-crashes
Open

Fix nine ways a pljs function can take down the backend#34
sfc-gh-okalaci wants to merge 11 commits into
up/02-error-reportingfrom
up/03-crashes

Conversation

@sfc-gh-okalaci

@sfc-gh-okalaci sfc-gh-okalaci commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Each of these terminates the connection, and most reproduce from plain SQL. Grouped
because they share a cause — PostgreSQL error handling and QuickJS reference counting
meeting at the same boundary — and because several report through the structured error
object added earlier in this stack.

The ones worth reading first

A composite column that converts to NULL crashes the backend.
pljs_jsvalue_to_datum() signalled a SQL NULL with PG_RETURN_NULL(), which expands to
fcinfo->isnull = true. Every column of a composite is converted through
pljs_jsvalue_to_datums() or pljs_jsvalue_to_record(), and both pass fcinfo == NULL
— they report the null through the is_null argument instead. So the null return wrote
through a null pointer. Reaching it takes nothing exotic: case DATEOID handles only a
JavaScript Date and breaks out of the switch for anything else, so a plain string for
a date column falls through to the trailing PG_RETURN_NULL() — the one commented
"shut up, compiler", which turns out to be an ordinary path.

CREATE TYPE r AS (d date);
CREATE FUNCTION f() RETURNS SETOF r AS $$
  pljs.return_next({ d: 'not-a-date' });
$$ LANGUAGE pljs;
SELECT * FROM f();          -- SIGSEGV, write to 0x1c

0x1c is the offset of FunctionCallInfoBaseData.isnull. The scalar form of the same
conversion has a real fcinfo and quietly returns NULL, which is why this stayed
hidden: RETURNS date looks fine and only the composite form dies.

pljs.find_function() crashes after enough lookups. It returned the compiled
function straight out of the per-user cache. The cache entry is that value's only
owner, but a JSValue returned from a C function belongs to its caller, so QuickJS
decremented a count nobody had incremented. After roughly a thousand lookups the
refcount reaches zero while the entry is still cached, and the next call through the
cache terminates the backend. A plain call to the target mixed in among the lookups
makes it fire sooner. pljs.start_proc leaked the function and the call result on
every context creation for the same reason.

A trigger could not use SPI at all. call_function() and call_srf_function()
connect to SPI; call_trigger() never did. So not only DDL but a bare
pljs.execute("SELECT 1") failed inside a trigger — most of what a trigger is for.
Nothing in the suite covered it, because the existing trigger tests only inspect
NEW/OLD and the TG_* variables.

The rest

A stale SPI_tuptable reused after pljs.commit(); a syscache pin held past the point
it was needed and leaked on one branch; a pg_language tuple read through
Form_pg_database (harmless only because both catalogs begin with an Oid at the same
offset); a PostgreSQL error raised inside pljs.return_next unwinding past the interpreter
instead of arriving as a catchable JavaScript exception, the way one from
pljs.execute() always has; a cursor error tearing down the whole SPI connection instead of the
statement; and the validator leaking its compiled function and context.

Commits

  • Free the SPI tuptable after converting results
  • Guard cursor fetch, move and close with an internal subtransaction
  • Release the pg_proc pin on every branch
  • Flush the error state in every PG_CATCH that reports to JavaScript
  • Free the compiled function and context in the validator
  • Free QuickJS state before dropping the cache's memory
  • Read pg_language through its own struct and release the pin
  • Do not let a PostgreSQL error longjmp out of pljs.return_next
  • Take a reference before handing a cached function to JavaScript
  • Connect to SPI in call_trigger, so a trigger can query
  • Do not write through a null fcinfo when a composite column is NULL

Every commit in this series builds from clean and passes the full ordered suite on its
own, verified per commit on PostgreSQL 17. The tip is additionally green on PostgreSQL
16, 17, 18 and 19beta3 — the versions this repository's CI matrix builds — with
pljs.memory_limit=64, and under AddressSanitizer with no reports.

@sfc-gh-okalaci sfc-gh-okalaci changed the title Fix eight ways a pljs function can take down the backend Fix nine ways a pljs function can take down the backend Aug 18, 2026
pljs_plan_execute() freed its SPI tuptable once the results were converted;
pljs_execute(), which backs pljs.execute(), did not.  After a pljs.commit() resets
SPI's internal state, the next pljs.execute() could reuse or free the stale
SPI_tuptable pointer -- memory the commit had already released -- and operate on freed
memory, terminating the backend.

Adds sql/pg_spi_freetuptable.sql.  It interleaves result-returning executes with
commits many times and asserts every batch still sees its full row count.  It is
coverage rather than a discriminating regression test: whether a plain SELECT notices a
dangling pointer depends on what the allocator did with the freed chunk, so a test that
reliably fails on the old code cannot be written at this level.
An error inside cursor_fetch(), cursor_move() or cursor_close() was handled by calling
SPI_rollback() followed by SPI_finish(), which tears down the entire SPI connection for
the whole call rather than undoing the failed statement.  Everything the function did
afterwards then operated without SPI.

Each now runs inside BeginInternalSubTransaction() with the error path releasing that
subtransaction and reporting through the JavaScript error object, so a failed fetch
leaves the surrounding function able to continue.

Adds sql/pg_cursor_error_recovery.sql.
pljs_call_handler() holds a syscache pin on the function's pg_proc row while it sets up
and compiles.  One branch returned without releasing it: a function whose body fails to
compile leaked the pin for the rest of the transaction.

Under USE_ASSERT_CHECKING that surfaces as "WARNING: resource was not closed: cache
pg_proc has count 1" at transaction end; without it the pin simply blocks concurrent
DDL for longer than intended.  Each branch now releases before it leaves.

No regression test accompanies this. The released tuple stays readable until its cache entry is actually evicted, so the use-after-free needs an eviction between the release and the read -- a cache-clobbering build (debug_discard_caches, which requires DISCARD_CACHES_ENABLED at configure time), not something a SQL script can arrange.
Several PG_CATCH blocks copied the ErrorData and threw a JavaScript exception without
calling FlushErrorState(), leaving the caught error on the errordata stack.  The next
ereport in the same transaction then stacked on top of it, and in the worst case the
stack grew until it hit its limit and the backend PANICked.

Every such block now copies, releases the subtransaction, and flushes, in that order --
which is the order PL/pgSQL uses, and is safe because the copy has already moved what
is needed into our own memory context.

Adds sql/pg_errordata_stack.sql, which drives many caught errors in one transaction and
asserts the backend survives with correct results.
pljs_call_validator() created a JSContext, compiled the function body into a JSValue to
check it, and then freed neither.  JS_FreeContext() will not free a context that still
has live references into it, so every CREATE FUNCTION leaked the context as well --
around 68kB per statement, on the QuickJS heap where PostgreSQL's memory accounting
cannot see it.

Both the value and the context are now released, on the rejection path as well as the
success path.  The rejection path matters more: the error report does not return, so
anything freed after it is never freed at all.

No regression test accompanies this. The leak is on the QuickJS heap, which pg_backend_memory_contexts cannot see, and the only SQL-level observable is exhaustion of pljs.memory_limit -- which at this point in the series is still read once at load time, so a test cannot lower it. It is measurable with a DDL loop against a backend started with a low limit.
pljs_cache_reset() deleted the cache's memory context while leaving every per-user
JSContext and every compiled function alive on the QuickJS heap.  Those allocations
outlived the only pointers to them, so each reset orphaned the whole set -- invisible
to pg_backend_memory_contexts, which does not see the QuickJS heap at all.

The reset now releases the compiled function references and each JSContext before the
memory context goes away.

No regression test accompanies this. As above: the state being freed is on the QuickJS heap, invisible to PostgreSQL's memory accounting, and the SQL-visible consequence needs a bounded pljs.memory_limit that this point in the series cannot yet set from a session.
pljs_find_function() fetched a pg_language tuple and read it through
Form_pg_database.  That produced the right answer only by accident: both catalogs begin
with an Oid at the same offset, so the one field being read happened to line up.  Any
further field access, or a change to either catalog's layout, would have read the wrong
bytes.  It now uses Form_pg_language.

The same function released its pg_proc pin only on the cache-miss branch, so a
pljs.find_function() that hit the cache -- the common case once a function has been
called once -- leaked the pin for the rest of the transaction, accumulating one per
lookup.

No regression test accompanies this. Both halves are unobservable by design: Form_pg_database and Form_pg_language start with an Oid at the same offset, so the corrected cast reads the same bytes; and the early return this adds a release to is reached only when a body yields no function without raising, whereas a syntax error raises and aborts the transaction, which releases the pin anyway. It is a correctness fix for a path that a future catalog change or a future compile failure would otherwise turn into a leak.
pljs.return_next() is a C function that QuickJS calls, so a PostgreSQL error raised
inside it siglongjmps straight through the interpreter.  QuickJS keeps its own stack
frame list, and unwinding past it without telling it leaves that list pointing at
frames that no longer exist.  It also means the error bypasses JavaScript entirely:
a try/catch around the call never runs, which is not how the rest of the API behaves
-- pljs.execute() has always converted PostgreSQL errors into JavaScript exceptions.

The body is split so the work happens in pljs_return_next_internal(), wrapped in a
PG_TRY whose PG_CATCH converts the error into a JavaScript exception.  The error still
reaches the caller: uncaught, it is re-raised once the interpreter has unwound.

Adds sql/pg_return_next_error_frames.sql.  The error it uses is the one an array-typed
column raises when the property it is given is not an array, which needs no other
change to reach.  The assertion that distinguishes the two versions is that a
JavaScript catch around return_next now runs at all.
pljs.find_function() returned the compiled function straight out of the per-user cache.
The cache entry is that value's only owner, but a JSValue returned from a C function
belongs to its caller, so QuickJS decremented a count nobody had incremented.  After
enough lookups the refcount reached zero while the entry was still cached, and the next
call through the cache terminated the backend:

    SELECT ff_loop(1000);
    server closed the connection unexpectedly

A plain call to the target mixed in among the lookups makes it fire sooner, which is
worth knowing when judging whether a workload is exposed -- a lookup-only loop can run
much longer before it dies.

The fix is to hand out a reference we own.  setup_start_proc(), the other consumer, now
releases what it is given and also releases the result of the start_proc call; neither
was released before, so every context creation with pljs.start_proc set leaked both.

Adds sql/pg_find_function_refcount.sql, which interleaves lookups with direct calls,
forces a collection, and looks the function up again while it is being replaced.
call_function() and call_srf_function() connect to SPI before entering JavaScript.
call_trigger() never did, so every SPI entry point failed inside a trigger function --
not only DDL, but a bare

    pljs.execute("SELECT 1");

in a BEFORE INSERT trigger.  Querying from a trigger is much of what a trigger is for,
so a large part of that surface was unusable.  Nothing covered it: the existing trigger
tests inspect NEW, OLD and the TG_* variables and never touch SPI.

SPI_finish() goes before the exception check, as in call_function(), because the report
does not return and anything after it would never run.  The connection is atomic
unconditionally: a trigger has no CallContext, so there is no nonatomic case to honour,
and a trigger must not be able to commit.

Adds sql/pg_trigger_spi.sql, covering a plain query, an insert into another table, a
prepared plan, a cursor, DDL from a trigger, a failing query leaving the session usable,
and a statement-level trigger.
pljs_jsvalue_to_datum() signalled a SQL NULL with PG_RETURN_NULL(), which expands to
`fcinfo->isnull = true; return (Datum) 0`.  That only works where fcinfo is real, and
it is not: every column of a composite is converted through pljs_jsvalue_to_datums()
or pljs_jsvalue_to_record(), both of which pass fcinfo == NULL and take the null flag
through the is_null argument instead.  All four null-returning sites in that function
therefore wrote through a null pointer.

`case DATEOID` handles only a JavaScript Date and breaks out of the switch for
anything else, so a plain string for a date column falls through to the trailing
PG_RETURN_NULL() -- the one commented "shut up, compiler", which turns out to be an
ordinary path.  That makes the crash reachable from unremarkable user JavaScript:

    CREATE TYPE r AS (d date);
    CREATE FUNCTION f() RETURNS SETOF r AS $$
      pljs.return_next({ d: 'not-a-date' });
    $$ LANGUAGE pljs;
    SELECT * FROM f();          -- SIGSEGV, write to 0x1c

0x1c is the offset of FunctionCallInfoBaseData.isnull.  The scalar form of the same
conversion has a real fcinfo and quietly returns NULL, which is why this went
unnoticed: `RETURNS date` looks fine and only the composite form dies.

The null return now goes through a helper that sets whichever of is_null and fcinfo it
was actually given.  Array columns are covered by the same change: the element
conversion is handed the caller's fcinfo, which is NULL there too.

Not addressed here: because the check is Is_Date() rather than a parse, a *valid* date
string in a composite column also becomes NULL instead of being fed to the type's
input function.  That is the pre-existing behaviour of the scalar path as well, and
changing it is a change of semantics rather than a crash fix.  sql/pg_composite_null_datum.sql
records the current result so the change is visible when it is made.
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.

1 participant