The object namespace, walked by name rather than printed - #157
Conversation
`\Device\MountPointManager` is not an address, and nothing here could turn it into one: a symbol names what the linker placed, while an object is created at run time and filed under a name in a tree the object manager keeps. The debugger's answer is `!object`, an extension printing text. This is the same walk answering in values, which is what a caller reading a device's security descriptor needs -- the descriptor pointer is in the object's header, and the header is only reachable once the name has been resolved. `object_at` resolves a path, `objects_in` lists a directory, and `symbolic_link_target` reads where a link points. **Every bucket is walked rather than the one the name hashes to.** The hash is the object manager's own, over a name folded with the kernel's upcase table, and a wrong reimplementation of it does not fail -- it looks in the wrong bucket and reports that the object is not there, which is an answer a caller acts on. A directory holds tens of entries, so the saving is not worth that risk. **Not one literal offset.** `_OBJECT_HEADER` has moved between Windows versions, so every field comes from the target's own type information -- and two things that look like constants are derived rather than assumed. A pointer's width is the distance between a directory entry's two pointer fields, because a 32-bit kernel read from this host would otherwise be decoded with the host's width and every read after it would run off the end of something. The bucket count is the array's span over that width, because 37 is this build's number and not the structure's. **A cap is an error rather than a short list**, which is the same rule the security descriptor reader in the consumer follows: a directory reported shorter than it is answers "which symbolic links reach this device" wrongly, and that is the one direction a security question must not fail in. A chain that points at itself is refused rather than walked. Two subtleties the tests pin by mutation, one at a time: - The header's `SecurityDescriptor` keeps three object-manager flags in its low bits. Taken as an address it reads a descriptor three bytes into its own header and reports a DACL that is not there. - `TypeIndex` is exclusive-ored with a per-boot cookie **and with a byte of the header's own address**. Dropping either term reads a type out of the wrong table slot, which is how a device would read as a directory and be walked through. `LinkTarget` shares its storage with a callback pointer: `Callback` lands exactly on `Length` and `MaximumLength`, and `CallbackContext` lands on `Buffer`. So the string is checked before it is followed, and the test lays that union out as it really is rather than relying on a read that happens to fail. This does not work on a kernel minidump. Measured against the consumer's checked-in dump: `nt!ObpRootDirectoryObject` reads `????????`, so the walk stops at its first read and names the address rather than reporting an empty namespace. A live kernel is this code's tier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f726fbb198
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| root: self.symbol_offset("nt!ObpRootDirectoryObject")?, | ||
| info_mask_to_offset: self.symbol_offset("nt!ObpInfoMaskToOffset")?, | ||
| header_cookie: self.symbol_offset("nt!ObHeaderCookie").ok(), | ||
| type_index_table: self.symbol_offset("nt!ObTypeIndexTable").ok(), |
There was a problem hiding this comment.
ObTypeIndexTable without the extra p
The Windows kernel global is nt!ObTypeIndexTable, as the Globals documentation above also names it, but this lookup requests nt!ObpTypeIndexTable. Because the error is discarded with .ok(), every DebugEngine-backed walk silently receives type_index_table: None; consequently KernelObject::type_name is never populated, callers cannot filter symbolic links as required by link_target, and the NotADirectory traversal guard is disabled.
Useful? React with 👍 / 👎.
| .header_of(body) | ||
| .wrapping_add(u64::from(self.layout.header_security)); | ||
| // The low three bits are the object manager's own, never part of the address. | ||
| let descriptor = self.pointer_at(at)? & !0b111; |
There was a problem hiding this comment.
On 64-bit kernels the security descriptor is stored as an _EX_FAST_REF, whose low four bits hold the reference count. When bit 3 is set, masking only 0b111 leaves the returned address eight bytes past the actual descriptor, so downstream security-descriptor and ACL decoding can produce incorrect security results; the mask needs to account for the target pointer width.
Useful? React with 👍 / 👎.
| pub fn objects_in(&self, path: &str) -> Result<Vec<KernelObject>, ObjectError> { | ||
| let directory = match path.trim_end_matches('\\') { | ||
| "" => self.pointer_at(self.globals.root)?, | ||
| path => self.object_at(path)?.address, |
There was a problem hiding this comment.
When objects_in is called with a valid path to a leaf such as \Device\MountPointManager, object_at returns that device and this line passes its body directly to named_in. The walker then interprets the first 37 pointer-sized fields of a device object as directory buckets, potentially following arbitrary kernel pointers instead of returning NotADirectory; the final resolved object's type must be checked before enumeration.
Useful? React with 👍 / 👎.
| if object != 0 { | ||
| out.push(object); | ||
| } |
There was a problem hiding this comment.
The loop bound is based on out.len(), but that counter advances only when an entry's Object pointer is non-null. On a corrupt target where a null-object entry links to itself—or merely a chain containing more than the configured limit of null entries—the loop never reaches TooMany and can hang the debugger indefinitely, contrary to the stated corruption bound; count every traversed chain link rather than only emitted objects.
Useful? React with 👍 / 👎.
| if length == 0 || buffer == 0 { | ||
| return Ok(String::new()); |
There was a problem hiding this comment.
A UNICODE_STRING with a nonzero Length and null Buffer is malformed, but this branch returns a successful empty string. In particular, link_target validates the nonzero even length and maximum first, then reaches this path and reports Ok(""), turning corrupt target data into a valid symbolic-link target instead of an ObjectError.
Useful? React with 👍 / 👎.
…llows Four review findings, each pinned by breaking the rule it is for. **The security descriptor field is an `_EX_FAST_REF`.** The object manager keeps a count of outstanding fast references in the bits an aligned address leaves spare, and that count is **four** bits on a 64-bit kernel -- measured, `nt!_EX_FAST_REF::RefCnt` is `Pos 0, 4 Bits` on 26100 x64. Clearing three of them left the fourth standing on any object with eight or more live references, and the address handed back was then eight bytes into the descriptor: what a caller decodes from there is a DACL read out of the middle of a header, which is a wrong answer about who may open a device rather than a failure to answer. The mask is derived from the pointer width the layout already derives, since the count fills what the descriptor's alignment leaves. **A bound counts the links it follows, not the objects it emits.** An entry whose `Object` is null contributes nothing to the list, so a bound counting the list never reached one -- and one of those pointing at itself is a loop the walk never leaves. That is not a test that fails, it is a debugger that stops answering, and the mutation for it hangs rather than going red. **Listing a leaf is refused rather than enumerated.** The guard against walking *through* a device existed; the same guard at the *end* of a path did not, so `objects_in` on a device read a driver's own fields as thirty-seven bucket pointers and followed whatever they held. **A length with no buffer is malformed, not empty.** Answered as an empty string it becomes a symbolic link whose target is `""`, which a caller publishes as a device reachable under no name at all. One finding is **declined**, and the reason belongs here because the next round will raise it against code that by then looks deliberate: the walk was said to ask for `nt!ObpTypeIndexTable`, with the extra `p`, and to lose every type name to the `.ok()` that follows. It asks for `nt!ObTypeIndexTable` and always has. `x nt!ObpTypeIndexTable` resolves nothing on the checked-in dump; `x nt!ObTypeIndexTable` resolves to `fffff805 cc3c6800`. Nothing changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0399913985
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if length == 0 || !length.is_multiple_of(2) || length > maximum { | ||
| return Err(ObjectError::Malformed { | ||
| reason: "the link target is not a string this can vouch for", | ||
| }); | ||
| } |
There was a problem hiding this comment.
When a callback-backed symbolic-link object has a callback address whose low words happen to form a nonzero even Length and a larger MaximumLength, this heuristic accepts the callback arm and unicode_at decodes bytes from CallbackContext as a link destination. Filtering by type_name cannot prevent this because callback-backed objects also have type SymbolicLink; inspect the target's callback/flags discriminator and reject or handle that arm before interpreting the union as a UNICODE_STRING.
Useful? React with 👍 / 👎.
The union `LinkTarget` sits in is not discriminated by the object's type: a callback-backed symbolic link is a `SymbolicLink` like any other, so the doc comment telling a caller to filter on `type_name` first was pointing at a gate that is not there. `Callback` lands exactly on `Length` and `MaximumLength` and `CallbackContext` lands on `Buffer`, so a code address whose low half happens to be an even, non-zero, in-range length gets past checks that ask only whether this is a string -- and what gets decoded is whatever the context pointer addresses. So the check is now what a **link target** is rather than what a string is: the lengths have to be a string's, and then the thing they describe has to be an object path, which begins at the root with a backslash. The test that was here covered an address the lengths caught; the case that matters is the one they do not, and it is now beside it. **The discriminating flag is deliberately not read**, and this is where the reason goes. `_OBJECT_SYMBOLIC_LINK::Flags` is what the object manager itself branches on, and which bit that is could not be measured here: local kernel debugging is not enabled on this host (`attach_kernel_local` answers `0x80004001`), and a kernel minidump carries no namespace at all. A bit taken from reading about it rather than from a target would be a rule nothing checked, which is the shape of mistake this crate keeps finding in review. Refusing a callback link cannot be wrong in the direction that matters. Reading the flag would let one be answered instead, and that is worth doing from a live kernel rather than from memory. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7ed948722
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if found | ||
| .type_name | ||
| .as_deref() | ||
| .is_some_and(|kind| kind != DIRECTORY) |
There was a problem hiding this comment.
When either optional type global is unavailable, or any read in type_of fails, type_name is None and this predicate permits enumeration. Calling objects_in on a leaf in that supported configuration then interprets the object's body as directory buckets, potentially returning a false empty result or following arbitrary pointers; require Some("Directory") before passing the address to named_in (and apply the same fail-closed rule to intermediate components).
Useful? React with 👍 / 👎.
| let target = self.unicode_at(at)?; | ||
| if !target.starts_with('\\') { | ||
| return Err(ObjectError::Malformed { | ||
| reason: "the link target is not an object path, so this is not a target", | ||
| }); | ||
| } |
There was a problem hiding this comment.
Fresh evidence is the newly added starts_with('\\') gate: it still cannot distinguish the union arms when a callback address's low words satisfy the length checks and its CallbackContext points to UTF-16 data beginning with a backslash. In that case this returns arbitrary callback context as a symbolic-link destination, so the link's flags/discriminator must be checked rather than inferring the active arm from its contents.
Useful? React with 👍 / 👎.
| let length = u16::from_le_bytes( | ||
| bytes[self.layout.unicode_length as usize..][..2] | ||
| .try_into() | ||
| .unwrap_or_default(), | ||
| ) as usize; |
There was a problem hiding this comment.
For object and type names, unicode_at reads Length but ignores the adjacent MaximumLength. A corrupt or torn UNICODE_STRING with Length > MaximumLength is therefore accepted and bytes beyond the declared buffer can become part of a name used for path matching, potentially resolving the wrong object instead of returning Malformed; validate the two fields before reading the buffer.
Useful? React with 👍 / 👎.
…not tell says no
Three findings, and the first sent me to read the kernel rather than argue with
the reviewer.
**The link union's discriminator is `Flags & 10h`**, measured out of
`nt!ObpParseSymbolicLinkEx` on 26100 x64: it loads `Flags`, tests `10h`, and on
that branch calls through the pointer at `+0x08` with the context at `+0x10`;
with the bit clear it reads `+0x08` as the `_UNICODE_STRING`. That is now read
first, so a callback-backed link is *answered* -- it has no target -- rather than
refused for failing a string check.
Worth recording why this was measured rather than looked up: the neighbouring
bits are all something else. `2h` asks whether the token is sandboxed, `8h`
masks an access mask, `1h` is a silo check. `2h` is the one a reader would try
first, and it is the wrong one.
The content checks stay, because they are cheap and they are what the kernel
itself does two blocks earlier -- `cmp word ptr [rax],5Ch`, the same backslash.
They are evidence now rather than the answer.
**A guard that cannot tell says no.** Both directory guards read
`type_name.is_some_and(...)`, which passes an object whose type could not be
read -- and the guard exists to stop a device's own fields being read as
thirty-seven bucket pointers, so passing on doubt is passing on exactly the case
it is for. Both now require `Some("Directory")`. That makes the type load-bearing,
so `ObHeaderCookie` and `ObTypeIndexTable` stop being optional globals: a walk
that cannot name a type cannot make the guarantee, and saying so at
`object_globals` is better than a guard that silently opens.
**A string is checked against its own maximum.** `Length` past `MaximumLength` is
a torn structure, and reading the length alone takes whatever follows into a name
the walk then matches paths against -- which resolves some *other* object rather
than failing to resolve this one.
Each of the three is pinned by breaking it, one at a time. The `maximum` mutation
also found a bug in my own harness: restoring a rule whose broken form is the
empty string prepends it to the file rather than putting it back, which
`cargo fmt` caught as a parse error at line 1.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 419320414d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| header_cookie: self.symbol_offset("nt!ObHeaderCookie")?, | ||
| type_index_table: self.symbol_offset("nt!ObTypeIndexTable")?, |
There was a problem hiding this comment.
On a target/PDB where ObHeaderCookie or ObTypeIndexTable cannot be resolved but the root and name metadata are available, these ? operators make object_globals fail before every with_namespace call. This unnecessarily disables safe operations that do not descend through an object—such as resolving a one-component path, listing \, or reading a symbolic-link target—and contradicts the documented type_name: None fallback; resolve these globals optionally and let the existing directory guards fail closed only when traversal actually requires a known type.
Useful? React with 👍 / 👎.
| fn utf16(bytes: &[u8]) -> String { | ||
| let (pairs, _) = bytes.as_chunks::<2>(); | ||
| let units: Vec<u16> = pairs.iter().copied().map(u16::from_le_bytes).collect(); | ||
| String::from_utf16_lossy(&units) |
There was a problem hiding this comment.
When the namespace contains a name with an unpaired UTF-16 surrogate, from_utf16_lossy replaces it with U+FFFD, and object_at subsequently uses that lossy String as the object's identity. A caller querying a literal U+FFFD can therefore resolve an object whose actual name differs, and multiple distinct non-scalar names can collapse to the same value so .find() returns whichever appears first; preserve the original code units for matching or reject such names rather than matching on the display representation.
Useful? React with 👍 / 👎.
… identity Two findings, and the first is a round trip: the previous round made the type globals required so the directory guards could fail closed, and this one asks for them back because resolving a one-component path, listing the root and reading a link target need no type at all. Both rounds were right about different things, and neither position was the answer. What was wrong underneath is that the guard had one refusal for two facts. An object this walk could not type is not an object that *is* something else, and reporting a directory as `NotADirectory` because the type table would not read is a lie the caller would act on. So `Untyped` is its own variant, the globals go back to optional, and the guards still fail closed -- saying which of the two it is. A target that cannot name types now answers everything that needs no type and refuses, accurately, the one thing that does. **And a rendering is not an identity.** An unpaired surrogate is a legal object name and an illegal `String`, so `from_utf16_lossy` turns it into a replacement character: two different names render alike, and a caller asking for the replacement character reaches an object whose name contains no such character. For a walk whose answer feeds a security question, that is a device answering under a name that is not its own -- and it is a name a driver chooses, not one it is stuck with. The name is still rendered, because an object nobody can list is worse than one nobody can resolve by name, and `KernelObject::exact_name` says which it is. `object_at` will not match an object it cannot reproduce exactly. A link target that is not exactly its own bytes is refused outright, since there is no listing to preserve there. This round's harness refuses a mutation that maps a rule to the empty string, which is how the last one corrupted the file while restoring. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a5d8719a6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reason: "the link target is not a string this can vouch for", | ||
| }); | ||
| } | ||
| let (target, exact) = self.unicode_at(at)?; |
There was a problem hiding this comment.
When a valid symbolic link has a target longer than 1,024 bytes, this call routes it through unicode_at, which applies MAX_NAME_BYTES and returns TooMany { what: "an object name", ... }. A link target is a separate counted UNICODE_STRING and can exceed the crate's deliberately conservative object-name limit, so symbolic_link_target rejects otherwise valid long targets; pass an operation-specific bound or decode link targets separately.
Useful? React with 👍 / 👎.
| pub fn symbolic_link_target(&self, link: u64) -> Result<String, ObjectError> { | ||
| self.with_namespace(|namespace| namespace.link_target(link)) |
There was a problem hiding this comment.
When the target exposes the symbolic-link and UNICODE_STRING layouts but omits or renames an unrelated namespace global such as ObpInfoMaskToOffset, symbolic_link_target still enters with_namespace, whose mandatory object_globals call fails before any link bytes are read. Namespace::link_target never uses the root, name-info table, type table, or directory layout, so this standalone operation should resolve only its own required fields instead of becoming unavailable whenever namespace-walking metadata is incomplete.
Useful? React with 👍 / 👎.
…ded as a path Two findings, and the second is the third round on one seam: an operation requiring more than it uses. Round three made the type globals required so the directory guards could fail closed; round three's own finding took them back for the operations that never descend; this one points out that reading a symbolic link still needed the root pointer and the optional-header table, neither of which it touches. So the seam goes rather than the symptom. **Every global is optional at resolution and required at use.** `Globals` records what the target has; `needs` asks for one where it is read and names it in the refusal. Walking a path wants the root, reading a name wants the offsets table, naming a type wants the cookie and the table, and reading a link wants none of them -- which it now demonstrably does, on a target that resolves nothing at all. The layout is deliberately *not* treated this way, and the comment says so, so the next round has the answer rather than the question: every type in it comes out of one PDB, so a partial answer there is not something a real target produces, while a global is a symbol that a build can rename. **And a link target is bounded as a path rather than as a name.** It was sharing `MAX_NAME_BYTES`, which is sized for one path *component* -- so an ordinary long target was refused, with a message about object names, which is the tell that the bound belonged to something else. One note on method. The first mutation I wrote for the globals rule passed, and it deserved to: it added a requirement inside a helper the operation under test never calls, so it changed a path the test does not take. The rule is "reading a link needs none of these", and the mutation that states it puts the requirement in `link_target` itself. That one fails, which is what makes the test worth having. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d0cf8baa2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reason: "the link target is not text, so it is not a path to follow", | ||
| }); | ||
| } | ||
| if !target.starts_with('\\') { |
There was a problem hiding this comment.
When a valid symbolic-link destination is a DOS or relative string rather than an object-manager path—for example, a KnownDllPath value such as C:\Windows\System32—this gate returns Malformed even though the preceding flag check established that the union contains a UNICODE_STRING. The new Flags discriminator is fresh evidence that the content heuristic discussed in the earlier callback finding is no longer needed; symbolic_link_target should return the decoded target regardless of whether it begins with \.
Useful? React with 👍 / 👎.
| 4 => u64::from(u32::from_le_bytes( | ||
| bytes[..4].try_into().unwrap_or_default(), | ||
| )), | ||
| _ => u64::from_le_bytes(bytes[..8].try_into().unwrap_or_default()), |
There was a problem hiding this comment.
Because both Namespace::new and every Layout field are public, a downstream memory walker can supply a width other than 4 or 8; for widths below 8, read returns only that many bytes and this fallback slices bytes[..8], panicking inside otherwise fallible object_at/objects_in calls. Validate the layout or return ObjectError::Malformed for unsupported widths rather than treating every non-4 value as 8.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
…layout panics Two findings, and the first retires something I defended one round ago. **A target is returned whatever it looks like.** This required a leading backslash, which refuses `\KnownDlls\KnownDllPath` -- a real link whose target is the DOS path `C:\Windows\System32`. That rule was a stand-in for a discriminator, and it stopped being worth anything the moment there was a flag to ask: once `Flags` says the union holds a `UNICODE_STRING`, what is in it *is* the target and this has no business second-guessing it. What remains is structural -- a whole number of UTF-16 units, within its own maximum, addressing a buffer that reads. The justification I gave for keeping it was also wrong, and the correction is in the prose so the next reader has it: `ObpParseSymbolicLinkEx` does test a leading backslash two blocks before the flag, and it is testing the **remaining name** being parsed rather than the target. Reading one as the other is how a real link came to be refused. **And nothing panics on a layout this crate did not build.** `Layout` is public and so is `Namespace::new`, so every offset in it is a caller's to fill in and every one is an index into bytes this walk read. A pointer width of two had two bytes read and eight taken; a string field placed past the end of its structure is the same fault by another route. Both were a panic inside calls whose whole contract is that they return an error. The width is now refused rather than treated as eight, and field reads are bounds-checked against what was actually read -- so the class goes rather than the instance the finding named. Both mutations for that rule panic when backed out rather than going red, which is the defect stating itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eed6699de9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }); | ||
| } | ||
| let at = link.wrapping_add(u64::from(self.layout.link_target)); | ||
| let size = (self.layout.unicode_buffer as usize) + self.layout.pointer; |
There was a problem hiding this comment.
Fresh evidence beyond the earlier pointer-width finding is that link_target performs this unchecked addition before reaching pointer_at's width validation. With the public Layout set to an unsupported large width such as usize::MAX and readable link flags, debug builds panic here while optimized builds wrap the structure size and perform an undersized read; validate that the width is 4 or 8 before any size arithmetic so this fallible API returns Malformed instead.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| fn entries_of(&self, directory: u64) -> Result<Vec<u64>, ObjectError> { | ||
| let mut out = Vec::new(); | ||
| let mut followed = 0usize; | ||
| for bucket in 0..self.layout.buckets { |
There was a problem hiding this comment.
Because callers can construct the public Layout, buckets == 0 makes objects_in("\\") return a false successful empty list, while an arbitrarily large count can keep scanning zero bucket pointers indefinitely because followed advances only for nonzero chain links. Validate the bucket count as nonzero and reasonably bounded before this loop rather than allowing malformed layouts to bypass the existing corruption bound.
Useful? React with 👍 / 👎.
Two more findings on the same seam, which is the signal to stop answering them one at a time. The size arithmetic in `link_target` overflows before the width check it would eventually reach, and a bucket count of zero turns a full directory into an empty list while a huge one keeps the walk reading -- and before those, a width of two had two bytes read and eight taken, and a field offset past the end of its structure was taken from whatever followed. Four defects, one cause: `Layout` is public, so its fields are a caller's to fill in, and every read defended itself against the field it happened to use. So the check moves to `Namespace::new`, which now returns a `Result`. Two kinds of field, and the difference is worth stating rather than checking everything: a **width** or a **count** is what the walk sizes reads and loops from, so a wrong one panics or runs away and is refused. An **offset into a structure the walk reads whole** has to be inside what it reads, or the read succeeds and the field comes from past its end. An offset that is only added to an address is left alone, deliberately -- a wrong one reads somewhere else, which comes back as `Unreadable` naming the address, and there is nothing here to compare it against. The walk below can now rely on the layout, and there is no next field to find. Each of the four checks is pinned by disabling it alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bb493d522
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return bad("a directory's bucket count is not one a directory has"); | ||
| } | ||
| // A `UNICODE_STRING` is read whole: its two lengths, then its buffer. | ||
| let unicode = (self.unicode_buffer as usize) + self.pointer; |
There was a problem hiding this comment.
Fresh evidence after the new Layout::check is that it still accepts enormous public offsets: for example, with pointer == 4, unicode_buffer == u32::MAX - 4, name_info_name == 0, and name_info_size == u32::MAX, unicode is nearly 4 GiB and both checks pass. link_target then requests that entire size, and DebugEngine::read_memory allocates a correspondingly large Vec, potentially aborting on OOM instead of returning ObjectError; on 32-bit builds, still larger offsets can also overflow this addition. Use checked arithmetic and reject structure sizes beyond a small realistic bound.
AGENTS.md reference: AGENTS.md:L24-L24
Useful? React with 👍 / 👎.
| let directory = match path.trim_end_matches('\\') { | ||
| "" => self.pointer_at(self.needs(self.globals.root, ROOT_SYMBOL)?)?, |
There was a problem hiding this comment.
When a caller passes objects_in(""), trim_end_matches produces the same empty value as it does for the canonical root path "\\", so this branch silently lists the root namespace. This bypasses the path validation used by object_at and turns missing or empty CLI/API input into a successful query for a different directory; only an actual root path should take this branch, while "" should return BadPath.
Useful? React with 👍 / 👎.
Two findings, the first of which is a gap in the check I wrote last round rather than a new seam: it verified that the offsets are *ordered* the way a real layout's are and never that they are *small*. A buffer offset of nearly four gigabytes satisfies every ordering and then has the walk ask the target for a read that size, which is an allocation failure rather than a refusal — and on a 32-bit host the sums that check the ordering overflow before they are compared. So every offset is bounded before any of them is added to another. These describe structures of tens of bytes: `_OBJECT_HEADER` is 0x30, a name header 0x20, a `UNICODE_STRING` 0x10. Anything near a `u32`'s range is not one of them. **And the root is `\` and nothing else is.** `objects_in` trimmed trailing backslashes before deciding, which made the empty string the same value as the root — so a caller whose argument went missing got a successful listing of a directory it never asked about. `object_at` had always refused that, so one question had two answers depending on which door it came through. Both are pinned by disabling them alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6660564b7e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let directory = match path.trim_end_matches('\\') { | ||
| "" => self.pointer_at(self.needs(self.globals.root, ROOT_SYMBOL)?)?, |
There was a problem hiding this comment.
Fresh evidence beyond the earlier empty-path case is that objects_in("\\\\") still passes the prefix check and trim_end_matches('\\') reduces it to "", so it lists the root even though the documented root path is exactly "\\"; object_at rejects the same input as BadPath. Check for the exact root spelling before trimming trailing separators so malformed CLI/API input cannot resolve to a different directory.
Useful? React with 👍 / 👎.
…ropped Third disagreement between two path parsers, so the second one goes. `objects_in` had its own handling and it was lenient in a way `object_at` was not: the empty string listed the root, then a path of nothing but separators listed the root, because trimming trailing backslashes turns both into the same value. Each was fixed where it was found. The parser they were supposed to share was lenient too -- it *filtered* empty components rather than refusing them, so `\Device\X` quietly became `\Device\X` and `\` quietly became the root. So `components_of` refuses an empty component, answers the root as a path with no components in it, and both callers ask it. `object_at` refuses that answer because the root is a directory rather than an object in one; `objects_in` is the call that means it. One trailing separator stays a caller's convenience, which is the one piece of leniency that was deliberate. The test now walks every spelling that used to reach the root by a different route, and asserts the two calls agree on each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
…ete one Two findings on #159, both correct. **The halt was polled inside a chain and not before a bucket.** An empty bucket never enters that loop, so a directory that is empty -- or merely has a long run of empty buckets -- was unstoppable: `halting(&|| true)` still paid one target read per bucket, up to the thousand a layout may declare, and then answered `halted: false` having done all of it. It is polled before the read now. The test gains a third construction for it, because the two it had could not reach this one: an empty directory enters no chain and names no entry, so the bucket poll is the only one that can fire there. **And `skipped()` was being read as a completeness test it never was.** It counts entries the walk *reached* and could not present, so a walk stopped before reading any of them returns zero while missing an unknown number -- which a caller branching on `skipped() == 0` reads as the whole directory, the one reading these counts exist to prevent. `is_complete()` is the question to ask, and `skipped()` now says in its own docs that it is not that question. Both mutation-verified. Declined: renaming the new test to `test_*` per `AGENTS.md`. The convention is real and this module does not follow it -- `src/object.rs` has twenty-four descriptively named tests and no `test_`-prefixed one, merged that way across #157 and #158. Renaming one would make it the only exception in the file, which is worse than either consistency; renaming all twenty-four is a change of its own and not this PR's subject. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
…ete one Two findings on #159, both correct. **The halt was polled inside a chain and not before a bucket.** An empty bucket never enters that loop, so a directory that is empty -- or merely has a long run of empty buckets -- was unstoppable: `halting(&|| true)` still paid one target read per bucket, up to the thousand a layout may declare, and then answered `halted: false` having done all of it. It is polled before the read now. The test gains a third construction for it, because the two it had could not reach this one: an empty directory enters no chain and names no entry, so the bucket poll is the only one that can fire there. **And `skipped()` was being read as a completeness test it never was.** It counts entries the walk *reached* and could not present, so a walk stopped before reading any of them returns zero while missing an unknown number -- which a caller branching on `skipped() == 0` reads as the whole directory, the one reading these counts exist to prevent. `is_complete()` is the question to ask, and `skipped()` now says in its own docs that it is not that question. Both mutation-verified. Declined: renaming the new test to `test_*` per `AGENTS.md`. The convention is real and this module does not follow it -- `src/object.rs` has twenty-four descriptively named tests and no `test_`-prefixed one, merged that way across #157 and #158. Renaming one would make it the only exception in the file, which is worse than either consistency; renaming all twenty-four is a change of its own and not this PR's subject. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
1 participant
\Device\MountPointManageris not an address, and nothing indbgengcould turn it into one: a symbol names what the linker placed, while an object is created at run time and filed under a name in a tree the object manager keeps. The debugger's own answer is!object, an extension printing text. This walk answers in values.It exists for a consumer:
windbg-mcp'sdevice_securityneeds a device's security descriptor, that pointer lives in the object's header, and the header is only reachable once the name has been resolved.Surface
object_at(path)resolves a path to the object filed under it.objects_in(path)lists a directory.symbolic_link_target(link)reads where a link points.object_layout()andobject_globals()are public so a caller can see what the walk resolved.Three decisions worth reviewing
Every bucket is walked rather than the one the name hashes to. The hash is the object manager's own, over a name folded with the kernel's upcase table. A wrong reimplementation of it does not fail — it looks in the wrong bucket and reports the object as absent, which is an answer a caller acts on. A directory holds tens of entries.
Not one literal offset, and two things that look like constants are derived. A pointer's width is the distance between a directory entry's two pointer fields, so a 32-bit kernel read from a 64-bit host is not decoded with the host's width. The bucket count is the array's span over that width, because 37 is this build's number and not the structure's.
A cap is an error rather than a short list. A directory reported shorter than it is answers "which symbolic links reach this device" wrongly, which is the one direction a security question must not fail in.
What the tests pin, each verified by breaking the rule it is for
TypeIndexis xored with the cookie and a byte of the header's addressLinkTargetshares storage with a callback pointer —Callbacklands onLengthandMaximumLength,CallbackContextlands onBuffer— and the fixture lays that union out as it really is rather than relying on a read that happens to fail.Where it does not work
Not on a kernel minidump. Measured against the consumer's checked-in dump:
nt!ObpRootDirectoryObjectitself reads????????, so the walk stops at its first read and names the address rather than reporting an empty namespace. A live kernel is this code's tier, and the unit tests run against synthetic bytes because there is no other way to test a walk over a running kernel.Structure offsets were measured from the PDB on Windows 10 26100 x64 before anything was written.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf