fix: one entry a directory cannot name does not take the directory with it - #158
Merged
Merged
Conversation
…th it Found by running the namespace walk against a live kernel rather than against fixtures. On a Windows Server 26100 guest, `\GLOBAL??` holds some two hundred symbolic links and one whose `Name.Buffer` is not resident. The debugger's own `!object` prints it as `(*** Name not accessible ***)` and carries on; this walk propagated the read failure and lost the whole directory. **And it lost more than the listing.** `object_at` resolves every path component through the same enumerator, so any lookup whose path crossed that directory failed too -- `\GLOBAL??\MountPointManager` did not resolve on a machine where it plainly exists. A consumer asking "what reaches this device" was told "this directory cannot be listed", which reads as a device nothing reaches. An entry this cannot present is now skipped and counted. Three things about how, each of which is a rule with a test that fails without it. **The counts are two, not one.** A name paged out is a page that will be back; a name longer than its own maximum is an entry the object manager cannot have written. `Listing` carries `unreadable` and `malformed` separately, with `skipped()` for the callers that only need to know the list is short. Reporting a torn entry as "unreadable" would hand a reader the benign reading of the alarming case. **Only errors about the entry are skipped.** `Unavailable` says this *target* does not resolve `nt!ObpInfoMaskToOffset`, which is equally true of every entry there will ever be, so it stays fatal: swallowing it would answer with an empty directory for a build this walk cannot decode at all. That rule was **green under the obvious mutation** -- widening the arm to a catch-all -- because the test that looked like it covered it hands the walk an empty target and fails on the root pointer before an entry is reached. `a_global_the_target_lacks_is_not_ swallowed_by_the_skip` is what actually pins it. **A lookup past a skipped entry cannot call a name absent.** A directory with an entry this could not name may hold the very object asked for, so `NotFound` is now only for a directory that read in full, and `NotFoundInPart` carries the count for the case where it did not. The two send a reader opposite ways: correct the name, or try again when the page is in. The torn-name test changed contract and was rewritten rather than deleted. It protected the right thing in the wrong place: the danger is a torn name *resolving* to whatever follows its buffer, and an entry absent from the listing resolves to nothing, so skipping is exactly as safe as refusing and leaves the rest of the directory answerable. It now asserts both halves. 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. |
An assertion message carried fourteen literal spaces mid-sentence, because the script that wrote it passed the Rust `\`-newline through a non-raw Python string, where a trailing backslash is Python's own line continuation. The text is unchanged apart from the whitespace. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
![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: 0d4df0aa40
ℹ️ 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".
Comment on lines
+695
to
+699
| skipped => ObjectError::NotFoundInPart { | ||
| directory: walked.clone(), | ||
| component: component.clone(), | ||
| skipped, | ||
| }, |
There was a problem hiding this comment.
When a lookup misses after name_of classified an entry as Malformed (for example, Length > MaximumLength), this collapses listing.malformed and listing.unreadable into one count and returns NotFoundInPart, whose message says the entries could not be read and whose documentation directs callers to retry when the page is resident. That reports transient paging for structural corruption, despite Listing separating these cases because they require opposite responses; carry both counts or use distinct lookup errors here.
Useful? React with 👍 / 👎.
glslang
added a commit
to glslang/windbg-mcp
that referenced
this pull request
Sep 13, 2026
Picks up the whitespace fix on that branch. Repoint at `main` once glslang/dbgscope#158 merges — and check rather than assume, since a rebase-merge lands its commits under new SHAs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
Round one on #158. The finding is right and it is against this PR's own argument: `Listing` carries `unreadable` and `malformed` apart because the two need opposite responses, and then `NotFoundInPart` summed them and said the entries "could not be read" -- which reports an entry the object manager cannot have written as a page that will be back, and sends a reader away to retry something that will never succeed. It carries both counts now, and the message names both. The test that pinned the variant covered the malformed path only; the paged-out path is asserted beside it, and the collapse fails it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
glslang
added a commit
that referenced
this pull request
Sep 13, 2026
…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
glslang
added a commit
that referenced
this pull request
Sep 13, 2026
…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
glslang
added a commit
to glslang/windbg-mcp
that referenced
this pull request
Sep 13, 2026
Picks up the whitespace fix on that branch. Repoint at `main` once glslang/dbgscope#158 merges — and check rather than assume, since a rebase-merge lands its commits under new SHAs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by running the merged namespace walk against a live kernel rather than against fixtures.
On a Windows Server 26100 guest,
\GLOBAL??holds some two hundred symbolic links and one whoseName.Bufferis not resident. The debugger's own!objectprints it as(*** Name not accessible ***)and carries on; this walk propagated the read failure and lost the whole directory.It lost more than the listing.
object_atresolves every path component through the same enumerator, so any lookup whose path crossed that directory failed too —\GLOBAL??\MountPointManagerdid not resolve on a machine where it plainly exists. The consumer asking "what reaches this device" was told "this directory cannot be listed", which reads as a device nothing reaches.An entry this cannot present is now skipped and counted. Three rules, each with a test that fails without it.
The counts are two, not one. A name paged out is a page that will be back; a name longer than its own maximum is an entry the object manager cannot have written.
Listingcarriesunreadableandmalformedseparately, withskipped()for callers that only need to know the list is short. Reporting a torn entry as "unreadable" hands a reader the benign reading of the alarming case.Only errors about the entry are skipped.
Unavailablesays the target does not resolvent!ObpInfoMaskToOffset, which is equally true of every entry there will ever be, so it stays fatal. That rule was green under the obvious mutation — widening the arm to a catch-all — because the test that looked like it covered it hands the walk an empty target and fails on the root pointer before an entry is reached.a_global_the_target_lacks_is_not_swallowed_by_the_skipis what actually pins it.A lookup past a skipped entry cannot call a name absent.
NotFoundis now only for a directory that read in full;NotFoundInPartcarries the count otherwise. The two send a reader opposite ways: correct the name, or try again when the page is in.The torn-name test changed contract and was rewritten rather than deleted. It protected the right thing in the wrong place: the danger is a torn name resolving to whatever follows its buffer, and an entry absent from the listing resolves to nothing — so skipping is exactly as safe as refusing and leaves the rest of the directory answerable.
Verification
object::tests, 238 in the suite,cargo fmt --all --checkclean, no new clippy warnings inobject.rs.windbg-mcp: before, the link search reportedunavailable; after,complete, with\GLOBAL??\MountPointManagerfound and the entry count matching!objectexactly.windbg-mcppins this branch commit meanwhile; its PR repoints atmainonce this merges.🤖 Generated with Claude Code
https://claude.ai/code/session_01Ayv1beKpAVkmDJDLfYqoyf