From 78fc2fac5f3d5b1238b3c86852e32117dda286d1 Mon Sep 17 00:00:00 2001 From: "Joshua (D) Drake" <136637981+ChronicallyJD@users.noreply.github.com> Date: Tue, 1 Sep 2026 17:33:21 +0000 Subject: [PATCH 1/3] fix: reject truncated parallel export paths Co-authored-by: Cursor --- src/columnar_parallel_export.c | 15 +++++++++++++++ test/parallel_export_parquet.sh | 20 ++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/src/columnar_parallel_export.c b/src/columnar_parallel_export.c index 5b609384..d6d445de 100644 --- a/src/columnar_parallel_export.c +++ b/src/columnar_parallel_export.c @@ -606,6 +606,7 @@ pgcolumnar_parallel_export_parquet(PG_FUNCTION_ARGS) cur; int64 total = 0; int failed = -1; + char pathProbe[MAXPGPATH]; if (PG_ARGISNULL(0) || PG_ARGISNULL(1)) ereport(ERROR, @@ -700,6 +701,20 @@ pgcolumnar_parallel_export_parquet(PG_FUNCTION_ARGS) errmsg("relation \"%s\" is not a columnar table or a partitioned table with columnar partitions", RelationGetRelationName(rel)))); + /* + * Worker paths cross DSM in MAXPGPATH buffers and add a generated part + * name. Refuse a destination that cannot hold the longest possible name; + * truncating it would publish a differently named object and still stamp + * _SUCCESS over an unreadable export. + */ + if (snprintf(pathProbe, sizeof(pathProbe), "%s/part-%04d.parquet", + dir, INT_MAX) >= (int) sizeof(pathProbe)) + ereport(ERROR, + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), + errmsg("parallel export destination is too long"), + errdetail("The destination and generated part name must fit in %d bytes.", + MAXPGPATH))); + pexport_prepare_dir(dir); /* diff --git a/test/parallel_export_parquet.sh b/test/parallel_export_parquet.sh index f16d2d87..bf029114 100755 --- a/test/parallel_export_parquet.sh +++ b/test/parallel_export_parquet.sh @@ -191,6 +191,26 @@ check "the retry writes a fresh _SUCCESS marker" \ "$([ -f "$DCX/_SUCCESS" ] && echo yes || echo no)" yes # ---- error cases ------------------------------------------------------------ +# Worker destinations cross DSM in MAXPGPATH buffers and then gain a generated +# part name. A path that fits by itself but not with that suffix used to be +# silently truncated: the function returned success and wrote _SUCCESS beside +# part-0000.parqu, which read_parquet ignores. Reject it before creating the +# destination or any misleading output. +LONG_PARENT="$PGC_WORKDIR/long_path" +mkdir -p "$LONG_PARENT" +long_piece="$(printf 'a%.0s' {1..120})" +while [ ${#LONG_PARENT} -lt 980 ]; do + LONG_PARENT="$LONG_PARENT/$long_piece" + mkdir -p "$LONG_PARENT" +done +chmod 777 "$LONG_PARENT" +LONG_DIR="$LONG_PARENT/target" +long_out="$(err_of "SELECT pgcolumnar.parallel_export_parquet('t_col'::regclass, '$LONG_DIR', 2)")" +check "reject a destination whose generated part path would truncate" \ + "$(grep -qi 'destination is too long' <<<"$long_out" && echo error || echo ok)" error +check "long destination is rejected before it is created" \ + "$([ -e "$LONG_DIR" ] && echo created || echo absent)" absent + # st_1 was written above, so it is non-empty expect_error "reject a non-empty output directory" \ "SELECT pgcolumnar.parallel_export_parquet('t_col'::regclass, '$PGC_WORKDIR/st_1', 2)" From 7f3d4fc529b41d0366a2862ea9bb20f52ba84f6d Mon Sep 17 00:00:00 2001 From: OffgridwithJD Date: Tue, 1 Sep 2026 21:41:37 +0000 Subject: [PATCH 2/3] fix: probe the longest path this file builds, not the part name 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. -- 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. --- CHANGELOG.md | 18 ++++ src/columnar_parallel_export.c | 19 +++- test/parallel_export_parquet.sh | 171 ++++++++++++++++++++++++++++---- 3 files changed, 185 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38a79d9e..05ba30d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -208,6 +208,24 @@ true until the next version shipped. "it failed" without touching the guard are pinned as controls: a missing function and a wrong-arity call (`42883`), a null table name (`22004`) and `1/0` (`22012`). +- A parallel export destination is measured against the longest name the export + actually builds, not against the final part name (#863). + + **The guard probed a shorter path than the code composes.** It measured + `dir + "/part-2147483647.parquet"`, 24 bytes past the directory, and accepted + anything that fit. `pexport_remove_outputs` then 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.` -- 30 + bytes past the directory with a 7-digit pid. + + So a destination between 994 and 999 bytes long 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. + + A destination in that range is refused where it was previously accepted. The + export it would have produced was already unreadable, since the part names it + wrote were the truncated ones. - A shebang and the execute bit go together, and every directory that documents a command is swept (#856). Two things were left over from #852. diff --git a/src/columnar_parallel_export.c b/src/columnar_parallel_export.c index d6d445de..73ea1b1f 100644 --- a/src/columnar_parallel_export.c +++ b/src/columnar_parallel_export.c @@ -706,13 +706,26 @@ pgcolumnar_parallel_export_parquet(PG_FUNCTION_ARGS) * name. Refuse a destination that cannot hold the longest possible name; * truncating it would publish a differently named object and still stamp * _SUCCESS over an unreadable export. + * + * The longest such name is NOT the final part name. pexport_remove_outputs() + * composes "%s/%s" from this 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.. Measured: "/part-0000.parquet.tmp.PID" + * with a 7-digit pid (this platform's pid_max is 4194304) is dir + 30, while + * the final part name "/part-2147483647.parquet" is only dir + 24, so + * probing the part name alone left a window -- dir lengths 994..999 at + * MAXPGPATH 1024 -- where the destination was accepted and the cleanup scan + * then truncated a path it unlinks. Probe the longest form this file + * constructs instead. The part index and the pid are both ints, so INT_MAX + * bounds each, and the probe is 39 bytes wide against the part name's 24. */ - if (snprintf(pathProbe, sizeof(pathProbe), "%s/part-%04d.parquet", - dir, INT_MAX) >= (int) sizeof(pathProbe)) + if (snprintf(pathProbe, sizeof(pathProbe), "%s/part-%04d.parquet.tmp.%d", + dir, INT_MAX, INT_MAX) >= (int) sizeof(pathProbe)) ereport(ERROR, (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED), errmsg("parallel export destination is too long"), - errdetail("The destination and generated part name must fit in %d bytes.", + errdetail("The destination, the generated part name and the " + "in-flight temporary suffix must fit in %d bytes.", MAXPGPATH))); pexport_prepare_dir(dir); diff --git a/test/parallel_export_parquet.sh b/test/parallel_export_parquet.sh index bf029114..f08e698e 100755 --- a/test/parallel_export_parquet.sh +++ b/test/parallel_export_parquet.sh @@ -190,27 +190,158 @@ check "retry into the cleaned directory succeeds" \ check "the retry writes a fresh _SUCCESS marker" \ "$([ -f "$DCX/_SUCCESS" ] && echo yes || echo no)" yes -# ---- error cases ------------------------------------------------------------ -# Worker destinations cross DSM in MAXPGPATH buffers and then gain a generated -# part name. A path that fits by itself but not with that suffix used to be -# silently truncated: the function returned success and wrote _SUCCESS beside -# part-0000.parqu, which read_parquet ignores. Reject it before creating the -# destination or any misleading output. -LONG_PARENT="$PGC_WORKDIR/long_path" -mkdir -p "$LONG_PARENT" -long_piece="$(printf 'a%.0s' {1..120})" -while [ ${#LONG_PARENT} -lt 980 ]; do - LONG_PARENT="$LONG_PARENT/$long_piece" - mkdir -p "$LONG_PARENT" -done -chmod 777 "$LONG_PARENT" -LONG_DIR="$LONG_PARENT/target" -long_out="$(err_of "SELECT pgcolumnar.parallel_export_parquet('t_col'::regclass, '$LONG_DIR', 2)")" -check "reject a destination whose generated part path would truncate" \ - "$(grep -qi 'destination is too long' <<<"$long_out" && echo error || echo ok)" error -check "long destination is rejected before it is created" \ - "$([ -e "$LONG_DIR" ] && echo created || echo absent)" absent +# ---- destination-length guard (#863) ---------------------------------------- +# A destination that fits by itself but not with a generated suffix used to be +# silently truncated: the export returned success and stamped _SUCCESS beside a +# clipped part name, which read_parquet ignores. +# +# The guard has to probe the LONGEST path this file builds into a fixed +# MAXPGPATH buffer, and that is NOT the final part name. pexport_remove_outputs() +# composes "%s/%s" from the directory and a directory entry, and the entries it +# unlinks include the sink's in-flight form part-NNNN.parquet.tmp.: +# +# final part name "/part-2147483647.parquet" dir + 24 +# cleanup scan "/part-0000.parquet.tmp.1234567" dir + 30 +# +# 7-digit pids are reachable here (/proc/sys/kernel/pid_max is 4194304), so a +# guard that probes only the part name accepts dir lengths 994..999 while the +# cleanup scan's buffer truncates a path it then unlinks. The probe must be the +# longest constructed form, "/part-.parquet.tmp.", which is 39 bytes +# at INT_MAX for both numbers -- so the longest acceptable destination is +# MAXPGPATH - 1 - 39. +# +# Both boundaries are asserted: one byte under must still export (this class of +# fix breaks by becoming over-broad and rejecting legal paths) and one byte over +# must be refused with ERRCODE_PROGRAM_LIMIT_EXCEEDED, by SQLSTATE -- "it +# failed" also passes for a typo, a missing table or a dead server. + +PEXPORT_TOO_LONG_SQLSTATE=54000 # ERRCODE_PROGRAM_LIMIT_EXCEEDED + +# premise 1: the arithmetic is written for MAXPGPATH == 1024. +PGC_MAXPGPATH="$(sed -n 's/^#define[[:space:]]\{1,\}MAXPGPATH[[:space:]]\{1,\}\([0-9]\{1,\}\).*/\1/p' \ + "$("$PGC_PG_CONFIG" --includedir-server)/pg_config_manual.h" | head -1)" +if [ "$PGC_MAXPGPATH" != "1024" ]; then + echo "PREMISE FAILED: MAXPGPATH is [$PGC_MAXPGPATH]; these arms are written for 1024" >&2 + exit 1 +fi + +# premise 2: the cleanup scan still builds dir + "/" + entry into a fixed +# buffer, and the sink still appends ".tmp." to the part name. If either +# moves, the +30 above is stale and these lengths measure nothing. +pexport_anchor="$(grep -c 'snprintf(fp, sizeof(fp), "%s/%s", dir, de->d_name);' \ + "$PGC_SRCDIR/src/columnar_parallel_export.c")" +sink_anchor="$(grep -c 'psprintf("%s.tmp.%d", path, MyProcPid)' \ + "$PGC_SRCDIR/src/columnar_sink.c")" +if [ "$pexport_anchor" != "1" ] || [ "$sink_anchor" != "1" ]; then + echo "PREMISE FAILED: cleanup-scan anchor [$pexport_anchor] sink .tmp anchor [$sink_anchor], wanted 1 and 1" >&2 + exit 1 +fi + +# Longest destination that still holds the longest generated path, and the +# shortest destination at which the cleanup scan's "%s/%s" truncates with a +# 7-digit pid (len("/part-0000.parquet.tmp.1234567") == 30). +PEXPORT_LEN_OK=$(( PGC_MAXPGPATH - 1 - 39 )) # 984 +PEXPORT_LEN_OVER=$(( PEXPORT_LEN_OK + 1 )) # 985 +PEXPORT_LEN_WINDOW=$(( PGC_MAXPGPATH - 30 )) # 994 +# Build a directory path of an EXACT length and create its parents 777, so the +# server (running as postgres) can create the final component itself. Sets +# LP_PATH; it does not echo, because an exit inside a command substitution would +# leave the suite running with an ungated premise. +LP_PATH="" +path_of_len() { + local want="$1" p="$PGC_WORKDIR/lp" rem + while [ $(( want - ${#p} - 1 )) -gt 255 ]; do + p="$p/$(printf 'a%.0s' $(seq 1 200))" + done + rem=$(( want - ${#p} - 1 )) + if [ "$rem" -lt 1 ]; then + echo "PREMISE FAILED: cannot build a ${want}-byte path under $PGC_WORKDIR" >&2 + exit 1 + fi + p="$p/$(printf 'a%.0s' $(seq 1 "$rem"))" + if [ "${#p}" -ne "$want" ]; then + echo "PREMISE FAILED: built a ${#p}-byte path, wanted $want" >&2 + exit 1 + fi + mkdir -p "$(dirname "$p")" || { echo "PREMISE FAILED: mkdir -p for $want failed" >&2; exit 1; } + chmod -R 777 "$PGC_WORKDIR/lp" || { echo "PREMISE FAILED: chmod for $want failed" >&2; exit 1; } + if [ ! -d "$(dirname "$p")" ] || [ -e "$p" ]; then + echo "PREMISE FAILED: parent missing or target already present for $want" >&2 + exit 1 + fi + LP_PATH="$p" +} + +# Run one statement ONCE and record its 5-char SQLSTATE, "none" when it +# succeeded, or HANG on the wall-clock cap. VERBOSITY verbose puts the SQLSTATE +# and the message on the same ERROR line, so one run yields both, and a caller +# can assert the message without executing the statement a second time -- a +# second run of an accepted export hits the require-empty check and returns a +# DIFFERENT sqlstate (55000), which is exactly how "it failed" lies. +# +# Sets globals rather than echoing: a global assigned inside $( ) is assigned in +# a subshell and never reaches the caller, so SQLSTATE_LAST_OUT would arrive +# empty and its message check would silently compare nothing. +SQLSTATE_LAST="" +SQLSTATE_LAST_OUT="" +run_sqlstate() { + local rc + SQLSTATE_LAST="" + SQLSTATE_LAST_OUT="$(timeout -s KILL 180 env PATH="$PGC_BINDIR:$PATH" psql \ + -h 127.0.0.1 -p "$PGC_PORT" -U postgres -d "$PGC_DB" -qtA 2>&1 < Date: Wed, 2 Sep 2026 15:54:30 +0000 Subject: [PATCH 3/3] docs: describe the defect, not the review iteration (#863) Raised on review and correct. The entry documented an intra-branch refinement -- "the guard probed a shorter path than the code composes" -- for a guard that has never existed in a released version: both commits on this branch are unreleased, so there was no earlier probe for a reader to be corrected about. Worse, its last paragraph said the newly refused exports "were already unreadable, since the part names it wrote were the truncated ones". That is false for the 994..999 window, where the part names fit and only the cleanup scan truncates, and it contradicts this branch own commit message, which states the sink is not at risk. A user reading it would conclude that complete, readable exports were broken. The entry now states what a user would have hit: no length check at all, part names truncating at 1000 bytes and up with _SUCCESS stamped over the result, and the cleanup scan truncating from 994. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_011QP9UpEMdAj814XAPmftAH --- CHANGELOG.md | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 05ba30d2..c700fc82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -208,25 +208,34 @@ true until the next version shipped. "it failed" without touching the guard are pinned as controls: a missing function and a wrong-arity call (`42883`), a null table name (`22004`) and `1/0` (`22012`). -- A parallel export destination is measured against the longest name the export - actually builds, not against the final part name (#863). - - **The guard probed a shorter path than the code composes.** It measured - `dir + "/part-2147483647.parquet"`, 24 bytes past the directory, and accepted - anything that fit. `pexport_remove_outputs` then 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.` -- 30 - bytes past the directory with a 7-digit pid. - - So a destination between 994 and 999 bytes long 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. - - A destination in that range is refused where it was previously accepted. The - export it would have produced was already unreadable, since the part names it - wrote were the truncated ones. +- A parallel export refuses a destination too long to hold the names it generates + (#863). + + **`pgcolumnar.parallel_export_parquet` checked the destination's length not at + all.** Worker paths cross shared memory in `MAXPGPATH` buffers and have a + generated name appended, and `snprintf` truncates rather than failing, so a long + destination produced short names and the export completed as though nothing had + happened. + + Two distinct failures, both silent. A destination of 1000 bytes or more truncated + the part names themselves: the run wrote `part-0000.parqu` instead of + `part-0000.parquet`, stamped `_SUCCESS` beside it, and reported success for an + export `pgcolumnar.read_parquet` does not recognise. A destination of 994 to 999 + bytes wrote correct part names but truncated the path that + `pexport_remove_outputs` composes for its cleanup scan -- `"%s/%s"` from the same + directory and the sink's in-flight `part-NNNN.parquet.tmp.`, which is 30 + bytes past the directory with a seven-digit pid -- so the scan unlinked a + truncated path and left the temporary file behind. + + The destination is now probed against the longest name the file constructs, + `"/part-%04d.parquet.tmp.%d"` at `INT_MAX` for both, which is 39 bytes past the + directory against the final part name's 24. Anything that does not fit is refused + with `54000` before the destination directory is created, because a truncated run + publishes a differently named object and still stamps `_SUCCESS` over it. + + The sink itself was never at risk and is unchanged: `columnar_sink.c` builds its + temporary name with `psprintf`, which allocates. - A shebang and the execute bit go together, and every directory that documents a command is swept (#856). Two things were left over from #852.