Fix: check the parts of a buffer descriptor that were taken on trust - #1704
Conversation
📝 WalkthroughWalkthroughChangesBuffer integrity
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ImportRegistry
participant POSIXSharedMemory
participant ImportedBuffer
ImportRegistry->>ImportRegistry: Check descriptor for identity
ImportRegistry->>POSIXSharedMemory: Open and inspect object size
POSIXSharedMemory-->>ImportRegistry: Return actual size
ImportRegistry->>ImportedBuffer: Store mapping and source descriptor
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/simpler/buffer.py`:
- Around line 449-451: Replace the Unicode multiplication character in the
docstring near the endpoint and address_space description with an equivalent
ASCII expression, preserving the docstring’s meaning and surrounding text.
In `@src/common/task_interface/buffer.h`:
- Around line 330-336: Define a single POSIX_SHM name encoding in the
BufferDescriptor schema: update the validator in BufferDescriptor handling for
BackendKind::POSIX_SHM to reject bodies that are not valid UTF-8 in addition to
embedded NULs, and keep Python’s buffer materialization aligned by validating
the same rule before desc.body.decode in python/simpler/buffer.py and raising
the descriptor-validation error type. Add coverage for the malformed UTF-8 case
in both the C++ tests around test_buffer.cpp and the Python construction tests
in test_buffer.py so the native and Python paths enforce the same contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 018d3d7c-2b60-4070-908c-8f555d2d8725
📒 Files selected for processing (5)
python/simpler/buffer.pysrc/common/hierarchical/worker_manager.hsrc/common/task_interface/buffer.htests/ut/cpp/types/test_buffer.cpptests/ut/py/test_buffer.py
Two ways `ImportRegistry.materialize` accepted a backing it had not verified. A cache hit returned the stored `ImportedBuffer` without looking at the descriptor that asked for it. Identity says WHICH allocation, not how big it is or how to reach it, so a second descriptor carrying one identity and a different nbytes / access / backend_kind / body describes something the existing mapping is not — and the caller got that mapping with no sign anything differed. The hit now compares field-wise and refuses a mismatch. The mapping already handed out wins: callers may hold addresses into it, so a conflict is refused rather than resolved by replacing it. The POSIX_SHM branch mapped by name without checking the object is as large as the descriptor claims. Every view bound check is `byte_offset + extent <= nbytes`, so an unverified nbytes makes all of them comparisons against a number no memory stands behind — including the overflow-safe ones. The object's real size is the one quantity here an owner cannot overstate, and it is now compared before the mapping is cached. `ImportedBuffer` keeps the descriptor it was built from, which is what makes the first check possible. Map-once stays a cache; it is not a trust boundary. The conflict error names only the fields that differ, as `name: old -> new`. Two full reprs would carry a 32-byte body twice for what is usually a one-field disagreement, leaving the reader to diff them by eye.
`validate_buffer_descriptor` bounded `body_len` against DESC_MAX_BYTES and stopped there, so the body's contents were never checked against the backend that says how to read them. `backend_kind` is what selects the reading, so a body that does not fit it is not a smaller backing — it is a different value. The sharp case is an address: a DEVICE_MALLOC body of three bytes reads as a truncated pointer, with nothing about it to distinguish it from a real one, and `materialize` then hands that out as a base. The four address-bearing backends now require exactly 8 bytes and a non-zero base, since nothing is mapped or allocated at 0 and a zero body is an unfilled one. POSIX_SHM requires a non-empty name with no embedded NUL — the name reaches a C API that stops at the NUL, so two distinct names could otherwise open one object. REMOTE_SIDECAR requires an empty body, its authoritative descriptor riding in the per-task sidecar rather than here. Reserved bytes past `body_len` must be zero. That tail crosses a process boundary with the descriptor, so whatever the owner's memory held there crosses with it. It is distinct from the struct's `_pad` fields, which equality and hashing ignore on purpose so two decodes of one backing key alike: this is a length-delimited payload's reserved region, not alignment slack. The construction path already zeroes it (verified against the bound constructor's value-init + memcpy), so nothing that builds a descriptor today is affected. The test fixture built a POSIX_SHM descriptor with no body at all, which the schema correctly refuses; it carries a name now, and a device-backed variant covers the address form. A POSIX_SHM name is restricted to printable ASCII other than '/'. The native validator alone would accept any non-NUL bytes while the Python side decodes the name as UTF-8 before the open, so `b"\xff"` passed here and failed there — one field with two schemas. Printable ASCII is a UTF-8 subset, which makes that decode total; '/' is excluded because shm_open forbids it inside a name, and space and the control range because the name is rendered into paths and logs. The reserved-must-be-zero rule extends to a Tensor's shape/stride slots past `ndims`. They are the same case as the body tail — a producer chose not to fill them, and the blob memcpy carries all 144 bytes across the boundary regardless — so leaving them out would make the rule look general while covering one field. The `_pad` fields stay excluded, and the comment now says why: they are alignment slack rather than a slot anyone selects, and `CanonicalIdentity`'s is deliberately tolerated so two decodes of one backing key alike. `docs/buffer-abi.md` is a published page whose Backends table described what each backend materializes to but not what its body may contain, which is now enforced. The table gains that column, and the two consequences a user can hit — the zero-reserved rule and the shm size check at import — are stated under it.
The existing bound is asserted against `ChipTensor`, the element the blob carries today. `Tensor` is 144 B where `ChipTensor` is 128 B, so the wire cutover swaps the element without touching either cap and the region only has to grow by 16 B x 256 for a full frame to overflow — with the descriptor size frozen, that would be a frame-size problem discovered by the first 256-tensor task. Asserting the wire element too makes the pair fail the build instead. It holds with room to spare today: 37896 B of a 64016 B region.
`simpler-remote-worker` rejects anything but `host_tcp` (hw-native-sys#1011), and hw-native-sys#1688 narrowed the unit tests it knew about onto that profile. The zero-residual acceptance tests landed from hw-native-sys#1692 in the same window still asked for `sim`, so the merge of two independently green PRs left every case in that file failing its own setup with `only host_tcp transport is accepted by simpler-remote-worker` — the rollback each one exists to observe never ran. Unrelated to the Buffer ABI; it rides here because it is one line and it is what keeps `ut` red on every PR that touches nothing near it.
3619995 to
9c1784a
Compare
|
Both inline threads addressed and resolved. Three more items came from a maintainer review off-thread, folded into the same commits: The reserved-must-be-zero rule now covers
The G3 conflict error names only the differing fields ( CI.
|
ChaoWao
changed the title
1 participant
Three places a buffer descriptor was taken on trust
The wire ABI froze its layout, and
validate_buffer_descriptoris the gate every receive boundaryruns. Three things it was documented to establish, it did not check. No field, offset, or enum
value changes — every
static_assertinbuffer.his untouched, so this is a tightening of whatthe existing bytes are allowed to say.
A cached mapping was returned without looking at the descriptor that asked for it
Identity says WHICH allocation, not how big it is or how to reach it. A second descriptor carrying
one identity and a different
nbytes/access/backend_kind/bodydescribes something theexisting mapping is not, and the caller got that mapping with no sign anything differed:
The mapping already handed out wins — callers may hold addresses into it — so a conflict is refused
rather than resolved by replacing it. Map-once stays a cache; it is not a trust boundary.
A POSIX shm object was mapped without checking it is as large as claimed
Every view bound check is
byte_offset + extent <= nbytes. An unverifiednbytesmakes all of themcomparisons against a number no memory stands behind — including the overflow-safe ones #1703 just
landed. The object's real size is the one quantity here an owner cannot overstate:
A body was accepted whatever the backend that reads it says it should be
body_lenwas bounded and nothing else. Butbackend_kindis what selects how a consumer readsbody, so a body that does not fit that reading is not a smaller backing — it is a different value.The sharp case is an address: a
DEVICE_MALLOCbody of three bytes reads as a truncated pointer,indistinguishable from a real one, which
materializethen hands out as a base.FORK_SHM/FORK_COW/DEVICE_MALLOC/VMM_WINDOWPOSIX_SHM/REMOTE_SIDECARThe
POSIX_SHMcharset is what makes one field stop having two schemas: the native validatoraccepted any non-NUL bytes while the Python side decodes the same field as UTF-8 before the open, so
b"\xff"passed here and failed there. Printable ASCII is a strict UTF-8 subset, so the decode istotal by construction rather than by a second check that could drift.
/is excluded becauseshm_openforbids it inside a name; space and the control range because the name is rendered intopaths and diagnostics.
Bytes a producer chose not to fill must be zero — the tail of
bodypastbody_len, and aTensor's shape/stride slots pastndims. Both cross the process boundary with the struct (the blobmemcpy carries all 144 bytes regardless), so whatever the owner's memory held there would cross with
it. The
_padfields are deliberately not covered, and the comment says why rather than leaving itto be inferred: they are alignment slack no producer selects, and
CanonicalIdentity's is toleratedon purpose so two decodes of one backing still key alike.
I checked the construction path already zeroes those regions before tightening this, so nothing that
builds a descriptor today is affected. Two test fixtures were: both built a
POSIX_SHMdescriptorwith no body at all, a shape no real system produces.
The 256-tensor bound was asserted against the wrong element
MAILBOX_ARGS_CAPACITYis checked againstChipTensor.Tensoris 144 B whereChipTensoris128 B, so the wire cutover swaps the blob's element without touching either cap, and the region only
has to grow by 16 B × 256 for a full frame to overflow — with the descriptor size frozen, that is a
frame-size problem found by the first 256-tensor task. Asserting the wire element makes the pair fail
the build instead. It holds with room to spare: 37896 B of a 64016 B region.
One unrelated fix, riding along
simpler-remote-workeraccepts only thehost_tcptransport (#1011). #1688 narrowed the remote unittests it knew about onto that profile; #1692 landed
test_remote_zero_residual.pyin the same windowstill asking for
sim. Two independently green PRs, red after the merge — every case in that filefailed its own setup, so the rollback each one exists to observe never ran.
It is one line, it touches nothing near buffers, and it is what keeps
utred on every PR in flight,so it is here as its own commit. Say the word and I will split it out.
Verification
cpputtest_buffer— 18/18 (3 new: body schema per backend, non-zero reserved tail,non-zero slots past
ndims)pyutfull suite — 1149 passed, 13 skipped, 0 failed (the 3 remote-transport failures gogreen with the fix above)
Not here
address_spacematrix.materializestill resolves a DEVICE backing for a hostendpoint. That is a behavioural gate needing owner/registry context, with its own test surface.
owner_instance_idafter fork/adoption, and the release-path ordering (a mapping-closefailure must not skip the owner unlink; a released handle must not keep deriving
Tensors). Bothare owner-authority and lifetime changes; doing them here would blur where this change ends.
SIMPLER_BUILD_COMMITchange.buffer.hjustifies having no wire version field by citingthat guard, so I checked it exists rather than assuming:
python/simpler/task_interface.py:95-133compares the extension's stamp against
git rev-parse HEADand raises. It caught me during thiswork, after a
reset --hardmoved HEAD. The comment is accurate as written.🤖 Generated with Claude Code