feat: implement Close, Read, and QueryInfo handlers for E2E file read#14
Open
tejas-claude-bot[bot] wants to merge 11 commits intomainfrom
Open
feat: implement Close, Read, and QueryInfo handlers for E2E file read#14tejas-claude-bot[bot] wants to merge 11 commits intomainfrom
tejas-claude-bot[bot] wants to merge 11 commits intomainfrom
Conversation
Add the server-side handlers needed for a complete file read flow: Create → QueryInfo → Read → Close. Protocol types: - Add accessor methods to SMBCloseRequest, SMBReadRequest, SMBQueryInfoRequest - Add constructors for SMBCloseResponse, SMBReadResponse, SMBQueryInfoResponse - Make close::flags and query_info::info_type modules public Trait extensions: - Add read_data() to ResourceHandle trait with implementations for SMBFileSystemHandle (seeks + reads) and SMBIPCHandle (stub) - Add read_data() to Open trait, delegating to the underlying handle - Add open_table_mut() to Session trait for close cleanup - Add remove_open() to Server trait for global open table cleanup - Fix SMBOpen::inner() todo!() → return None (terminal handler) Handlers (tree_connect.rs): - handle_close: returns file metadata if POSTQUERY_ATTRIB flag set, removes open from server (outer) then session (inner) tables - handle_read: reads data from the open's underlying handle, enforces minimum_count, returns SMBReadResponse - handle_query_info: supports FileBasicInformation (class 4), FileStandardInformation (class 5), FileNetworkOpenInformation (class 34) per MS-FSCC; returns InvalidInfoClass for unsupported types - Fix lock ordering in handle_create: server write before session write Lock ordering: all handlers acquire locks outer→inner (server → connection → session → open), readers before writers. NTStatus: add FileClosed, EndOfFile, InvalidInfoClass, InvalidDeviceRequest Tests: - 13 unit tests for request accessor and response constructor round-trips - 3 integration tests (smbclient E2E: file read, dir listing, missing file)
- Fix SMBCreateRequest NameOffset subtract (68→64): offset is relative to SMB2 header start (64 bytes), not 68. This caused 4-byte misalignment producing leading NUL chars and filename truncation. - Fix SMBInfoType enum values to match MS-SMB2 spec: File=1, Filesystem=2, Security=3, Quota=4 (was incorrectly starting at 0). - Fix SMBQueryInfoResponse StructureSize: 9 per MS-SMB2 2.2.38, not 17. - Add FileAllInformation (class 18) handler in QueryInfo dispatch per MS-FSCC 2.4.2 (composite of Basic+Standard+Internal+EA+Access+Position+ Mode+Alignment+Name). - Fix tree_id in TreeConnect response header (use dynamic ID, not hardcoded 1). - Add SMB_SHARE_PATH env var support in main.rs for configurable share root. - Sanitize file paths in SMBFileSystemShare::handle_create: strip trailing NUL terminators and convert backslashes to forward slashes. - Improve integration test diagnostics: capture smbclient stdout/stderr, verify file contents in file_read test. All 66 unit tests and 10 integration tests pass.
Introduce protocol::body::file_info module with proper typed structs for MS-FSCC file information classes: - FileBasicInformation (2.4.7) - FileStandardInformation (2.4.41) - FileInternalInformation (2.4.20) - FileEaInformation (2.4.12) - FileAccessInformation (2.4.1) - FilePositionInformation (2.4.35) - FileModeInformation (2.4.26) - FileAlignmentInformation (2.4.3) - FileNameInformation (2.4.28) - FileNetworkOpenInformation (2.4.29) - FileAllInformation (2.4.2) All types use SMBFromBytes/SMBToBytes/SMBByteSize derive macros for automatic serialization, replacing manual Vec<u8> byte-pushing in tree_connect.rs build_file_* methods. Includes 14 unit tests (round-trip serialization + size assertions). All 80 unit tests and 10 integration tests pass.
…vention Move each MS-FSCC file information struct out of mod.rs into its own file (access.rs, alignment.rs, basic.rs, ea.rs, internal.rs, mode.rs, name.rs, network_open.rs, position.rs, standard.rs). mod.rs now only contains module declarations, re-exports, and the composite FileAllInformation type. All 80 unit tests and 10 integration tests pass.
…rification
B1: file_id() now uses global_id for persistent (DurableFileId) and
session_id for volatile (FileId) per MS-SMB2 §2.2.14.1 / §3.3.1.10.
B2: handle_close remove_open now passes file_id.persistent (global_id)
to the server's GlobalOpenTable instead of file_id.volatile.
B3: find_open and handle_close now verify Open.DurableFileId ==
FileId.Persistent per MS-SMB2 §3.3.5.10/12/20, returning
STATUS_FILE_CLOSED on mismatch.
B4: handle_create response header now passes 0 (STATUS_SUCCESS) instead
of echoing the request's ChannelSequence into the response Status
field (MS-SMB2 §2.2.1.2).
B6: handle_close logs warn! when server/connection lock acquisition
fails during GlobalOpenTable cleanup.
B7: FileAllInformation::to_bytes() capacity hint now accounts for the
variable-length file name.
Tests: added SMBFileId wire-format unit tests (size, layout, round-trip,
field independence).
…es derive FileAllInformation now uses #[derive(SMBByteSize, SMBFromBytes, SMBToBytes)] with #[smb_direct(start(fixed = N))] annotations for each sub-struct at its computed offset. Removes the manual to_bytes() impl. Adds a round-trip test for FileAllInformation (serialize + deserialize).
- FileAccessInformation.access_flags: u32 → FileAccessFlags bitflags (MS-FSCC §2.4.1 / MS-DTYP §2.4.3 ACCESS_MASK) - FileAlignmentInformation.alignment_requirement: u32 → FileAlignmentRequirement enum (MS-FSCC §2.4.3 device alignment boundaries) - FileModeInformation.mode: u32 → FileModeFlags bitflags (MS-FSCC §2.4.26 file mode flags) FileEaInformation.ea_size remains u32 — it is a plain byte count per MS-FSCC §2.4.12, not flags or an enum. All types exported from file_info module. Tests and tree_connect.rs call site updated to use typed constructors.
Security & correctness fixes: - Fix path traversal vulnerability: normalize_path() rejects "../" escaping the share root (STATUS_ACCESS_DENIED) - Cap read_data buffer to 8 MB (MAX_READ_SIZE) to prevent OOM from malicious clients per MS-SMB2 §3.3.5.12 - Return STATUS_END_OF_FILE when read returns 0 bytes at/past EOF per MS-SMB2 §3.3.5.12 - Enforce OutputBufferLength in handle_query_info: truncate and return STATUS_BUFFER_OVERFLOW per MS-SMB2 §3.3.5.20.1 - Add NTStatus::BufferOverflow (0x80000005) and InfoLengthMismatch (0xC0000004) Encapsulation: - Make SMBFileId fields private with new(), persistent(), volatile() - Make SMBFileMetadata fields private with new() and getters - Make all file_info struct fields private with constructors and getters - FileStandardInformation: accept bool for delete_pending/directory Tests: 95 unit tests pass (15 new: normalize_path, bool getters, FileAllInformation getters). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add missing SMBError import in ipc.rs - Move doc comments inside bitflags! macros - Remove unused SMBByteSize import - Fix unnecessary cast and needless borrow - Allow too_many_arguments on FileAllInformation::new (matches MS-FSCC spec) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
75cdd4e to
2fa3140
Compare
… deduplicate close logic Build SMBCreateResponse after add_open so file_id reflects assigned global_id/session_id instead of zeroes. Replace single file.read() with take()+read_to_end() to handle OS short reads correctly. Refactor handle_close to reuse find_open() instead of duplicating lookup logic. Use try_into() for u64→u32 global_id conversion instead of silent truncation. Replace FileNameInformation::new with from_name() that auto-computes UTF-16 byte length. Fix pre-existing _e/e typos in error logging. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nings SMBByteSize is only used inside trace!() which is a no-op without the logging feature. Similarly, error variables in match arms are only consumed by tracing macros. Gate the import with #[cfg(feature = "logging")] and add #[allow(unused_variables)] to affected match arms. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.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.
Summary
Implements the server-side handlers needed for a complete file read flow: Create → QueryInfo → Read → Close.
Changes
Protocol Types
SMBCloseRequest,SMBReadRequest,SMBQueryInfoRequestSMBCloseResponse,SMBReadResponse,SMBQueryInfoResponseclose::flagsandquery_info::info_typemodules publicTrait Extensions
read_data()toResourceHandletrait with implementations forSMBFileSystemHandle(seeks + reads) andSMBIPCHandle(stub returningInvalidDeviceRequest)read_data()toOpentrait, delegating to the underlying handleopen_table_mut()toSessiontrait for close cleanupremove_open()toServertrait for global open table cleanupSMBOpen::inner()todo!()→ returnsNone(terminal handler)Handlers (
tree_connect.rs)handle_closePOSTQUERY_ATTRIBflag set; removes open from server then session tableshandle_readminimum_count; returnsSMBReadResponsehandle_query_infoFileBasicInformation(class 4),FileStandardInformation(class 5),FileNetworkOpenInformation(class 34) per MS-FSCCLock Ordering
All handlers acquire locks outer → inner (server → connection → session → open), readers before writers. Also fixes lock ordering in the existing
handle_create.NTStatus
Added:
FileClosed,EndOfFile,InvalidInfoClass,InvalidDeviceRequestTests
#[ignore]d) — smbclient E2E: file read, directory listing, nonexistent file error handlingHow to Test