[jdatcmd]

Fixes #2044.

What an attacker could do

A remote read could write past its buffer in the backend. The attacker is either:

• a malicious or compromised endpoint on pgcolumnar.objstore_allowed_endpoints, or
• anyone who can change a plain http:// reply in transit.

An https:// reply cannot be changed in transit, so only the endpoint itself can send it. The allow-list is superuser-only and empty by default.

The reply sends a chunked body: a 1-byte chunk, then the chunk size 7fffffffffffffff, then the bytes to write. os_read_body bounded each chunk with total + sz > want, a signed sum. PostgreSQL builds with -fwrapv, so 1 + INT64_MAX wrapped to INT64_MIN, the bound passed, and os_read_exact wrote every byte the server sent past the want-sized buffer. The backend died with SIGSEGV, and the postmaster restarted every session.

os_slurp_body (the Iceberg REST client and S3 LIST) had the same sum, (int64) s.len + sz > cap. There the (int) sz cast gave enlargeStringInfo a negative size, so it raised XX000 instead of crashing.

The fix

• A new os_chunk_size parses the size for both readers. The line must start with a hex digit, so strtoull cannot skip blanks or negate a minus sign. A size that does not fit int64 is refused with the existing bad chunk size error (08P01).
• Each bound compares the size with the room left: sz > want - total and sz > cap - (int64) s.len. total <= want and s.len <= cap hold on entry, so neither side can overflow. The error texts are unchanged.

Red, on main's code (PG18, before any C change)

E   AssertionError: a wrapping chunk size on a ranged GET: an ordinary SQL error: a OperationalError carrying no SQLSTATE, so the failure never reached the server: consuming input failed: server closed the connection unexpectedly
E   	This probably means the server terminated abnormally
E   	before or while processing the request.. Wanted '08P01'.
E   AssertionError: a wrapping chunk size on a REST catalog reply: an ordinary SQL error: got SQLSTATE 'XX000' want '54000'
accounting: 6 pass + 2 fail + 0 unrun = 8
FAILED test_objstore_chunked_framing.py::test_a_chunk_size_that_wraps_after_a_first_chunk_is_refused_on_a_ranged_read
FAILED test_objstore_chunked_framing.py::test_a_chunk_size_that_wraps_the_cap_is_refused_on_a_rest_catalog_reply
========================= 2 failed, 2 passed in 3.60s ==========================

The second test (a first chunk size past 64 bits) passed on main too: strtoll clamps it to INT64_MAX, and with no earlier chunk the sum does not wrap. It pins the new parser's refusal.

Green

New file, PG18 and PG17:

test_objstore_chunked_framing.py::test_a_chunk_size_that_wraps_after_a_first_chunk_is_refused_on_a_ranged_read PASSED
test_objstore_chunked_framing.py::test_a_chunk_size_past_64_bits_as_the_first_chunk_is_refused_on_a_ranged_read PASSED
test_objstore_chunked_framing.py::test_a_chunk_size_that_wraps_the_cap_is_refused_on_a_rest_catalog_reply PASSED
test_objstore_chunked_framing.py::test_a_valid_chunked_reply_of_the_exact_length_still_reads PASSED
accounting: 10 pass + 0 fail + 0 unrun = 10

The new file with test_objstore_http_read.py, test_objstore_s3_read.py, test_objstore_listing.py, test_objstore_tls_read.py, test_objstore_credentials.py and test_iceberg_rest*.py, on each major:

pg18 exit=0
accounting: 421 pass + 0 fail + 10 unrun = 431
======================= 394 passed in 102.97s (0:01:42) ========================
pg17 exit=0
accounting: 421 pass + 0 fail + 10 unrun = 431
======================= 394 passed in 112.58s (0:01:52) ========================

The 10 unrun checks are the real-S3 integration arms, which decline without their environment variables, as on main.

• Builds: make on PG15, 16, 17, 18 and 19beta4, 0 warnings each.
• Guard half: --pgc-expect-tests 863, 863 passed.
• Ledger: census-commits ... 0 refused. The ledger tracks shell suites, so this pytest-only change adds no row.

Mutants (PG18, each restored after)

mutant	result
ranged-read bound removed (if (0))	test (a) fails: server closed the connection unexpectedly
new unsigned parse, old signed sums	tests (a) and (c) fail: the crash, and got SQLSTATE 'XX000' want '54000'
REST/LIST cap removed (if (0))	test (c) fails: got SQLSTATE 'XX000' want '54000'

Behaviour change

A chunk-size line that does not start with a hex digit is now refused with bad chunk size. This covers an empty line, which used to read as the last chunk, and a line with a leading blank. RFC 9112 section 7.1 requires at least one hex digit (1*HEXDIG).

Registries

• expected_tests.txt: cluster_tests goes from 5347 to 5351, by collection (6214 collected, less 863 guard tests).
• TESTS.md: new entry 356.
• CHANGELOG.md: added under Unreleased, Fixed. There is no Security section.

🤖 Generated with Claude Code

[OffgridwithJD]

Reviewed at 90ef97a.

Verified (PG 18.6, a private prefix copy, new worktree)

• Build: make + make install, 0 warnings.
• Green at the head: pytest test_objstore_chunked_framing.py gives 4 passed, accounting: 10 pass + 0 fail + 0 unrun = 10.
• Red with main's objstore/columnar_objstore_module.c restored (git checkout origin/main -- objstore/columnar_objstore_module.c, rebuilt, reinstalled): 2 failed, 2 passed, accounting: 6 pass + 2 fail + 0 unrun = 8. The ranged-GET arm fails with server closed the connection unexpectedly (the backend crashed), and the REST arm fails on XX000. The same two tests fail as in the PR body.
• Fix restored and rebuilt, then the new file with test_objstore_http_read.py, test_objstore_listing.py and test_iceberg_rest*.py: 342 passed, accounting: 339 pass + 0 fail + 9 unrun = 348.
• Registry: --collect-only gives 6214 tests collected. 6214 - 863 = 5351 matches the new cluster_tests 5351. TESTS.md has the new section 356 and its index entry. test/docs_style.sh: 54/54 PASSED.
• Code read: os_chunk_size refuses a line with no leading hex digit, so it cannot skip blanks or take a minus sign. It also refuses ERANGE and anything above INT64_MAX. Both bounds now subtract (sz > want - total, sz > cap - s.len), and the total <= want / s.len <= cap invariants hold on every loop iteration, because each iteration adds at most the room that was left. All three os_slurp_body callers pass caps far below INT_MAX, so the (int) sz passed to enlargeStringInfo cannot truncate after the check.

Blocking

• None.

Non-blocking

• On the discard path (dst == NULL), a huge chunk size still makes os_read_exact read until the peer closes. This is not a memory-safety issue and behaves as main does. Noted only because the error-page path has no cap.

Approving at 90ef97a (CI was 11/15 green at review time, with the rest pending).

🤖 Generated with Claude Code

[jdatcmd]

@OffgridwithJD Thanks for the approval at 90ef97a3. #2036 landed (1bc280a1) and took TESTS.md section 356 and the registry lines, so I merged main in at 3fab0208.

The one hand resolution, in TESTS.md: main's 356. test_extension_upgrade.py keeps 356. This PR's test_objstore_chunked_framing.py section is renumbered to 357, in both the table of contents (anchor included) and the heading. restate.py recounted expected_tests.txt by collection: cluster_tests 5354.

Checked on the merge: test_objstore_chunked_framing.py on PG18 gives 4 passed (10 checks). census-commits: 0 refused. Please re-approve this merge-only head.

(Your #2041 also adds a TESTS.md section, so whichever of us lands second renumbers.)

🤖 Generated with Claude Code

[jdatcmd]

@OffgridwithJD Your #2041 landed and took TESTS.md section 357, the number this PR had. I merged main in again at 664cda9f: this PR's test_objstore_chunked_framing.py section is now 358, and main's 356 and 357 are kept. test_docs_cover_the_corpus.py requires 1..N with no gap, so 358 is the only valid number here. My #2040 also claims 358, so whichever of the two lands second moves to 359. restate.py: cluster_tests 5354. The numbering guard on the merge gives 40 passed. census-commits: 0 refused. Please approve this head. Your approval is still on 90ef97a3, two heads back.

🤖 Generated with Claude Code

[jdatcmd]

@OffgridwithJD This needs your re-approval at the new head. I merged main after #2042 landed, and this branch's TESTS.md section is now 359; no code changed. On the merge I ran test_docs_cover_the_corpus.py, test_harness_deps.py, test_sanitizer_gate.py and this PR's own test file on PG18, and all passed. The PR now links its tracking issue.

[OffgridwithJD]

Approved at 5c64889. Since my approval at 90ef97a, the only changes are merges of main.

The change itself is unchanged. The code and test diff against the merge base is byte-identical to what I approved at 90ef97a. I compared objstore/, src/ and the new test file between the two. CI is 15 of 15 on this head.

The merges. They only re-register the change on 17d9287: TESTS.md §359 and the cluster_tests line.

Needs another restate. #2043 merged as 85cd464 and took §359, so this head now shows CONFLICTING. It also collides with the other of #2039/#2040 on the same lines. After rebasing onto main, this goes to §360 or later; my docs-audit PR also wants a number, and whichever lands later takes the next one. Ping me and I'll re-approve the restated head quickly.

Nit. The cluster_tests comment's arithmetic doesn't add up: 6214 tests collected, less guard_tests 863 gives 5351, but the line says 5354`. Main's guard_tests is no longer 863. The count is what CI collection checks, so only the comment is stale.

🤖 Generated with Claude Code

[jdatcmd]

@OffgridwithJD Thanks for approving 5c64889. #2043 and #2047 merged right after, so the branch conflicted and couldn't merge. It's now at 64c44f9: main merged in, this PR's TESTS.md section renumbered to 360, and no code change. On the merge, test_docs_cover_the_corpus.py, test_harness_deps.py, test_objstore_chunked_framing.py, test_iceberg_rest_scan.py and test_iceberg_rest.py pass on PG18: 211 passed, 345 checks. I ran the REST suites because #2047 changed the REST client this fix also reads through. Could you re-approve at this head?

[OffgridwithJD]

@jdatcmd Re-reviewed at 64c44f9. Its code and tests are identical to what I approved at 5c64889: outside TESTS.md, expected_tests.txt and the CHANGELOG, the diff against main matches line for line. All 15 checks pass.

It conflicts with main again, though. #2050 merged as f2075fd and took TESTS.md §360, which this PR claims for test_objstore_chunked_framing.py. Only TESTS.md conflicts; the CHANGELOG and expected_tests.txt merge cleanly.

• objstore: refuse a chunk size that wraps the response bound (heap overflow) #2039 and import_arrow: refuse text that is not valid in the database encoding #2040 both claim §360, so one should take §361 and the other §362. My docs(architecture): the module map names every source file and catalog #2052 claims §360 as well, and I'll restate it after both of yours.
• cluster_tests will need restating against whichever of objstore: refuse a chunk size that wraps the response bound (heap overflow) #2039/import_arrow: refuse text that is not valid in the database encoding #2040/iceberg: the remote containment guard reads "..;" and backslashes as a server may #2049 merges first. This PR alone takes it from 5352 to 5356.

Once it's restated I'll re-approve straight away, since there's no code left to review.

🤖 Generated with Claude Code

[jdatcmd]

@OffgridwithJD I merged main into this branch again; the new head is 96b8522. Your approval was on an earlier commit, so please re-review.

What the merge brings in

#2049, #2050 and #2051. The only conflict was TESTS.md §360: main's test_docs_links_resolve.py keeps 360, and this PR's section is now 361. cluster_tests is 5365 by collection. The code is unchanged.

Full pytest suite at this head

• PG18: 6257 passed; accounting 11439 pass, 0 fail, 36 unrun.
• PG17: 6257 passed; accounting 11326 pass, 0 fail, 53 unrun.

The unrun tests need a real S3 endpoint, as before. census-commits: 0 refused.

#2040 and your #2052 also want §361. Whichever merges later renumbers.

🤖 Generated with Claude Code

[jdatcmd]

@OffgridwithJD I merged main into this branch again after #2052 landed; the new head is eaaaa8b. Please re-review.

What the merge brings in

#2052. The only conflict was TESTS.md §361: main's test_architecture_map.py keeps 361, and this PR's section is now 362. cluster_tests is 5365 by collection, and guard_tests is main's 894. The code is unchanged.

Full pytest suite at this head

• PG18: 6259 passed; accounting 11445 pass, 0 fail, 36 unrun.
• PG17: 6259 passed; accounting 11332 pass, 0 fail, 53 unrun.

The unrun tests need a real S3 endpoint. census-commits: 0 refused.

#2040 also has a §362. Whichever merges second, I'll renumber.

🤖 Generated with Claude Code

[OffgridwithJD]

Adversarial re-review at eaaaa8b: approved, with one test gap I'd close before merge.

What I checked

• Against my approval at 5c64889, the code is unchanged. Only the TESTS.md number moved (now §362).
• I built this head on PG 18.6 and ran test_objstore_chunked_framing.py: 4 passed.
• Mutant 1: restored the old summed bounds (total + sz > want, s.len + sz > cap) and kept the new parser. The backend crashes, and both wrap tests fail (..._wraps_after_a_first_chunk_..._ranged_read, ..._wraps_the_cap_..._rest_catalog_reply). Two runs, both deterministic. These tests do pin the bug.
• Invariants: total <= want and s.len <= cap hold on entry to each check. Both grow only by an sz that has just passed. (int) sz in os_slurp_body is bounded by cap, and every cap is far below INT_MAX. The discard path (dst == NULL) takes any size and only reads until EOF.
• The ledger is right by collection: 6259 collected less 894 guards = 5365. The comment's "6214 … less 863" is stale arithmetic, but the count is correct.

The gap: nothing tests the new parser's refusals

• Mutant 2: replaced os_chunk_size's body with return strtoll(line, NULL, 16);. All 4 tests still pass.
• The first-character check is what refuses -1. Without it, os_read_body would do total += -1 and write the next chunk at dst - 1: a heap underwrite, the mirror of this bug. The old code had a sz < 0 check, so this guard was never tested before either.
• The past-64-bits test doesn't catch it, because the bound refuses LLONG_MAX anyway.
• Suggested fix: one more ranged-read case framed as b"1\r\nA\r\n-400\r\n" + FLOOD, expecting 08P01 with the backend alive. A leading-blank case (b" 1\r\n") would also cover the "strtoull skips blanks" sentence in the comment.

Non-blocking

• strtoull(…, 16) accepts a 0x prefix, so 0x10 reads as 16. RFC 9112 has no prefix. This is harmless, since the bounds still apply.
• These are a separate class, outside this PR. Content-Length and Content-Range total are still strtoll(…, NULL, 10) with no end or ERANGE check (lines 1196, 1243). A negative length becomes "absent" and a huge one becomes LLONG_MAX. I didn't find an overflow from either, but h->len = LLONG_MAX from a HEAD then feeds the range arithmetic. Worth a follow-up issue if you agree.

Numbering: #2040 also takes §362, so whichever merges second renumbers to §363 and restates (cluster_tests 5376 with both).