Memory safety fixes - #43
Merged
Merged
Conversation
Two things stop pljs compiling against the REL_19_STABLE branch that .github/workflows/build_and_test.yml already tests: * time_as_8601() calls strftime() and gmtime(), whose declarations arrived through a transitive include that PostgreSQL 19 no longer provides. src/types.c includes <string.h> but never <time.h>. * pljs_datum_to_jsvalue_fallback() passes a Datum straight to VARDATA() and VARSIZE_ANY_EXHDR(), which take a pointer. That conversion is no longer implicit, so the Datum is unwrapped with DatumGetPointer() into a struct varlena * first. Whether this is an error or a warning depends on the compiler, which is why CI has stayed green: gcc on the runner image still warns, while clang treats both as errors and stops. Recent gcc does the same, so the CI leg is one image update away from failing too. Nothing about the behaviour changes -- the values passed to VARDATA() and VARSIZE_ANY_EXHDR() are the same bytes as before, just with the type spelled out. Detoasting is unchanged and is a separate question: this function still assumes its argument is already detoasted, as it did before. Verified: builds clean on PostgreSQL 16, 17, 18 and 19beta3, and the full suite passes on all four. No regression test accompanies this. What it fixes is a compile failure, so the evidence that it works is that the tree builds -- which the CI matrix already checks on every branch it tests. There is no SQL-level observable either way.
The BYTEAOID conversion iterated to bytes_per_element * length while indexing the typed array by element and writing array_copy[i], where array_copy holds length elements. Converting an Int32Array therefore wrote four times past the end of the allocation, and an Int16Array twice. SET_VARSIZE used the correct byte count, so the returned bytea was right and the overrun was pure collateral damage past the end of a palloc'd chunk. That is the worst shape for such a bug: nothing fails at the point of the error, the damage depends on what the allocator had placed after the chunk, and it surfaces later as an unrelated crash. Adds sql/pg_typedarray_views.sql, which converts every typed-array width at lengths spanning several allocation chunks, including offset views and a zero-length view.
The cached function body was copied two different wrong ways. pljs_context_to_function_cache() allocated strlen + 1 bytes but copied only strlen of them. palloc does not zero, so the final byte was uninitialised and the cached string had no terminator. pljs_function_cache_to_context() then copied a fixed NAMEDATALEN bytes out of that allocation. For any function body shorter than 63 characters that reads past the end of the chunk, and it is what then ran off the end of the unterminated string above. Both sides now use pstrdup, which sizes and terminates in one step. There is no regression test, and one cannot be written at this level: the over-read touches memory the process already owns, so no value changes and nothing fails. It takes a sanitizer or a clobbering allocator to observe. No regression test accompanies this. The over-read changes no query output -- the truncated copy is never compiled -- so nothing at the SQL level can distinguish the two versions. It shows up as a heap-buffer-overflow under a sanitizer build, which `make installcheck` has no way to express.
Returning from inside a PG_TRY block leaves PG_exception_stack pointing at a sigjmp_buf in a stack frame that has already been unwound. The next ereport(ERROR) then siglongjmp()s into that dead frame, which terminates the backend. pljs_find_function did exactly this on the permission-denied path. It now leaves func as JS_UNDEFINED and falls through to the shared return, so the PG_TRY is left by its own PG_END_TRY. Adds sql/pg_find_function_no_perm.sql, which drives the crash sequence: look up a function the caller lacks EXECUTE on, then force an unwrapped elog(ERROR) by returning a non-array value for an int[] column.
pljs_datum_to_jsvalue_fallback() read variable-length datums with VARDATA(), which assumes a 4-byte header, while sizing them with VARSIZE_ANY_EXHDR(), which does not. For any value carrying a 1-byte short header -- what PostgreSQL stores for a small value in a column whose type is not PLAIN -- the two disagree: the read started three bytes into the payload and ran three bytes past its end. Nothing detoasted the datum either, so a compressed or out-of-line value was handed to JavaScript as its raw stored bytes. Every type not named in the switch above reaches this path: domains, ltree and other extension types, inet, tsvector, range types. A literal was fine, because one is built with a 4-byte header; the same value read back out of a column was not. With ltree the corrupted level count is enough to take the backend down. PG_DETOAST_DATUM_PACKED() plus VARDATA_ANY() is the accessor pair that matches VARSIZE_ANY_EXHDR(). The copy detoasting may allocate is freed here, so the function still allocates nothing in the common case, as before. The regression test extends the existing ltree coverage in types.sql with a table-backed value, and adds a domain over text exercising all three shapes: a literal, a short-header column value, and a 5000-byte value stored compressed.
As written the test passed on an unfixed tree. Its longest case was three elements, and at that size palloc rounds the request up to a chunk with enough slack to absorb the overrun, so the output stayed correct and the corruption was invisible -- which is exactly what the fix's own commit message says about the bug. sql/bytea.sql already returned an Int32Array(4) and passed for the same reason. ta_big() returns a 262144-element Uint32Array. Past palloc's 8KB chunk limit the allocation gets its own malloc'd block sized to the request, so the old loop wrote 4MiB into 1MiB and ran off the end of the mapping: on an unfixed build this segfaults the backend, and the test fails. ta_nil() adds the zero-length view the original commit message claimed was covered. The cursor.fetch.apply case moves to sql/cursor.sql, where it belongs; it has nothing to do with typed arrays or with this fix.
The test creates a role, and roles are cluster-wide. It is also built to kill the backend when the fix it guards is absent -- and when it does, the DROP ROLE at the bottom never runs. Every later run then failed on 'role "ff_limited" already exists', which is precisely the revert-and-re-run loop anyone verifying the fix will be in. Dropping the role up front makes the test idempotent; the notice is silenced so the output does not depend on whether it was there. Also drops the CREATE EXTENSION IF NOT EXISTS: no test file in this suite creates the extension except init-extension, which REGRESS runs first, and baking the resulting "already exists, skipping" notice into the expected output encoded the run order for no benefit.
PG_CATCH() restores PG_exception_stack, but it does not pop the errordata
stack -- only FlushErrorState() does. Eleven PG_CATCH() blocks in functions.c
returned a thrown JavaScript value without flushing, so each error caught
through them consumed one of the five slots ERRORDATA_STACK_SIZE provides, for
the life of the session.
The fifth such error raised "ERRORDATA_STACK_SIZE exceeded" at PANIC level,
which takes down every backend in the cluster, not just the one running the
query. Reaching it needs no privileges and nothing unusual:
for (let i = 0; i < 5; i++) {
try { pljs.execute('SELECT * FROM missing'); } catch (e) { }
}
pljs.execute(), plan.execute(), pljs.prepare(), cursor open, cursor fetch and
move, cursor close, pljs.commit(), pljs.rollback(), pljs.find_function() and the
window storage accessor were all affected. The block in pljs_elog() already had
the right shape and is left alone; the blocks that end in PG_RE_THROW() are
correct as they are, since the error stays live for an outer handler.
Where the message is copied out with CopyErrorData(), the flush goes between the
copy and the subtransaction rollback, matching plpgsql's exec_stmt_block(): the
copy is made in the caller's context so it survives the reset of ErrorContext,
and the rollback then runs with a clean error state. Those sites also free the
copy, which they previously leaked.
sql/pg_caught_error_stack.sql drives six of the eleven sites twenty times each
in a single session -- the stack is per-backend, so spreading them across
sessions would hide the bug. Cursor open is reachable too but reports an
unrelated syscache leak whose message names a tuple id, so it is left out.
This was referenced Aug 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
extends #25 to fix additional issues found while testing this PR.