Key the recursive-glob visited set on (device, inode) - #9010
Merged
Conversation
isDirVisited() remembers directories by st_ino alone. Inode numbers are only unique within a filesystem, so once a recursive pattern crosses a mount point two unrelated directories can look like the same one and the second gets skipped along with its whole subtree. No error, the rows just aren't there. Every ext2/3/4 root directory is inode 2, so two such mounts under one ** pattern hit this every time. The set was also unordered_set<int> while ino_t is 64-bit, so inodes that differ only above bit 32 collide as well - that one needs just one filesystem, e.g. XFS with inode64 or a large btrfs. std::set<std::pair<dev_t, ino_t>> fixes both. Kept std::set rather than unordered_set because std::pair has no default hash. npm_packages.cpp carries a copy of the same function with the same two problems, so it's fixed here too. Signed-off-by: Cole Munz <colemunz@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes recursive-glob directory loop detection by keying the visited-directory set on (st_dev, st_ino) instead of st_ino alone, preventing silent subtree pruning when inode numbers collide across different mounted filesystems and avoiding inode narrowing collisions.
Changes:
- Update recursive-glob traversal in
osquery/filesystem/filesystem.cppto track visited directories using(device, inode)pairs. - Apply the same visited-directory fix to the similar traversal logic in
osquery/tables/system/npm_packages.cpp. - Replace the visited set implementation from
unordered_set<int>to an ordered set of(dev_t, ino_t)identifiers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| osquery/filesystem/filesystem.cpp | Fixes recursive glob directory loop detection by tracking visited directories as (device, inode) pairs. |
| osquery/tables/system/npm_packages.cpp | Aligns npm package directory traversal loop protection with (device, inode) visited identifiers. |
Comments suppressed due to low confidence (1)
osquery/filesystem/filesystem.cpp:16
- This file now refers to
dev_t/ino_t(viaDirIdentifier) but does not include<sys/types.h>. Relying on<sys/stat.h>to pull in these typedefs is not portable and can break builds on some platforms/toolchains. Add<sys/types.h>explicitly.
#include <codecvt>
#include <set>
#include <sstream>
#include <fcntl.h>
#include <sys/stat.h>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+12
to
16
| #include <set> | ||
| #include <stdlib.h> | ||
| #include <string> | ||
| #include <sys/stat.h> | ||
| #include <unordered_set> | ||
| #include <utility> |
zwass
approved these changes
Jul 29, 2026
zwass
left a comment
Member
There was a problem hiding this comment.
The fix makes sense to me. Thank you!
pascalrobert
pushed a commit
to CollabInfra/osquery
that referenced
this pull request
Jul 29, 2026
Fixes osquery#8992. `isDirVisited()` remembers directories by `st_ino` alone. Inode numbers are only unique within a filesystem, not globally, so as soon as a recursive pattern crosses a mount point two unrelated directories can look like the same one — and the second gets skipped along with everything under it. There's no error; the rows just aren't there. Every ext2/3/4 root directory is inode 2, so two ext4 mounts under one `**` pattern hit this every time. Reproducing it doesn't need loop devices or root: ``` unshare -Urm --propagation private bash -c ' mount -t tmpfs tmpfs ./fa; mount -t tmpfs tmpfs ./fb touch ./fa/canary_a ./fb/canary_b stat -c "%n dev=%D ino=%i" ./fa ./fb' ./fa dev=3b ino=1 ./fb dev=3c ino=1 ``` Different devices, same inode number. Globbing both together returns `fb/` and nothing beneath it; globbing `fb` on its own returns everything. The set was also `unordered_set<int>` while `ino_t` is 64-bit here, so inodes differing only above bit 32 collide too — that one doesn't need two filesystems, just XFS with `inode64` or a large btrfs: ``` sizeof(ino_t)=8 sizeof(int)=4 emplace(0x100000002) inserted=1 emplace(0x200000002) inserted=0 <- distinct inodes, treated as already visited ino 4294967295 stored as int -> -1 ``` `std::set<std::pair<dev_t, ino_t>>` fixes both. I kept `std::set` rather than `unordered_set` because `std::pair` has no default hash, and this set stays small. `npm_packages.cpp` has a copy of the same function with both problems, so I fixed it there as well — the issue only mentions `filesystem.cpp`, but the defect is identical. Symlink-loop protection still works: with the change, a `link -> parent` symlink is listed once and not descended into, which is what the set was added for. On testing — I couldn't find a way to express this in `osquery/filesystem/tests/filesystem.cpp`, since the cross-device case needs mount privileges and the narrowing case needs control over inode allocation. I verified the traversal behaviour by driving the real `isDirVisited` logic over the two tmpfs mounts above, before and after. Happy to add a test if you can point me at a pattern for it. Signed-off-by: Cole Munz <colemunz@gmail.com>
pascalrobert
pushed a commit
to CollabInfra/osquery
that referenced
this pull request
Aug 4, 2026
Fixes osquery#8992. `isDirVisited()` remembers directories by `st_ino` alone. Inode numbers are only unique within a filesystem, not globally, so as soon as a recursive pattern crosses a mount point two unrelated directories can look like the same one — and the second gets skipped along with everything under it. There's no error; the rows just aren't there. Every ext2/3/4 root directory is inode 2, so two ext4 mounts under one `**` pattern hit this every time. Reproducing it doesn't need loop devices or root: ``` unshare -Urm --propagation private bash -c ' mount -t tmpfs tmpfs ./fa; mount -t tmpfs tmpfs ./fb touch ./fa/canary_a ./fb/canary_b stat -c "%n dev=%D ino=%i" ./fa ./fb' ./fa dev=3b ino=1 ./fb dev=3c ino=1 ``` Different devices, same inode number. Globbing both together returns `fb/` and nothing beneath it; globbing `fb` on its own returns everything. The set was also `unordered_set<int>` while `ino_t` is 64-bit here, so inodes differing only above bit 32 collide too — that one doesn't need two filesystems, just XFS with `inode64` or a large btrfs: ``` sizeof(ino_t)=8 sizeof(int)=4 emplace(0x100000002) inserted=1 emplace(0x200000002) inserted=0 <- distinct inodes, treated as already visited ino 4294967295 stored as int -> -1 ``` `std::set<std::pair<dev_t, ino_t>>` fixes both. I kept `std::set` rather than `unordered_set` because `std::pair` has no default hash, and this set stays small. `npm_packages.cpp` has a copy of the same function with both problems, so I fixed it there as well — the issue only mentions `filesystem.cpp`, but the defect is identical. Symlink-loop protection still works: with the change, a `link -> parent` symlink is listed once and not descended into, which is what the set was added for. On testing — I couldn't find a way to express this in `osquery/filesystem/tests/filesystem.cpp`, since the cross-device case needs mount privileges and the narrowing case needs control over inode allocation. I verified the traversal behaviour by driving the real `isDirVisited` logic over the two tmpfs mounts above, before and after. Happy to add a test if you can point me at a pattern for it. Signed-off-by: Cole Munz <colemunz@gmail.com>
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
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.
Fixes #8992.
isDirVisited()remembers directories byst_inoalone. Inode numbers are only unique within a filesystem, not globally, so as soon as a recursive pattern crosses a mount point two unrelated directories can look like the same one — and the second gets skipped along with everything under it. There's no error; the rows just aren't there. Every ext2/3/4 root directory is inode 2, so two ext4 mounts under one**pattern hit this every time.Reproducing it doesn't need loop devices or root:
Different devices, same inode number. Globbing both together returns
fb/and nothing beneath it; globbingfbon its own returns everything.The set was also
unordered_set<int>whileino_tis 64-bit here, so inodes differing only above bit 32 collide too — that one doesn't need two filesystems, just XFS withinode64or a large btrfs:std::set<std::pair<dev_t, ino_t>>fixes both. I keptstd::setrather thanunordered_setbecausestd::pairhas no default hash, and this set stays small.npm_packages.cpphas a copy of the same function with both problems, so I fixed it there as well — the issue only mentionsfilesystem.cpp, but the defect is identical.Symlink-loop protection still works: with the change, a
link -> parentsymlink is listed once and not descended into, which is what the set was added for.On testing — I couldn't find a way to express this in
osquery/filesystem/tests/filesystem.cpp, since the cross-device case needs mount privileges and the narrowing case needs control over inode allocation. I verified the traversal behaviour by driving the realisDirVisitedlogic over the two tmpfs mounts above, before and after. Happy to add a test if you can point me at a pattern for it.