Uh oh!
There was an error while loading. Please reload this page.
Refuse a PTE or PTD file whose schema version is newer than the runtime can read - #22114
Conversation
…read Every exported PTE file carries a schema version. The exporter stamps it from EXECUTORCH_SCHEMA_VERSION, and nothing in the runtime ever read it back. The only gate on a file was the four byte identifier ET12, which says "this is an ExecuTorch program" and says nothing about which shape of program it is. A file written by an exporter newer than the runtime was therefore accepted, then misread field by field, and failed later at load or during execution with an error that points somewhere else. This matters more now that the two halves can come from different builds. The wheel ships prebuilt runtime libraries, so a user can export with one installation of ExecuTorch and run with a runtime that was never built next to it. Read the version in Program::load, right after the root table is obtained, and refuse anything above Program::kMaxSupportedSchemaVersion with InvalidProgram and a message that names both numbers. The comparison is "less than or equal", not "equal", because the project promises that an older file keeps working on a newer runtime, and the schema only ever grows by appending optional fields. Only a file from the future is refused. The new constant sits next to the class it guards, and the two constants point at each other in comments so that a bump of one without the other is easy to spot. Nothing writes a version other than zero today, so no file in the wild changes behavior. This is the reader side that a future version bump needs in order to mean anything. Two new tests build a minimal program through the real FlatBuffer builder, one stamped at the supported version and one above it, and check that the first loads and the second returns InvalidProgram at the cheapest verification level. Ran the program test suite on Linux x86_64 before and after the change: the same tests pass in both, plus the two new ones. Deleting the check and keeping the tests makes the negative test fail, with the newer file loading successfully, so the check is what catches it. Measured on the object file, the cost is 24 bytes with logging disabled and 239 bytes with logging enabled. The companion PTD file has the same unread version field, and this change does not touch it.
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/22114
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 2 Unclassified FailuresAs of commit 8f613a8 with merge base 1afd07f ( NEW FAILURE - The following job has failed:
UNCLASSIFIED FAILURES - DrCI could not classify the following jobs because the workflow did not run on the merge base. The failures may be pre-existing on trunk or introduced by this PR:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
…read The data file has the same hole the program file had. Both writers, the C++ one in serialize.h and the Python one in serialize.py, stamp a schema version into every PTD file, and nothing in the runtime ever read it back. The four byte identifier FT01 was the only gate, and it says nothing about which shape of data follows. Read the version in FlatTensorDataMap::load, right after the root table is obtained, and refuse anything above FlatTensorDataMap::kMaxSupportedSchemaVersion with InvalidExternalData, which is what the checks around it return. The rule is the same as for the program file: "less than or equal", so an older data file keeps loading. Two new tests build a minimal PTD file in memory, one stamped at the supported version and one above it. The positive one is the control. Without it, a rejection could just as well come from a malformed fixture as from the version. Ran the data map suite on Linux x86_64: the six existing tests pass unchanged and both new ones pass. Deleting the check makes the negative test load the newer file with no error at all, so the check is what catches it. Measured on the object file, the cost is 32 bytes with logging disabled and 243 bytes with logging enabled.
The comment next to each version check said the four byte identifier "only says that this is a program" / "FlatTensor data". That undersells it. The identifier is itself the coarse compatibility gate: ET12 and FT01 are bumped only on a backward-incompatible schema change, so the identifier selects a schema family and check_header already rejects a file from a different family as IncompatibleVersion. The new version scalar is the finer gate within one family. Reword both comments to say that, so a future reader does not conclude the identifier carries no version meaning. Comment-only, no behavior change.
Gasoonjia
left a comment
There was a problem hiding this comment.
Thanks for doing the BC/FC check! I think there're two questions im not sure clear:
- When should we update the ExecuTorch version? What's the rule behind it?
- Is there any ci to mandatory make the version bump happen?
Gasoonjia
commented
Aug 25, 2026
@claude review the PR |
Claude finished @Gasoonjia's task in 2m 7s —— View job PR Review: Refuse a PTE/PTD file whose schema version is newer than the runtime can readI read the full diff plus the surrounding load paths in What's good
The real gap — nothing enforces the constants stay in syncThis is the same concern behind @Gasoonjia's questions. The correctness of the gate depends on four hand-maintained constants agreeing:
Right now the only thing tying them together is the cross-reference comments. If someone bumps the writer ( Consider adding a tiny guard so a bump can't drift:
That would answer question 2 directly and make the comments enforceable rather than advisory. Minor
Answers to @Gasoonjia
Overall: correct and low-risk as written. The one thing I'd push for before merge is a mechanical guard that the writer and reader constants can't diverge — otherwise the whole mechanism is one forgotten edit away from silently regressing FC. |
The version gate this PR adds is only correct while five hand-maintained constants agree: the PTE writer (EXECUTORCH_SCHEMA_VERSION) and its runtime reader (Program::kMaxSupportedSchemaVersion), and the two PTD writers (_FLAT_TENSOR_VERSION, kSchemaVersion) and their reader (FlatTensorDataMap::kMaxSupportedSchemaVersion). Until now only cross-reference comments tied them together, so bumping a writer without its reader would make a runtime refuse files it should read, and bumping one PTD writer without the other would let a file be stamped below its real layout and misread. Extend schema/test/test_schema.py, which already enforces cross-file schema sync and runs in OSS CI with no build wiring, with a check that parses the five literals as text and asserts the compatibility relationship: each writer must be <= its reader ceiling (a reader may support a version before any writer emits it, but never the reverse), and the two PTD writers must be exactly equal because they stamp the same field of the same file. The parse keys on file path, not symbol, since the two reader ceilings share a name, and fails closed if a constant can no longer be found so a reformat can't silently disable the guard. Also state the bump rule next to EXECUTORCH_SCHEMA_VERSION and soften the file-identifier comments: the identifier is bumped by convention on a breaking change, not by anything the code enforces.
shoumikhin
commented
Aug 25, 2026
Thanks @claude — good catch on the drift risk. I added the guard, and it's the right call to do it in this PR since this is what turns the constants into a real gate. What I added: a check in
The relationship is writer-≤-reader, not strict equality: a runtime is allowed to support a version before any writer emits it (forward-compat), but a writer must never stamp a version its reader would refuse. The two PTD writers are compared with I verified it locally: passes today, fails when I bump one writer without its reader, fails when the two PTD writers disagree, and still passes when only a reader is bumped ahead (the legitimate case). On the minors:
On mandating a bump when a |
shoumikhin
commented
Aug 25, 2026
@claude review the PR |
Claude finished @shoumikhin's task in 1m 32s —— View job Re: the drift guard ( |
| Constant | Declaration | Regex |
|---|---|---|
EXECUTORCH_SCHEMA_VERSION | = 0 (exir/version.py:14) | ✅ |
Program::kMaxSupportedSchemaVersion | = 0 (program.h:85) | ✅ |
_FLAT_TENSOR_VERSION | : int = 0 (serialize.py:49) | ✅ |
kSchemaVersion | = 0 (serialize.h:26) | ✅ |
FlatTensorDataMap::kMaxSupportedSchemaVersion | = 0 (flat_tensor_data_map.h:47) | ✅ |
The design decisions are right:
- Keying on file path, not symbol name is the correct call — the two reader ceilings both spell
kMaxSupportedSchemaVersion, so a symbol-based parse would be ambiguous. Path-keyedre.searchon each file avoids that, and sincekMaxSupportedSchemaVersiondoesn't contain the literal substringkSchemaVersion, the PTD-writer regex inserialize.hcan't accidentally match a ceiling either. - Writer
<=reader, PTD writers==matches the compatibility policy exactly: a runtime may support a version before any writer emits it (FC), but a writer must never stamp a version its reader refuses; and the two PTD writers stamp the same field, so they must be equal. The asymmetry is intentional and correctly encoded. - Fail-closed on a missing constant (
assertIsNotNonewith a message that tells the next editor to fix the test) is what makes a rename/reformat surface loudly instead of silently disabling the guard — this was the exact failure mode I was worried about. - Putting it in
test_schema.py(already in OSS CI, no new build wiring) is the pragmatic choice over astatic_assert, which couldn't reach the Python writers anyway.
On the minors — all handled as expected: the file-identifier comments now say "by convention," the bump rule is stated next to EXECUTORCH_SCHEMA_VERSION, and leaving the version()-before-verification read and the test reinterpret_cast as-is is fine.
One small note (non-blocking): the guard proves the five constants agree with each other, but the writer/reader ceiling and the actual serialized field are still tied only by the runtime code that reads version(). That's fine — that path is what your four new C++ tests exercise, so the two halves (constants-agree here, field-is-gated there) are each covered.
On follow-ups: filing separate issues for (a) mandating a version bump on .fbs shape changes and (b) the Python PTD reader still using != instead of <= sounds right — both are genuinely out of scope for this PR, and the constant-sync half was the load-bearing gap.
I couldn't run the suite here (sandbox blocked python), but the parse is simple text matching and I verified each pattern against its literal by hand; your local run (passes today, fails on each induced drift) covers the dynamic behavior.
LGTM.
• branch shoumikhin/program-version-gate
Claude finished @shoumikhin's task in 2m 4s —— View job Re-review: Refuse a PTE/PTD file whose schema version is newer than the runtime can readI re-read the full diff against The drift guard resolves the main gap
Readers — verified placement
Tests
Minors addressed
One optional follow-up (non-blocking)As you noted, the guard covers the constants agreeing half. It does not mandate a version bump when a Nothing here blocks merge. Nice work turning the cross-reference comments into an enforced check within this same PR. |
<cstring> and <vector> already arrive through buffer_data_loader.h and the generated flatbuffers header, so drop them. Replace the per-symbol using declarations with namespace directives, matching the convention used elsewhere.
Uh oh!
There was an error while loading. Please reload this page.
…22117) ### Summary The Python `FlatTensorSerializer.deserialize` refused any PTD file whose `version` did not **exactly** equal `_FLAT_TENSOR_VERSION`: ```python if flat_tensor.version != _FLAT_TENSOR_VERSION: raise NotImplementedError(...) ``` That rejects an **older** file too, which contradicts: - the append-only schema policy in `schema/README.md` (older files stay loadable), and - the C++ runtime readers, which accept anything `<=` their supported version and only refuse a file **newer** than they understand (`Program::load`, `FlatTensorDataMap::load`). This aligns the Python PTD reader with that policy: compare with `>` instead of `!=`, so an older or equal file loads and only a newer one is refused. The error message now says the file is newer than this reader supports. ### Context This was called out as a follow-up during review of #22114 (which adds the `<=` gate on the C++ side). It keeps the Python export/tooling path consistent so a future non-zero PTD version won't regress it. ### Test plan Two tests in `extension/flat_tensor/test/test_serialize.py`: - `test_deserialize_refuses_newer_version` — a bumped-version file is refused. - `test_deserialize_accepts_older_version` — a file older than the reader still loads (the case the old `!=` wrongly rejected). Both are behavior-only; no schema or format change. Version constants are unchanged (still `0`), so this is inert for existing files and only changes behavior once a non-zero version is ever stamped.
### Summary `FlatTensorDataMap::load()` reads the flatbuffer root table without first bounds-checking the root offset: ```cpp const flat_tensor_flatbuffer::FlatTensor* flat_tensor = flat_tensor_flatbuffer::GetFlatTensor(flat_tensor_data->data()); // then immediately: flat_tensor->named_data(); // walks the root vtable flat_tensor->segments(); ``` `GetFlatTensor()` interprets the `uoffset_t` at the start of the buffer as the root-table offset and returns `buf + offset`. The first field access then walks that table's vtable. Nothing on this path runs full flatbuffer verification, so a corrupt or truncated PTD file with a bad root offset makes these accessors dereference memory **outside the buffer** (an OOB read, flagged by ASan). The program loader already guards against exactly this in `runtime/executor/program.cpp`. This change mirrors that guard for PTD files. ### What changed After the existing identifier and alignment checks, before `GetFlatTensor()`: - Confirm the buffer is at least a flatbuffer header long (`uoffset_t` + file identifier). - Confirm the root offset is `>= kMinBufferSize` and leaves room for a vtable `soffset_t` within the buffer. - Return `InvalidExternalData` otherwise (the same error the surrounding checks use). FlatTensor is **not** size-prefixed — both writers finish the buffer plain (`builder.Finish` in C++, default `flatc` in Python) and the reader uses `GetFlatTensor` (not the size-prefixed variant) — so the root offset is at byte 0, exactly as in the program case. ### Scope This is a pre-existing hardening, independent of the schema-version work in #22114 (the version gate this protects is additive; the OOB path exists today via `named_data()`/`segments()`). ### Test plan New `FlatTensorDataMapTest.RejectsOutOfBoundsRootOffset`: copies a valid PTD into a max-aligned buffer, overwrites only the root offset with a value past the end (leaving identifier + alignment intact), and asserts `load()` returns `InvalidExternalData`. Without the check, that same input is an out-of-bounds vtable read under ASan. > Note: I was unable to build/run the C++ suite in my local environment, so I'm relying on this PR's CI (which builds the runtime and runs `extension/flat_tensor` tests, including under ASan) as the authoritative check. The change mirrors an existing, tested guard in `program.cpp`.
Summary
Every exported PTE file and every PTD data file carries a schema version. The exporters stamp it, and nothing in the runtime ever read it back. The only gate on a file was its four byte identifier,
ET12for a program andFT01for data. That says "this is an ExecuTorch file" and says nothing about which shape of file it is. A file written by an exporter newer than the runtime was therefore accepted, then misread field by field, and failed later at load or during execution with an error that points somewhere else.This matters more now that the two halves can come from different builds. The wheel ships prebuilt runtime libraries, so a user can export with one installation of ExecuTorch and run with a runtime that was never built next to it.
This PR reads the version in both readers, right after the root table is obtained, and refuses anything above the highest version the runtime supports.
Program::loadreturnsInvalidProgram,FlatTensorDataMap::loadreturnsInvalidExternalData, which is what the checks around each of them return. Both name the two numbers:The comparison is "less than or equal", not "equal", because the project promises that an older file keeps working on a newer runtime, and the schemas only ever grow by appending optional fields. Only a file from the future is refused.
Each new constant sits next to the class it guards, and points in a comment at the writer constants for the same number:
EXECUTORCH_SCHEMA_VERSIONfor the program,kSchemaVersionand_FLAT_TENSOR_VERSIONfor the data file. A bump of one without the other is then easy to spot. Nothing writes a version other than zero today, so no file in the wild changes behavior. This is the reader side that a future version bump needs in order to mean anything.One limit worth stating plainly: a runtime that is already deployed has no check at all, so this protects runtimes built from this change onward. That is inherent to adding a reader.
Test plan
Four new tests, two per format. Each pair builds a minimal file in memory through the real FlatBuffer builder, one stamped at the supported version and one above it. The first is expected to load and the second to be refused. The positive test is the control: without it, a rejection could just as well come from a malformed fixture as from the version. The program test asks for the cheapest verification level, which shows the check does not depend on full verification being enabled. The data test follows the byte layout that
save_ptd()writes, with no segments.Built and ran both suites on Linux x86_64, before and after the change:
The existing tests pass unchanged in both runs, plus the four new ones. As a control, deleting either check while keeping the tests makes that format's negative test fail, with the newer file loading and reporting no error, so the check is what catches it.
Binary size, measured on the two object files.
program.cpp.ogrows by 24 bytes with logging disabled and 239 bytes with logging enabled.flat_tensor_data_map.cpp.ogrows by 32 bytes with logging disabled and 243 bytes with logging enabled.