Skip to content

fix(main-red): the LTX-2.5 VAE loader read the safetensors mmap through a uint16_t* it is not allowed to form (#674) - #688

Open
localai-bot wants to merge 1 commit into
mainfrom
row/FIX-MAIN-RED-DSR-ALIGN
Open

fix(main-red): the LTX-2.5 VAE loader read the safetensors mmap through a uint16_t* it is not allowed to form (#674)#688
localai-bot wants to merge 1 commit into
mainfrom
row/FIX-MAIN-RED-DSR-ALIGN

Conversation

@localai-bot

Copy link
Copy Markdown
Collaborator

Closes #674.

main has been RED on sanitize-cpu (address,undefined) since cefacd2d0
(#641, the LTX-2.5 landing). This closes it. The other half of that same red,
the device-leakage DSR ratchet (#553), needed no change and is dealt with
below.

What was wrong

Ltx2LoadVaeWeights read the safetensors mmap through a pointer it is not
allowed to form:

const uint16_t* src = reinterpret_cast<const uint16_t*>(t.data);
for (int64_t i = 0; i < numel; ++i) values[i] = Bf16ToF32(src[i]);

StTensor::data points into the read-only mapping at
8 + <JSON header length> + <sum of the preceding tensors' sizes>
(safetensors_reader.h:16-23). Not one of those three terms is required to be
even, so a BF16 tensor beginning on an odd byte is an ordinary safetensors
file and not a corrupt one. Forming and dereferencing a const uint16_t* there
is undefined behaviour on every target, and a genuine fault on the
strict-alignment ones this project builds and ships for: build-test-cpu-arm64,
Jetson/Orin (sm_110), Thor.

The fix

vt::LoadUnaligned<uint16_t> (include/vt/unaligned.h) — a std::memcpy with
no alignment precondition, which compiles to the same single load where the
address does happen to be aligned. Not a cast, not a pragma, not a suppression,
and not a change to any gate.

That seam exists because of this bug class: #301 created it the first time
this red-ed main. This is the third recurrence — #301 (closed:
cpu_ops.cpp:33, laguna.cpp:1028), #627 (open: qwen3_5_weights.cpp:332),
and now the VAE loader — and minimax_h3_vae_loader.cpp:87-101 already carried
both the repair and the reason in prose while a new loader reached main with
the cast anyway. Nothing greps for the pattern; that observation is recorded on
#674 and #627 rather than acted on here.

The coverage that caught it was accidental — that is fixed too

test_ltx2_video only reached the defect because ltx2_fixture's JSON header
happens to make one VAE tensor's offset odd today. A rename or a reshape in
that fixture silently retires the coverage and leaves every assertion green.

So this adds a case that forces the odd offset — it writes the file itself
and pads the counted JSON header by one space, keeping whichever of the two
parities is odd — and then asserts the parity it depends on before loading:

REQUIRE((reinterpret_cast<uintptr_t>(file.Get(name).data) % 2) == 1);

An edit that makes the address even now fails that REQUIRE instead of passing
while covering nothing. The values are chosen bf16-exact so the value check is
equality rather than a band, which makes a wrong-by-one-byte read a hard
failure. The case deliberately does not use Workspace: writing the whole
LTX-2.5 fixture would make it depend on the very fixture whose accidental
coverage it replaces.

Evidence

RED then GREEN, same binary, same command. setarch -R because sanitizer builds
SIGSEGV instantly on this host without it (ASLR entropy vs the kernel), which
reads exactly like a crash in the code under test.

cmake -S . -B build-sanitize -DVLLM_CPP_BUILD_TESTS=ON -DVLLM_CPP_CUDA=OFF \
      -DVLLM_CPP_SANITIZE='address,undefined'
UBSAN_OPTIONS=print_stacktrace=1 \
ASAN_OPTIONS=detect_leaks=1:strict_string_checks=1 VT_POOL_BYPASS=1 \
setarch -R ctest --test-dir build-sanitize -R '^test_ltx2_video$' --output-on-failure
step result
RED, whole target, unmodified 7965f12bf ***Failedltx2_loader.cpp:1325:91: runtime error: load of misaligned address 0x7fffc81cc129 for type 'const uint16_t', stack #0 Ltx2LoadVaeWeights #1 Ltx2VideoEngine::Load ltx2_video.cpp:777 #8 test_ltx2_video.cpp:218
RED, the NEW case alone, unmodified src same finding at 0x7ffff7fb005b (odd), stack #1 DOCTEST_ANON_FUNC_153 test_ltx2_video.cpp:1189 — so the forced-offset case reproduces it on its own
GREEN, the NEW case alone 1 passed | 0 failed, assertions: 11 | 11 passed, Status: SUCCESS!
GREEN, whole target, rebased head 1/1 Test #30: test_ltx2_video ... Passed 157.35 sec

Also green on the rebased head: scripts/check-device-leakage.py --report,
tests/scripts/test_device_leakage.py (26/26),
scripts/check-doc-checkpoint.py, scripts/check-public-doc-tables.py,
and scripts/agent-preflight.sh including its committed-range and
commit-trailer gates.

Baseline to subtract

The other half of the cefacd2d0 red: no change needed

device-leakage was already repaired by 11cc1d589, which routed LTX-2.5's
device question through CurrentPlatform().device_type() rather than taking the
ALLOWLIST escape. Verified rather than assumed — both CI steps run verbatim:
kcuda=0, DSR 32 == baseline 32, test_device_leakage.py 26/26, with
scripts/device-leakage-baseline.json untouched since 99b7443bd
(accelerator-seam S7), so it is a repair and not a raised threshold. #553 is
closed with that evidence. Its still-open successors #659 and #660 are separate
defects and are untouched here.

Scope, and what is left owed

Only the site that is red is touched. 19 sibling sites in 11 other loaders
form the same typed pointer over StTensor::data; the full grep is posted as an
inventory on #627, which owns the class. The widest is
qwen3_dspark_weights.cpp:99, a const int64_t* needing 8-byte alignment — UB
on 7 of every 8 offsets rather than 1 of 2. The sweep is value-identical by
construction but #627 requires 27B/35B/Coder golden-md5 inertness evidence for
the shared loader, and this repair has no GPU to produce it, so it is recorded
as owed rather than done blind.

docs/FEATURES.md records the resulting guarantee on the row it belongs to —
"Safetensors direct load" — rather than in the LTX-2.5 row, which is already at
its entry budget.

🤖 Generated with Claude Code

…t16_t* it is not allowed to form (#674)

`main` has been RED on `sanitize-cpu (address,undefined)` since `cefacd2d0`
(#641, the LTX-2.5 landing). Baseline run 31724380111 reports one failing test
with one finding:

    30/423 Test  #30: test_ltx2_video ...***Failed
    src/vllm/model_executor/models/ltx2_loader.cpp:1288:91: runtime error:
    load of misaligned address 0x7f3c895be129 for type 'const uint16_t',
    which requires 2 byte alignment

REPRODUCED LOCALLY at `7965f12bf`, same site (`:1325` after the file shifted),
same class, full stack through `Ltx2LoadVaeWeights` <- `Ltx2VideoEngine::Load`:

    ltx2_loader.cpp:1325:91: runtime error: load of misaligned address
    0x7fffc81cc129 for type 'const uint16_t' ...
      #0 vllm::Ltx2LoadVaeWeights(...) ltx2_loader.cpp:1325
      #1 vllm::multimodal::Ltx2VideoEngine::Load(...) ltx2_video.cpp:777

THE DEFECT. `StTensor::data` points into the read-only safetensors mmap at
`8 + <JSON header length> + <sum of the preceding tensors' sizes>`. Not one of
those three terms is required to be even, so a BF16 tensor starting on an ODD
byte is an ordinary file and not a corrupt one. Forming a `const uint16_t*`
there and dereferencing it is UB on every target, and a real fault on the
strict-alignment ones this project builds and ships for: `build-test-cpu-arm64`,
Jetson/Orin (sm_110), Thor.

THE REPAIR is the seam, not a cast and not a suppression: `vt::LoadUnaligned`
(include/vt/unaligned.h), which is a `std::memcpy` with no alignment
precondition and compiles to the same single load where the address does happen
to be aligned. That seam exists BECAUSE of #301, the first time this class red
main. This is its third recurrence -- #301 (closed: cpu_ops.cpp, laguna.cpp),
#627 (open: qwen3_5_weights.cpp `TransposeBf16`), and now the VAE loader -- and
minimax_h3_vae_loader.cpp:87-101 already carried the repair AND its reason in
prose while the new loader reached main with the cast. Nothing gates the
pattern; that is recorded on #674 rather than fixed here.

THE COVERAGE THAT CAUGHT IT WAS ACCIDENTAL, which is the part worth fixing.
`ltx2_fixture`'s JSON header happens to land one VAE tensor on an odd byte
today; a rename or a reshape in that fixture retires the coverage silently and
leaves every assertion green. So this adds a case that FORCES the odd offset --
it writes the file itself and pads the counted JSON header by one space, keeping
whichever of the two parities is odd -- and then ASSERTS the parity it depends
on before loading:

    REQUIRE((reinterpret_cast<uintptr_t>(file.Get(name).data) % 2) == 1);

An edit that makes the address even now fails that REQUIRE instead of passing
while covering nothing. The values are chosen bf16-exact so the check is
equality rather than a band: a wrong-by-one-byte read is a hard failure.

RED, then GREEN, same binary, same command, `setarch -R` (this host SIGSEGVs
sanitizer builds on ASLR entropy otherwise, which reads exactly like a crash in
the code under test):

  RED   new case alone, unmodified src:
        ltx2_loader.cpp:1325:91: runtime error: load of misaligned address
        0x7ffff7fb005b ...  #1 DOCTEST_ANON_FUNC_153 test_ltx2_video.cpp:1189
  GREEN new case alone:  1 passed | 0 failed | assertions: 11 | 11 passed
  GREEN whole target:    1/1 Test #30: test_ltx2_video ... Passed 379.03 sec
  GREEN whole target, re-run on THIS head after rebase onto 43a6c55:
                         1/1 Test #30: test_ltx2_video ... Passed 157.35 sec

  cmake -S . -B build-sanitize -DVLLM_CPP_BUILD_TESTS=ON -DVLLM_CPP_CUDA=OFF \
        -DVLLM_CPP_SANITIZE='address,undefined'
  UBSAN_OPTIONS=print_stacktrace=1 \
  ASAN_OPTIONS=detect_leaks=1:strict_string_checks=1 VT_POOL_BYPASS=1 \
  setarch -R ctest --test-dir build-sanitize -R '^test_ltx2_video$'

NOT FIXED HERE, and reported rather than swept: 19 sibling sites form the same
pointer over `StTensor::data` in 11 other loaders, the widest being
`qwen3_dspark_weights.cpp:99`, which forms a `const int64_t*` and so needs
8-byte alignment. They are inventoried on #627, which owns the class and asks
for 27B/35B/Coder golden-md5 inertness evidence this repair has no GPU to
produce. Only the site that is red is touched.

The other half of the `cefacd2d0` main-red, the `device-leakage` DSR ratchet,
needs no change: `11cc1d589` already routed it through the platform seam.
Verified at `7965f12bf` by running both CI steps verbatim -- kcuda 0, DSR 32 ==
baseline 32, `test_device_leakage.py` 26/26 -- with
`scripts/device-leakage-baseline.json` untouched since `99b7443bd`, so it is a
repair and not a raised threshold. #553 is closed with that evidence.

FOLLOWING_AGENTS_PROTOCOL

Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: Claude:claude-opus-5 [Claude Code]
@localai-bot

Copy link
Copy Markdown
Collaborator Author

Verified the alignment claim on Thor itself (aarch64, sm_110, NVIDIA Thor compute_cap 11.0, g++ 13.3, Ubuntu 24.04) rather than reasoning about it from x86. Result confirms the fix but narrows one claim in the PR body, so recording both halves.

I built a standalone reproduction of the exact shape — a bf16 payload placed at a deliberately odd offset inside a byte buffer, read once through reinterpret_cast<const uint16_t*> and once through the memcpy seam — and ran three arms on Thor.

The UB is real and reproduces on Thor:

align_repro.cpp:64:20: runtime error: load of misaligned address 0xaaab18e4b2b1
  for type 'const uint16_t', which requires 2 byte alignment
0xaaab18e4b2b1: note: pointer points here
 00 00 00  00 80 3f 00 40 40 40 80  40 80 bf 00 c0 00 3f 00 ...
              ^
    #0 ... in main align_repro.cpp:64

Same diagnostic class as the CI failure. The fix arm is clean under the identical sanitizer flags, and both arms produce byte-identical values (1, 2, 3, 4, -1, -2, 0.5, 0), so the memcpy seam is value-preserving here as expected.

What did NOT reproduce — and this is the correction. The PR body says the cast is "a genuine fault on the strict-alignment ones this project builds and ships for: build-test-cpu-arm64, Jetson/Orin (sm_110), Thor." On Thor it does not fault:

arm -O2 UBSan -O2 -mstrict-align
DEFECT exit 0, correct values misaligned-address error exit 0
FIX exit 0, correct values clean exit 0

ARMv8 permits unaligned LDRH at EL0 — Linux leaves SCTLR_EL1.A off — so the compiler emits a plain halfword load that the hardware tolerates, and even -mstrict-align did not trap it in this shape.

So the accurate statement is: undefined behaviour that the sanitizer catches and that reds a required gate, not a crash waiting to happen on our aarch64 targets. That is still a fix worth landing exactly as written — UB is UB, the sanitize-cpu job is red because of it, the seam already exists for this bug class, and a future compiler is entitled to assume the alignment and miscompile around it. But the justification should rest on that rather than on a fault nobody can demonstrate, because an overclaim here is the kind of thing that gets quoted back later.

Two things in this PR I'd highlight as the strongest parts, unchanged by the above:

Repro and runner are throwaway; nothing was installed on the Thor host and no build tooling was added.

localai-bot pushed a commit that referenced this pull request Aug 14, 2026
…#382 (#564)

Commits the spec for the ROCm head_dim=128 decode arm, the ROCm half of #382.
Zero source files; the implementation is not merged and this spec says so
explicitly.

Every code and upstream anchor it cites was verified exact at the pinned
oracle during review: bf16_decode_opt at rocm_paged_attn.hip:1684 matches its
quoted snippet verbatim, the EPL static_asserts at 268/289, the launch switches
at 1875-1904 with only 8/16 instantiations, the five-combination fallback
dispatch at 1937-1947, the CUDA counter-claim at cuda_paged_attn.cu:321/329/2796,
and CALL_CUSTOM_LAUNCHER_BLK_HEAD upstream at the pin.

Review repairs landed on the branch before merge. The blocking one: the spec
opened with "Landed the ROCm d=128 decode arm" when nothing had landed --
git log -S'VT_ATTN_DECODE_D128' -- src/vt/rocm/ is empty on main and
rocm_paged_attn.hip still gates on d == 256 || d == 512. Merging that text
would have put a false "landed" on main, where the next agent greps for the
flag, finds nothing, and cannot tell "never merged" from "reverted" from
"renamed". Renamed to "Result on the implementation branch" with a banner
quoting the two commands that show it, and section 4 moved to future tense for
the same reason.

Also added the two sections AGENTS.md requires and the spec lacked -- Risks and
decisions, and Stop conditions -- drawn from material already in the spec
rather than invented: the bf16-tie reduction-order risk that is why the arm
ships default OFF, the single-board provenance of the 3.53x, the sm_110
1.6x-SLOWER reversal recorded but deliberately unreconciled, the dangling
VT_ATTN_DECODE_WMMA forward reference, and the unquantified residual #488 gap.
Plus a base-SHA mismatch and an upstream line anchor that had drifted by two.

The spec is honest about what it could not gate: it labels its own throughput
table "indicative, not the 2-3x-idle-reproduced standard" and states that no
post-change per-call oracle re-measure was run.

Known-baseline failures only: windows-msvc-* are the PR-only arm (#584), and
sanitize-cpu (address,undefined) is red on main itself (#674, fix pending in
#688). This PR changes only Markdown.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

main is RED on sanitize-cpu: Ltx2LoadVaeWeights reinterpret_casts unaligned safetensors bytes to const uint16_t* (third recurrence of the class)

2 participants