Crash fix: copy the error message out of ErrorContext before taking it - #61
Conversation
queue_item_note_error() called CopyErrorData() from a PG_CATCH() body, which is the one place it may not run: errstart() switches to ErrorContext and an error longjmps out without switching back, so the copy lands in the context FlushErrorState() is about to reset. An assert-enabled build aborts outright on the assertion CopyErrorData() carries for this; a release build frees the copy under itself and gets away with it only because the message is taken first. Switch away for the duration of the copy. test/t/004 goes from killing the postmaster before its first assertion to passing all seven, and its fifth - that the recorded reason is the error itself - is reached for the first time.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The comment named errstart() where it is errfinish() that switches to ErrorContext and then deliberately leaves CurrentMemoryContext set there when it re-throws, and it claimed FlushErrorState() frees the copy out from under us, when FreeErrorData() frees it explicitly a few lines earlier. The real cost on a release build is milder and worth stating accurately: the copy eats into the space ErrorContext reserves so that reporting an error can never itself fail, which is precisely what the assertion in CopyErrorData() is protecting.
dpage
left a comment
dpage
left a comment
There was a problem hiding this comment.
I reviewed this against the PostgreSQL sources rather than taking the description on trust, and the diagnosis holds: CopyErrorData() has carried Assert(CurrentMemoryContext != ErrorContext) since 2003, so it is present across the whole 14 to 18 range, and because queue_item_note_error() is the first statement of the PG_CATCH() body nothing has had a chance to switch away first. The assertion failure was certain rather than merely possible. Switching to TopMemoryContext is the right fix and is the idiom core itself uses in autovacuum.c; the MemoryContextSwitchTo() pair is balanced on every path, including the early return, and FreeErrorData() runs before the switch back, so there is neither a leak nor a double free. TopMemoryContext is also a better choice here than capturing a pre-PG_TRY() context, since the latter could be a transaction context that the imminent abort deletes.
I also checked that the fix is complete. CopyErrorData() appears exactly once in the tree, and of the other four PG_CATCH() bodies, two allocate nothing in the current context and the two in bm25.c already switch to a saved context as their first action, so there is no second instance of this bug.
One correction, which I have pushed directly rather than making you round-trip for it (7c046ea): the justification comment had the mechanism wrong in two places. It named errstart(), but that function only records edata->assoc_context and never switches; it is errfinish() that does MemoryContextSwitchTo(ErrorContext) and then deliberately leaves it set that way before PG_RE_THROW(), with a comment of its own saying as much. The comment also said the copy is "freed under it by FlushErrorState()", which does not happen: FreeErrorData() frees it explicitly, well before the caller reaches FlushErrorState(). The actual release-build cost is milder and more interesting, namely that the copy eats into the headroom ErrorContext reserves so that reporting an error can never itself fail, which is exactly what the assertion protects. The PR description reads the same way, so it is worth amending for the record.
Two optional points I have deliberately not changed. The new declarations put both identifiers at column 20, whereas the rest of worker.c is pgindent-canonical with identifiers at column 16, so the next pgindent run will produce a spurious diff here; and there is a case for doing the context switch once at the top of the PG_CATCH() body rather than locally, matching what bm25.c already does, which would let this function drop the switch entirely. Neither is worth holding a crash fix for.
Finally, two things worth knowing that are not objections to this PR:
The green tick here neither demonstrates the fix nor guards against a regression, because CI installs release packages from apt.postgresql.org with USE_ASSERT_CHECKING off, so the assertion can never fire in CI. test/t/004 does exercise the path and was already passing on the merge base. A cassert-enabled leg in the matrix would make this whole class of bug visible, and I think that is worth doing as separate work.
Whilst reading the surrounding code I noticed a pre-existing bug a few lines below, from 9ab3625 rather than from this change: strlcpy() clips the message at 1023 bytes without regard to character boundaries, so a longer message cut inside a multi-byte UTF-8 sequence leaves an invalid byte sequence in failed_item_error. That then goes through quote_literal_cstr() into the UPDATE in queue_item_record_failure(), which raises "invalid byte sequence for encoding UTF8"; that error is swallowed, so the attempt is never charged and the row is reclaimed on the next poll, which is precisely the loop test 004 exists to rule out. pg_mbcliplen() would close it. I will raise that separately.
Approving and merging. Nice catch on this one.
2 participants
queue_item_note_error() called CopyErrorData() from a PG_CATCH() body, which is the one place it may not run: errstart() switches to ErrorContext and an error longjmps out without switching back, so the copy lands in the context FlushErrorState() is about to reset. An assert-enabled build aborts outright on the assertion CopyErrorData() carries for this; a release build frees the copy under itself and gets away with it only because the message is taken first.
Switch away for the duration of the copy. test/t/004 goes from killing the postmaster before its first assertion to passing all seven, and its fifth - that the recorded reason is the error itself - is reached for the first time.