glob: fix fd leak on NAMETOOLONG and remove no-op arena frees#29380
Conversation
- transitionToDirIterState: close work_item.fd before returning NAMETOOLONG, matching the cleanup pattern used in the other error paths in this function. - prepareMatchedPath/appendMatchedPath: remove arena.allocator().free() calls on the duplicate-match path. These are no-ops on an arena, and one of them ran before a log statement that still referenced the freed buffer. - Remove redundant dupeZ of an already-arena-backed path on the lstat error path.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughModified error handling and file descriptor lifecycle management in glob walker. Removed unnecessary path duplication in error path handling, added conditional file descriptor closure for path length errors, and changed duplicate matched-path bookkeeping to retain instead of free newly created strings. Changes
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
@robobun adopt |
|
✅ Adopted. Local: CI #45947: all Linux (incl. ASAN) + Windows x64 green. Remaining reds are pre-existing on the base commit — #29358 merged with the same On testability: the Approved by Jarred; no open review threads. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — three targeted fixes with no side effects.
Extended reasoning...
Overview
Single-file PR touching src/glob/GlobWalker.zig. Three independent cleanups: an fd leak on the NAMETOOLONG error path, removal of no-op ArenaAllocator.free() calls (one of which preceded a log statement still referencing the buffer), and elimination of a redundant dupeZ on a path that was already null-terminated and arena-backed.
Security risks
None. No auth, crypto, or permission-sensitive code paths are touched.
Level of scrutiny
Low. Each change is a contained, mechanically verifiable fix with no branching logic or design decisions involved. The fd-close pattern matches what the rest of the function already does. The arena-free calls are documented no-ops in Zig's stdlib. The dupeZ removal is safe because path is typed [:0]const u8 at that point.
Other factors
No outstanding reviewer comments. Build CI was triggered. Bug hunting system found nothing. All three changes reduce the gap between intent and implementation without altering observable behaviour in the non-error paths.
…h#29380) Three small cleanups in `GlobWalker.zig`: - `transitionToDirIterState`: close `work_item.fd` before returning `NAMETOOLONG`, matching the cleanup pattern used in the other error paths in this function. - `prepareMatchedPath`/`appendMatchedPath`: remove `arena.allocator().free()` calls on the duplicate-match path. These are no-ops on `std.heap.ArenaAllocator`, and one of them ran *before* a log statement that still referenced the freed buffer. - Remove redundant `dupeZ` of an already-arena-backed path on the lstat error path.
…h#29380) Three small cleanups in `GlobWalker.zig`: - `transitionToDirIterState`: close `work_item.fd` before returning `NAMETOOLONG`, matching the cleanup pattern used in the other error paths in this function. - `prepareMatchedPath`/`appendMatchedPath`: remove `arena.allocator().free()` calls on the duplicate-match path. These are no-ops on `std.heap.ArenaAllocator`, and one of them ran *before* a log statement that still referenced the freed buffer. - Remove redundant `dupeZ` of an already-arena-backed path on the lstat error path.
4 participants
Three small cleanups in
GlobWalker.zig:transitionToDirIterState: closework_item.fdbefore returningNAMETOOLONG, matching the cleanup pattern used in the other error paths in this function.prepareMatchedPath/appendMatchedPath: removearena.allocator().free()calls on the duplicate-match path. These are no-ops onstd.heap.ArenaAllocator, and one of them ran before a log statement that still referenced the freed buffer.dupeZof an already-arena-backed path on the lstat error path.