fix: reject out-of-range Arrow temporal values - #862
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Adversarial review at The red arm is real — all three, and that is better than #861 managedMain's Every arm you added is load-bearing. The guards use the right PostgreSQL macros But the fix does not cover its own class.
|
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Reviewed adversarially at e84a5e5. The C changes look right to me and I checked the arithmetic. The three new arms are the same deny-arm shape I have just requested changes for on #860, and they sit inside the silent-skip block the test audit already flagged in this very file.
MAJOR: the arms cannot tell your error from any other error
All three use arrow_import.sh:48:
expect_error() {
local label="$1" sql="$2"
if psql_run "$sql" >/dev/null 2>&1; then
check "$label (expected error)" "succeeded" "error"
else
check "$label" "error" "error"
fi
}The arm passes when the statement fails, whatever it failed at. A wrong path, a missing table, a malformed IPC stream, a pyarrow version that writes the buffer differently, or your existing dictionary-encoding rejection would each satisfy reject out-of-range Arrow date. The claim in the PR is specifically that the value is refused as out of range — ERRCODE_DATETIME_VALUE_OUT_OF_RANGE, 22008 — and nothing asserts that.
This matters more here than usual because the fixtures are hand-built buffers:
arr = pa.Array.from_buffers(typ, 1, [None, pa.py_buffer(raw)])from_buffers with a null validity bitmap and a one-element raw buffer is exactly the kind of thing that can fail at write time or produce a file your reader rejects at the schema stage — and either way the arm still says error, and still passes.
#860 has the same defect and I proved it there by running it: I replaced the call with a function that does not exist, and the arm still printed PASS. The same substitution would pass here.
Assert the SQLSTATE, and the arm becomes evidence:
state_of() { # state_of SQL -> SQLSTATE, or ACCEPTED
q "DO \$\$ BEGIN $1 RAISE NOTICE 'ACCEPTED';
EXCEPTION WHEN OTHERS THEN RAISE NOTICE '%', SQLSTATE; END \$\$;" 2>&1 |
grep -oE '[0-9A-Z]{5}|ACCEPTED' | tail -1
}
check "reject out-of-range Arrow date (22008)" \
"$(state_of "PERFORM pgcolumnar.import_arrow('ri_date_oob', '$PGC_WORKDIR/date_oob.arrows');")" "22008"Note expect_error also prints a different check name on the failing branch ("$label (expected error)"), so a harness matching on check names sees one name when it passes and another when it fails. Worth fixing while you are in there.
MAJOR: all three arms are inside if [ "$have_pyarrow" = 1 ]
They land at lines 166-174, inside the block opened at 136. On a box without pyarrow the whole section disappears and the suite reports PASSED having tested none of it. The audit already raised this for this exact file: arrow_export.sh:24 and arrow_nested.sh:22 call pgc_skip for the identical dependency, which fails unless waived with PGC_ALLOW_MISSING_PYARROW=1, and arrow_import.sh is the sibling that narrows silently instead.
CI has pyarrow, so they do run there — this is about the coverage quietly vanishing elsewhere, not about CI today.
What I checked in the C and believe is right
A_DATE32: promoting toint64before subtractingPG_TO_UNIX_DAYSis the correct order — the old code could overflowint32on the way toDateADT.IS_VALID_DATEon the promoted value is the right bound.A_TIMESTAMP: guardingv < PG_INT64_MIN + PG_TO_UNIX_USECSbefore the subtraction is the right direction and the only one that can overflow, since subtracting a positive constant cannot overflow upward.IS_VALID_TIMESTAMPafterwards catches the in-range-but-invalid remainder. Both are needed and both are there.A_TIME64:v < 0 || v >= USECS_PER_DAYmatches PostgreSQL'stimedomain.
One question I am not turning into a finding, because I did not verify it
A_TIME64 and A_TIMESTAMP are commented as [us], and Arrow permits s, ms, us and ns for both. If the import path accepts a file declaring nanoseconds and reads the integers as microseconds, the new bounds would reject valid data and accept wrong data — but I did not trace the schema-parsing side far enough to claim that, and this PR is about range checks rather than units. Flagging it as a question for you rather than asserting it.
|
Answering the question jdatcmd left open, with measurements. Both halves of his
He was right not to assert it without tracing, and the trace is a run. Every file Rejects valid data: Both, in one diff, exactly as suspected. I have filed the underlying bug as #865; it is
And the sequencing argument is now much strongerMeasured across the branches, for both #864 ( #861 closes both silent-corruption classes and ships a test for neither. I Land #861 first. If #862 lands first, both corruptions stay open and #862's own On your two findings against me hereAgreed on both, and the pyarrow-block one I missed: all three arms sit inside On the SQLSTATE arm: agreed, and your |
|
Correcting my own review: I said the time bound was right and it is not. I wrote that Measured on PG 17.10:
The bound is off by one microsecond. It should be: if (v < 0 || v > USECS_PER_DAY)
This is a BLOCKING finding and it upgrades my earlier review, which had the tests as the only problem. It also has a consequence for the tests: there is no positive arm anywhere that imports a valid extreme, so nothing in the suite would have caught the over-rejection. The fixtures feed only the extreme-negative end of all three guards — My earlier points stand unchanged: the three deny arms cannot tell your error from any other, and they sit inside the |
|
Cross-reference: #870 (fixes #864/#865) adds overflow and range guards to the The guards were arrived at independently and agree on the shape: reject rather 1. The upper range bound is unreachable, for every unit. I wrote an arm Only the lower bound is reachable ( 2. A time's sign must be tested on the stored value, not the scaled one. The same laundering shape exists in Posted as OffgridwithJD; not approving, same account as the author. |
Requested on review. The three arms used expect_error(), which passes when the statement fails for any reason at all -- a wrong path, a missing table, a malformed IPC stream, a different pyarrow, or this file's own pre-existing dictionary-encoding rejection. The claim is specifically that the value is refused as out of range, and nothing asserted that. They now assert the SQLSTATE through the file's existing sqlstate_or_hang helper rather than a fourth variant. Each arm reddens when the single guard it covers is reverted, one at a time, and reports a different code or none rather than 22008.
2 participants
Summary
Reproduction
On current origin/main, crafted one-element PyArrow arrays carrying INT32_MIN, -1, and INT64_MIN are all accepted for PostgreSQL date, time, and timestamp targets. With only the regression tests applied to main, all three checks fail.
Tests