fix(store): read full link sidecar to avoid wrong-symbol truncation#334
Merged
singaraiona merged 2 commits intoJul 18, 2026
Merged
Conversation
Collaborator
|
Please reject a short read before trimming or interning. |
try_load_link_sidecar read the target table's sym name into a fixed 256-byte buffer (fread of 255 bytes). A name longer than 255 bytes was silently truncated, so ray_sym_intern interned a DIFFERENT symbol and the loaded column linked to the wrong table — silent data corruption on a save/load round-trip. The writer already emits the full, untruncated name. Read the whole sidecar into a buffer sized to the file (capped at 1 MiB to bound a corrupt/oversized file), and reject a short read (fread returning fewer bytes than the file size — an I/O error or a race-truncated sidecar) so a partial name can't be interned as a different symbol either. Add a regression test that links through a 300-byte target name and asserts the loaded link_target matches; it fails without the fix.
belowzeroff
force-pushed
the
fix/col-link-sidecar-truncation
branch
from
July 18, 2026 15:42
6c29a63 to
1c63994
Compare
Contributor
Author
|
Thank you. Added an explicit n == fsz short-read check before trimming/interning; a partial read now frees the buffer and leaves the column unlinked. Amended in 1c63994. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
try_load_link_sidecar(src/store/col.c) read a linked column's target tablesym name into a fixed 256-byte stack buffer (
freadof 255 bytes). A targetname longer than 255 bytes was silently truncated, so
ray_sym_interninterneda different symbol and the loaded column linked to the wrong table —
silent data corruption across a save/load round-trip. The writer side already
emits the full, untruncated name, so only the reader was capping.
(The
255limit insym.cbounds the number of dot-separated segments, notthe byte length of a name, so names longer than 255 bytes are legal and
reachable.)
The fix reads the whole sidecar into a buffer sized to the file, capped at 1 MiB
so a corrupt/oversized sidecar can't force a large allocation, using the
ray_alloc_raw/ray_free_rawpair already used elsewhere in the file. Thetrailing-whitespace trim and best-effort semantics (any failure leaves the
column unlinked) are preserved.
Test
link/persistence_long_target_namelinks a column through a 300-byte targetname, saves, loads, and asserts the loaded
link_targetstill resolves to theoriginal symbol. Verified as a genuine regression test — with the old reader the
suite reports:
and with the fix it passes.
Full suite under the debug ASan + UBSan build:
3632 of 3633 passed (1 skipped, 0 failed).Checklist
dev(notmaster)fix:)makebuilds cleanly (no new warnings)make testpasses; tests added/updated for behaviour changes