fix: reject truncated parallel export paths - #863
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Adversarial review at Red arm, run hereMain's Both arms load-bearing, and the second one is the reason this test is better than And you grep for I went looking for siblings and found none. Stating that, because a reviewer will wonder
None of them is a defect after this change, and I checked rather than
Your entry-point check therefore covers every one of them. That is the right
|
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Reviewed adversarially at c01cc98. The guard is the right idea and it is placed correctly — before pexport_prepare_dir, so nothing is created and no _SUCCESS is stamped. Two findings, both measured.
MAJOR: the guard does not cover the longest path this file builds
The probe checks the final part name:
snprintf(pathProbe, sizeof(pathProbe), "%s/part-%04d.parquet", dir, INT_MAX) /* dir + 24 */but columnar_parallel_export.c:330 builds, into a MAXPGPATH buffer, "%s/%s" from dir and a directory entry — and line 326 shows those entries include part-NNNN.parquet.tmp.<pid>:
guard probes dir + 24 "/part-2147483647.parquet"
cleanup scan builds dir + 30 "/part-0000.parquet.tmp.1234567"
dir=995..999 guard passes, the line-330 buffer truncates
So there is a window where the destination is accepted and the cleanup scan silently truncates a path it may then act on. Probing the longest form the file actually constructs closes it.
I checked the sink and it is not at risk, which is worth stating because the comment at line 272 points that way: columnar_sink.c:45 builds the temp name with psprintf, which allocates rather than truncating. The exposure is the fixed buffer at line 330, not the sink.
MAJOR: the fixture's margin is 12 bytes and nothing asserts it
while [ ${#LONG_PARENT} -lt 980 ]; do LONG_PARENT="$LONG_PARENT/$long_piece"; doneMeasured with a real PGC_WORKDIR:
workdir length 27
iterations 8
LONG_DIR length 1012
+ part suffix 1036 (MAXPGPATH 1024)
margin 12 bytes
The loop steps in 121-byte jumps from a base that depends on PGC_WORKDIR, and stops at the first value ≥ 980 — so the final length lands anywhere in 980…1100 depending on how long the temp directory name happens to be. Work it through: a workdir about 20 bytes shorter puts LONG_DIR at 995, the probe at 1019, under the limit — the guard would not fire and both new checks would fail, on a correct tree.
That is the clamped-fixture shape: the arithmetic assumes a range the fixture may not span, and no premise asserts it does. One line fixes it:
check "premise: the destination plus a generated part name exceeds MAXPGPATH" \
"$([ $(( ${#LONG_DIR} + 24 )) -ge 1024 ] && echo yes || echo no)" "yes"Better still, build to a target length arithmetically rather than by a 121-byte loop, so the margin is chosen rather than inherited from mktemp.
MINOR: the arm greps message text rather than the SQLSTATE
grep -qi 'destination is too long' ties the check to the wording. The code raises ERRCODE_PROGRAM_LIMIT_EXCEEDED (54000), which is the stable thing to assert.
This one is not the #860 defect and I want to be fair about the difference: here an unrelated failure makes the grep miss and the check go red, so it cannot pass for the wrong reason. It is a maintenance hazard rather than a false green.
What is right
Rejecting before pexport_prepare_dir is the correct placement — the PR's own argument, that truncation would publish a differently named object and still stamp _SUCCESS over an unreadable export, is the reason the ordering matters. INT_MAX as the index bound is conservative and I would keep it. The second check, that the destination is not created, is a good complement to the first.
|
Both of jdatcmd's findings reproduce here, and I missed both after looking Finding 1 confirmed — and line 330 was in my own sweep outputSo How I missed it: my sibling sweep printed eleven unchecked That is a count claim about a population I did not finish checking — the exact Finding 2 confirmed, and my red arm was luckMeasured with a real workdir: His arithmetic holds: the base is How I missed it: I ran the red arm, saw the two expected failures, and treated check "premise: the destination plus a generated part name exceeds MAXPGPATH" \
"$([ $(( ${#LONG_DIR} + 24 )) -ge 1024 ] && echo yes || echo no)" "yes"and if the guard is widened to 30 per finding 1, that premise moves with it — On his minor, I agree with his framing over my ownI raised "assert the SQLSTATE" on all four PRs. He is right that this one is not I still hold that |
|
Two additions from a second pass, one of which is a second unfixed site rather than a test problem. MAJOR:
|
Requested on review. The guard measured dir + "/part-2147483647.parquet", 24 bytes past the directory. pexport_remove_outputs() composes "%s/%s" from the same directory and a directory entry into a MAXPGPATH buffer, and the entries it acts on include the sink's in-flight form part-NNNN.parquet.tmp.<pid> -- 30 bytes past the directory with a 7-digit pid. So a destination of 994..999 bytes passed the guard and the cleanup scan then truncated a path it goes on to unlink. The probe now uses the longest form the file constructs, 39 bytes wide, and the error names the temporary suffix as well as the part name. The sink is not at risk and is unchanged: columnar_sink.c builds its temp name with psprintf, which allocates rather than truncating. Both directions are pinned: a destination in the window is rejected, and one just under it still exports and writes _SUCCESS, so the guard cannot quietly become over-broad. CHANGELOG added.
2 participants
Summary
Reproduction
On current origin/main, a valid 1007-byte destination returns 10 rows successfully and writes _SUCCESS, but the data file is silently named part-0000.parqu. read_parquet ignores that file, so the completion marker certifies unreadable output.
Tests