143 binrw structs ut refactor macros #152
Conversation
start testing read+write for all responses (wip)
include both. feature for smb-msg tests
- smb-dtyp - sort some directories, add tests for boolean, rename MultiSz to MultiWSz - smb-fscc - make sure all structs are read+write
- minor bugfix in FileGetEaInformation write
- migrate to new macro in smb-rpc
WalkthroughAdds binrw helpers and types (Boolean, MultiWSz, IoctlBuffer, pos/write helpers), expands create-context machinery and message macro behavior, broadens many derives (PartialEq/Eq), migrates tests to macro-based harnesses, and updates several fscc/query enum/trait bounds and Cargo features. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant CreateCtx as CreateContext machinery
participant Dispatcher as Context dispatcher
participant BinRW as BinRead/BinWrite
Caller->>CreateCtx: receive raw create-context bytes
CreateCtx->>Dispatcher: resolve type/name
Dispatcher->>BinRW: call BinRead::read_options(...) for specific context
BinRW-->>Dispatcher: parsed typed context data
Dispatcher-->>CreateCtx: wrap in CreateContext<T>
CreateCtx-->>Caller: return typed context list
Note right of Dispatcher #f0f4c3: New generic CreateContext<T> and type-dispatch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/smb-msg/src/echo.rs (1)
6-6: Pre-existing typo in public struct name.The struct name
EchoMesasgecontains a typo (should beEchoMessage). While not introduced in this PR, consider addressing this in a future change. Note that fixing it would be a breaking change if external users reference this type.crates/smb-fscc/src/info_classes.rs (1)
14-26: Add ReadEndian and WriteEndian implementations to the macro expansion.The new
FileInfoTypetrait bounds requireReadEndian + WriteEndian(lines 16, 18 of info_classes.rs), but thefile_info_classes!macro only generatesBinReadandBinWriteimpls via#[binrw::binrw]. Since ReadEndian and WriteEndian require manual implementation with aconst ENDIAN: EndianKindfield, all five generated types—SetFileInfo,QueryFileInfo,QueryFileSystemInfo,SetFileSystemInfo,QueryDirectoryInfo—will fail the trait bounds at compile time.Add to the macro expansion (after the
impl FileInfoTypeblock around line 108):impl $crate::binrw::meta::ReadEndian for $name { const ENDIAN: $crate::binrw::meta::EndianKind = $crate::binrw::meta::EndianKind::Little; } impl $crate::binrw::meta::WriteEndian for $name { const ENDIAN: $crate::binrw::meta::EndianKind = $crate::binrw::meta::EndianKind::Little; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (54)
README.md(2 hunks)crates/smb-dtyp/src/binrw_util.rs(1 hunks)crates/smb-dtyp/src/binrw_util/boolean.rs(1 hunks)crates/smb-dtyp/src/binrw_util/fixed_string.rs(1 hunks)crates/smb-dtyp/src/binrw_util/helpers.rs(3 hunks)crates/smb-dtyp/src/binrw_util/multi_sz.rs(1 hunks)crates/smb-dtyp/src/binrw_util/pos_marker.rs(2 hunks)crates/smb-dtyp/src/guid.rs(5 hunks)crates/smb-dtyp/src/security/sid.rs(1 hunks)crates/smb-dtyp/src/security/tests.rs(2 hunks)crates/smb-fscc/src/directory_info.rs(1 hunks)crates/smb-fscc/src/filesystem_info.rs(1 hunks)crates/smb-fscc/src/info_classes.rs(3 hunks)crates/smb-fscc/src/query_file_info.rs(2 hunks)crates/smb-fscc/src/quota.rs(2 hunks)crates/smb-fscc/src/set_file_info.rs(1 hunks)crates/smb-msg/Cargo.toml(2 hunks)crates/smb-msg/README.md(1 hunks)crates/smb-msg/src/cancel.rs(2 hunks)crates/smb-msg/src/compressed.rs(7 hunks)crates/smb-msg/src/create.rs(19 hunks)crates/smb-msg/src/dfsc.rs(5 hunks)crates/smb-msg/src/echo.rs(1 hunks)crates/smb-msg/src/encrypted.rs(2 hunks)crates/smb-msg/src/error.rs(1 hunks)crates/smb-msg/src/file.rs(7 hunks)crates/smb-msg/src/header.rs(1 hunks)crates/smb-msg/src/info/common.rs(3 hunks)crates/smb-msg/src/info/query.rs(6 hunks)crates/smb-msg/src/info/set.rs(2 hunks)crates/smb-msg/src/ioctl/common.rs(1 hunks)crates/smb-msg/src/ioctl/fsctl.rs(3 hunks)crates/smb-msg/src/ioctl/msg.rs(4 hunks)crates/smb-msg/src/negotiate.rs(1 hunks)crates/smb-msg/src/notify.rs(4 hunks)crates/smb-msg/src/oplock.rs(1 hunks)crates/smb-msg/src/plain.rs(3 hunks)crates/smb-msg/src/session_setup.rs(3 hunks)crates/smb-msg/src/smb1.rs(1 hunks)crates/smb-msg/src/test.rs(1 hunks)crates/smb-msg/src/tree_connect.rs(5 hunks)crates/smb-rpc/Cargo.toml(1 hunks)crates/smb-rpc/src/interface/srvsvc.rs(3 hunks)crates/smb-rpc/src/ndr64.rs(1 hunks)crates/smb-rpc/src/ndr64/align.rs(1 hunks)crates/smb-rpc/src/ndr64/arrays.rs(2 hunks)crates/smb-rpc/src/ndr64/ptr.rs(2 hunks)crates/smb-transport/Cargo.toml(1 hunks)crates/smb-transport/src/netbios/msg.rs(2 hunks)crates/smb-transport/src/tcp/msg.rs(1 hunks)crates/smb/Cargo.toml(1 hunks)crates/smb/src/resource.rs(1 hunks)crates/smb/src/resource/file_util.rs(2 hunks)crates/smb/src/session/setup.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
crates/smb-dtyp/src/security/tests.rs (1)
crates/smb-dtyp/src/security/sid.rs (1)
from_str(58-92)
crates/smb-msg/src/notify.rs (2)
crates/smb-fscc/src/chained_list.rs (1)
is_empty(134-136)crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_aoff_size(573-597)
crates/smb/src/session/setup.rs (1)
crates/smb-msg/src/session_setup.rs (1)
new(59-72)
crates/smb-msg/src/ioctl/msg.rs (1)
crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_aoff(344-363)
crates/smb-dtyp/src/binrw_util/fixed_string.rs (1)
crates/smb-dtyp/src/binrw_util/sized_string.rs (1)
display_utf16(282-290)
crates/smb-msg/src/compressed.rs (1)
crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_size_plus(627-645)
crates/smb-msg/src/tree_connect.rs (1)
crates/smb-dtyp/src/binrw_util/pos_marker.rs (2)
write_aoff_m(367-386)new(15-20)
crates/smb-msg/src/create.rs (3)
crates/smb-dtyp/src/binrw_util/pos_marker.rs (5)
seek_from(24-26)write_roff_plus(222-240)write_roff_size_b_plus(481-506)seek_from_if(29-35)new(15-20)crates/smb-dtyp/src/binrw_util/helpers.rs (1)
binread_if_has_data(29-43)crates/smb-dtyp/src/guid.rs (1)
as_u128(83-90)
crates/smb-msg/src/dfsc.rs (1)
crates/smb-dtyp/src/binrw_util/pos_marker.rs (5)
write_options(92-105)write_size_plus(627-645)write_roff_b_plus(451-474)seek_from(24-26)new(15-20)
crates/smb-dtyp/src/binrw_util/helpers.rs (2)
crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
read_options(73-83)crates/smb-dtyp/src/binrw_util/boolean.rs (1)
read_options(21-28)
🪛 Gitleaks (8.28.0)
crates/smb-msg/src/oplock.rs
[high] 101-101: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 111-111: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 118-118: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🪛 LanguageTool
crates/smb-msg/README.md
[grammar] ~15-~15: Ensure spelling is correct
Context: ...-related functionality in Rust. See the dcumentation of the crate for more information. Con...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test (single_threaded)
- GitHub Check: test (multi_threaded)
- GitHub Check: test (async)
🔇 Additional comments (66)
crates/smb-msg/src/info/common.rs (3)
58-66: LGTM: Comparison traits added to generated enum.Adding
PartialEqandEqderives to the macro-generated enum follows Rust best practices and enables direct equality comparisons in tests and application code. This aligns with the PR's test refactoring objectives.
102-102: LGTM: Appropriate comment style.Changing from a doc comment to a regular comment is correct here, as this line describes internal macro structure rather than documenting the public API.
112-121: LGTM: Comparison traits added to generated struct.Adding
PartialEqandEqderives to theRaw<Name>struct is appropriate. Since bothVec<u8>andPhantomData<T>already implement these traits, the derivation works correctly without additional trait bounds. Equality based on raw byte comparison is semantically correct and supports the test refactoring objectives.crates/smb-msg/src/encrypted.rs (3)
63-63: LGTM: Clean test utility import.The import of the
test_binrw!macro from thesmb_testscrate is appropriate for the test refactoring.
67-80: LGTM: Test refactoring is correct.The test case correctly represents the
EncryptedHeaderstruct. The hex string accurately encodes all fields including magic bytes, signature, nonce, message size (104), and session ID. The macro-driven approach improves maintainability.
82-95: LGTM: Second test case is accurate.The test data and hex encoding are correct. Both test cases now follow the same macro-driven pattern, which is consistent and maintainable.
crates/smb-msg/src/file.rs (1)
13-13: LGTM! Derive additions support macro-driven testing.The addition of
PartialEqandEqderives toFlushRequest,ReadRequest,ReadFlags,WriteRequest, andWriteFlagsenables direct equality comparisons required by the test macros (test_binrw!,test_request!,test_response!). This aligns with the PR's objective to migrate to macro-driven test patterns.Also applies to: 39-39, 111-111, 137-137, 205-205
crates/smb-msg/src/oplock.rs (2)
97-107: LGTM! Clean test migration to macro-driven approach.The test correctly validates
LeaseBreakNotifyserialization/deserialization with the expected binary payload. The macro-based approach is cleaner and more maintainable than manual Cursor-based tests.Note: The Gitleaks warning on line 101 is a false positive—this is a test GUID, not an API key or secret.
109-114: Test looks correct.The test validates
LeaseBreakAckbinary I/O with the expected payload. The Gitleaks warning on line 111 is a false positive.crates/smb-msg/src/echo.rs (1)
20-20: LGTM! Test refactoring simplifies verification.The migration to
test_binrw!macro-based tests correctly verifies binary read/write round-trips for bothEchoRequestandEchoResponse. The hex data "04000000" accurately represents the expected 4-byte structure (size=4, reserved=0).Also applies to: 24-30
crates/smb-rpc/Cargo.toml (1)
24-26: LGTM! Dev dependency addition supports test macro migration.The addition of
smb-testsas a dev-dependency is appropriate for the test macro refactor described in the PR objectives. The relative path and version are consistent with workspace conventions.crates/smb-rpc/src/interface/srvsvc.rs (2)
246-308: LGTM! Macro-driven test reduces boilerplate effectively.The
test_binrw_read!macro correctly validates deserialization ofNetrShareEnumOutwith comprehensive test data covering multiple share types (ADMIN$, C$, IPC$, and custom shares).
310-324: LGTM! Write test macro correctly validates serialization.The
test_binrw_write!macro properly tests serialization ofNetrShareEnumInwith appropriate test parameters including server name, info struct, and maximum length.crates/smb-rpc/src/ndr64/ptr.rs (2)
243-243: LGTM: Test macro import.The import of
test_binrwfromsmb_testsaligns with the PR's goal of migrating to macro-driven tests.
255-266: LGTM: Null pointer test case.The byte sequence correctly represents a null NDR pointer:
- Proper alignment padding (7 bytes)
- Null reference ID (0x0)
- Correct little-endian encoding of the
otherfieldcrates/smb-transport/src/tcp/msg.rs (2)
23-27: LGTM! Clean macro-based test migration.The test correctly verifies the binary representation of
SmbTcpMessageHeader:
- Magic byte:
0x00✓- Stream protocol length
0x123456serializes to12 34 56in big-endian (3 bytes as expected for u24) ✓The macro-driven approach simplifies the test while maintaining coverage of the serialization/deserialization round-trip.
19-19: The test_binrw macro is properly implemented and the test case is syntactically correct.The macro exists at
crates/smb-tests/src/binrw.rsand performs comprehensive round-trip testing by generating both read and write test functions. The test case syntax matches the expected struct form pattern:struct SmbTcpMessageHeader { stream_protocol_length: 0x123456, } => "00 12 34 56". The expected bytes correctly represent the magic byte (0x00) followed by the big-endian u24 encoding of 0x123456. No issues found.crates/smb-msg/src/negotiate.rs (1)
458-518: LGTM! Comprehensive test coverage.The
test_response!macro usage is well-structured with all required fields properly populated. The test includes a robustnegotiate_context_listcovering all major context types (preauth integrity, encryption, signing, RDMA transform, and compression), which thoroughly exercises the SMB 3.1.1 negotiation context handling.crates/smb-msg/src/notify.rs (3)
13-13: LGTM: Enhanced comparability for ChangeNotifyRequest.Adding
PartialEqandEqderives aligns with the PR's goal of extending comparability across key request/response types. All struct fields support these traits.
90-91: LGTM: Correct offset-aware buffer serialization.The conditional write combined with
write_aoff_sizecorrectly handles both empty and non-empty buffers:
- Empty buffer: Write is skipped, markers remain at default (0), resulting in the correct 8-byte structure validated by the test on line 161-164.
- Non-empty buffer:
write_aoff_sizewrites the buffer data and updates both offset and length markers appropriately.This pattern aligns with the PR's shift toward more explicit, offset-aware binary I/O.
144-208: LGTM: Test refactoring aligns with PR objectives.The migration from manual cursor-based tests to macro-driven tests (
smb_tests::test_binrw!,test_response!,test_response_read!) is consistent with the PR's goal of modernizing the test infrastructure. Test coverage appears comprehensive, including edge cases (empty buffer) and real-world scenarios (Azure notification).crates/smb-msg/src/cancel.rs (2)
5-14: LGTM!The addition of
PartialEqandEqderives is appropriate for this simple struct with primitive fields. This enables equality comparisons which can be useful for testing and assertions.
20-22: Macro is properly available and correctly applied.The
test_binrw!macro is exported with#[macro_export]at line 189 incrates/smb-tests/src/binrw.rsand is correctly invoked viasmb_tests::test_binrw!. The test hex output"04000000"accurately represents theCancelRequeststruct layout (4-byte structure size + 4-byte reserved field in little-endian format). The test is properly scoped and syntactically valid.crates/smb-transport/src/netbios/msg.rs (3)
60-60: LGTM: Trait derives enable test comparisons.Adding
PartialEqandEqis appropriate for the test refactor, as the macro-based tests need to compare instances for correctness.
270-272: LGTM: Clean test macro import.The import structure is clear and follows standard conventions for test utilities.
274-277: LGTM: Well-formed macro test cases.The
NetBiosNameandNBSSPacketHeadertests are correctly structured with single-line hex strings.Also applies to: 287-293
crates/smb-msg/src/ioctl/common.rs (4)
16-23: LGTM! Clean utility struct for raw IOCTL buffers.The
IoctlBufferstruct appropriately usesbinrw::helpers::until_eofto parse unstructured byte buffers, which is the correct approach for IOCTL requests without defined schemas.
25-37: LGTM! Standard conversion implementations.Both
Fromimplementations follow idiomatic Rust patterns for converting owned and borrowed byte collections.
39-43: LGTM! Correct trait implementation.The
get_bin_sizeimplementation correctly returns the buffer length, leveraging theDerefimplementation for clean access toVec<u8>'slen()method.
45-57: LGTM! Standard newtype pattern.The
DerefandDerefMutimplementations follow the standard newtype pattern, enabling transparent access to the underlyingVec<u8>methods.crates/smb-msg/src/ioctl/msg.rs (3)
145-148: Verify that this assertion logic change aligns with SMB protocol semantics.The assertion was changed from validating
out_buffer.is_empty()toin_buffer.is_empty(). For IOCTL responses, this makes logical sense since responses typically contain output data (out_buffer) but should not have input data (in_buffer), except for pass-through operations as noted.However, this is a significant behavioral change that affects validation semantics. Please confirm this aligns with the SMB2/3 protocol specification for IOCTL responses.
166-172: LGTM! Correct offset-aware buffer serialization.The
write_withhooks usingPosMarker::write_aoffensure that bothin_bufferandout_bufferare written at their respective offset positions, which correctly implements SMB's variable-position buffer encoding.
197-216: Manual verification required for refactored test data and conversion chain.The refactoring moved test data to hex string constants (150+ characters each), which are difficult to manually verify for correctness. While this improves maintainability with the new macro framework, the following require manual verification:
- The hex string constants
REQ_IOCTL_BUFFER_CONTENTandIOCTL_TEST_BUFFER_CONTENTmatch the original test data- The conversion chain at line 22-24 (
IoctlBuffer::from(...).into()) appears to have a redundant.into()call—IoctlBuffer::from()already producesIoctlBuffer, and no otherIntoimplementations forIoctlBufferwere found, so the.into()likely convertsIoctlBufferto itselfVerify by:
- Comparing the hex strings with the original test data (before refactoring)
- Simplifying line 24 to remove the redundant
.into()if the variant expectsIoctlBufferdirectlycrates/smb-msg/src/session_setup.rs (3)
7-7: LGTM! Standard test-friendly derives added.Adding
Debug, PartialEq, Eqderives is appropriate for testing and debugging purposes.
58-73: LGTM! Capabilities parameter added for DFS propagation.The signature change correctly propagates negotiated capabilities instead of hardcoding DFS support. This aligns with the changes in
session/setup.rswhere DFS is derived from connection negotiation.Note: This is a breaking API change if
SessionSetupRequest::new()is public, requiring all callers to provide thecapabilitiesparameter.
129-154: LGTM! Test refactoring uses new macro structure.The tests have been successfully migrated to use the new testing macros while maintaining the same test coverage. Note that the test explicitly sets
capabilities: NegotiateCapabilities::new().with_dfs(true), which is consistent with testing the DFS-enabled scenario.crates/smb/src/session/setup.rs (3)
317-328: LGTM! DFS capability now parameterized.The function correctly accepts a
dfsparameter and propagates it toNegotiateCapabilities, enabling dynamic DFS configuration based on connection negotiation rather than hardcoding it totrue.
330-339: LGTM! DFS capability correctly propagated from negotiation.The implementation correctly derives the DFS flag from the connection's negotiated capabilities and passes it to the session setup request.
Note: This is a behavior change from always advertising DFS support to conditionally advertising it based on what was negotiated. Ensure this aligns with the intended SMB protocol compliance.
365-383: Implementation follows SMB3 protocol specification for session binding DFS capability.The TODO comment on line 372 raises a concern based on incomplete understanding of the protocol. Per MS-SMB2 specification (section 3.2.5.3.3), the DFS capability bit in a session binding request is a negotiated capability for the current connection, not inherited from the original session. Each connection can negotiate DFS capability independently, and the client that supports DFS MUST set the DFS bit in the Capabilities field during re-setup.
The current implementation correctly uses
_setup.conn_info().negotiation.caps.dfs()for both new sessions and session binding, which is protocol-compliant. The TODO comment can be safely removed or updated to reflect that DFS capability renegotiation per connection is intentional and correct.crates/smb-rpc/src/ndr64/arrays.rs (3)
234-235: LGTM!The import is straightforward and necessary for the macro-driven test approach.
240-254: LGTM!The binrw attributes correctly implement the NDR array protocol with two-stage pointer handling. The conditional write on line 248 ensures
random_byteis only written during the ref ID stage, which aligns with theNdrArrayimplementation's two-pass read/write pattern.
265-281: Code migration is correct; the test_binrw! macro properly replaces manual Cursor-based testing.The git history confirms that commit 36c95cb migrated this test from manual Cursor-based implementation to the
test_binrw!macro. The hex string on line 280 matches exactly the byte sequence from the previous manual test. The macro automatically generates both serialization and deserialization tests usingassert_eq!, providing equivalent—and arguably better—validation than the original manual approach.crates/smb-rpc/src/ndr64/align.rs (3)
94-100: LGTM! Well-designed test structure.The
TestNdrAlignstruct effectively tests NDR64 alignment behavior by creating an unaligned position (3 bytes) withunalignandunalign2, then verifying thatNdrAlign<u32>correctly aligns to an 8-byte boundary before writing.
102-115: LGTM! Alignment calculation is correct.The test correctly validates NDR64 alignment behavior:
unalign(u8): 1 byte →00unalign2(u16): 2 bytes →00 00- Position 3 → 5 padding bytes →
00 00 00 00 00(aligns to offset 8)should_align.value(u32): 4 bytes little-endian →78 56 34 12The hexdump accurately represents the 12-byte output with proper 8-byte alignment.
90-90: Verification complete: test_binrw macro is properly available.The
test_binrwmacro is defined incrates/smb-tests/src/binrw.rs(line 189) with#[macro_export]and is correctly imported viause smb_tests::test_binrw;. The macro expands to both read and write tests, and theTestNdrAlignstruct usage is syntactically correct with proper NDR64 alignment validation.crates/smb/Cargo.toml (2)
15-15: Verify feature flag compatibility.The dependency configuration now disables default features and explicitly enables only the
clientfeature. Ensure that all required functionality fromsmb-msgis available through this feature configuration.
19-19: Verify smb-transport builds without default features.Disabling default features for
smb-transportmay affect available functionality. Confirm that all required features are either explicitly enabled elsewhere or not needed by this crate.crates/smb-dtyp/src/binrw_util/pos_marker.rs (2)
23-35: LGTM! Clean API extension with conditional seeking.The refactoring to delegate
seek_fromto a newseek_from_ifmethod is well-designed. The inline annotation is appropriate for this simple delegation, and the conditional variant provides useful flexibility without breaking existing code.
447-474: LGTM! Useful addition for offset calculations.The new
write_roff_b_plusmethod is well-documented and follows the established pattern of other writer helpers in this module. Theoffset_plusparameter provides necessary flexibility for writing structures with self-inclusive size fields.crates/smb-msg/src/info/query.rs (2)
14-14: LGTM! Adding PartialEq and Eq derives enhances testability.Adding
PartialEqandEqderives to these query information structures is beneficial for testing and comparison operations. These derives are safe for binary I/O types and enable more expressive assertions.Also applies to: 41-41, 99-99, 118-118, 178-178, 212-212
249-312: LGTM! Test modernization with macro-driven approach.The refactoring to use
test_request!andtest_response!macros aligns with the broader test harness modernization across the codebase. This approach reduces boilerplate and makes tests more maintainable.crates/smb-fscc/src/info_classes.rs (2)
4-6: LGTM! Import addition supports trait expansion.Adding
WriteEndianto the imports is necessary for the expanded trait bounds below.
52-54: LGTM! Explicit trait bounds improve clarity.Replacing the generic
brwtype parameter with explicitBinReadandBinWritebounds makes the requirements clearer and aligns with the trait refactoring above.crates/smb-dtyp/src/binrw_util/fixed_string.rs (1)
132-132: LGTM! Module path updated correctly.The module path change from
super::sized_wide_stringtosuper::sized_stringaligns with the broader reorganization of thebinrw_utilmodule. Thedisplay_utf16function is confirmed to exist at the new location with the same signature.crates/smb-dtyp/src/security/sid.rs (2)
126-130: LGTM! Excellent addition of negative test cases.Adding tests for invalid SID strings significantly improves test coverage. The test cases cover various failure modes including empty strings, incomplete formats, and malformed identifiers.
132-135: LGTM! Test modernization with macro approach.Refactoring the binary round-trip test to use the
test_binrw!macro aligns with the broader test harness modernization and reduces boilerplate.crates/smb-fscc/src/set_file_info.rs (1)
33-33: The review comment is based on incorrect information and should be dismissed.After comprehensive verification:
No Write variant exists in the current
SetFileInfoenum. The enum contains exactly 12 variants: Allocation, Basic, Disposition, EndOfFile, FullEa, Link, Mode, Pipe, Position, Rename, ShortName, and ValidDataLength.Zero references to
SetFileInfo::Writeexist anywhere in the codebase across all search patterns.No recent changes to the file are shown in git diff, and the git history does not indicate removal of a Write variant.
Line 33 is simply the closing brace of the
file_info_classes!macro invocation—not a breaking change.The premise of the original review comment (that a Write variant was removed) is factually inaccurate. There is no evidence of a breaking change or any issue at this location.
Likely an incorrect or invalid review comment.
README.md (1)
12-12: LGTM! Documentation improvements enhance clarity.The changes accurately reflect the project's capabilities:
- Clarifying "direct" dependencies distinguishes Rust implementation from C bindings
- "All SMB 2.X & 3.X dialects" is more precise than generic "support"
- Highlighting
binrwusage emphasizes memory safety- Authentication methods and cross-platform support are valuable additions
Also applies to: 26-27, 31-32
crates/smb-fscc/src/quota.rs (1)
18-18: LGTM! Trait derivations support testing.Adding
PartialEqandEqderives enables equality comparisons for test assertions, aligning with the broader test infrastructure improvements in this PR.Also applies to: 48-48
crates/smb-msg/src/error.rs (1)
92-94: LGTM! Macro-driven test reduces boilerplate.The migration from manual Cursor-based testing to
test_response!macro aligns with the PR's objective of standardizing test infrastructure.crates/smb-fscc/src/filesystem_info.rs (1)
24-24: LGTM! Macro signature update aligns with trait bound refactor.The removal of explicit
Read/Writequalifiers is part of the broaderfile_info_classes!macro refactor where trait bounds are now handled internally viaBinRead/BinWriteconstraints.Also applies to: 32-32
crates/smb-dtyp/src/binrw_util.rs (1)
3-3: LGTM! Module restructuring adds new binrw utilities.The changes introduce
BooleanandMultiWSztypes while consolidating string utilities undersized_string. The prelude exports ensure backward compatibility for existing consumers.Also applies to: 8-10, 13-13, 18-23
crates/smb-fscc/src/directory_info.rs (1)
32-32: LGTM! Macro signature update aligns with trait bound refactor.Consistent with the
file_info_classes!macro changes across the codebase.crates/smb-fscc/src/query_file_info.rs (2)
41-41: LGTM! Macro signature update aligns with trait bound refactor.Consistent with the
file_info_classes!macro changes across the codebase.
401-406: LGTM! Improved null terminator handling.The explicit comment and adjusted read length (
ea_name_length as u64 + 1) correctly handle the null-terminated EA name, improving code clarity and correctness.crates/smb-rpc/src/ndr64.rs (1)
17-31: LGTM! Macro-driven test improves maintainability.Moving
TestNdrStringPtrto module scope and using thetest_binrw!macro reduces boilerplate while maintaining test coverage.
afiffon
force-pushed
the
143-binrw-structs-ut-refactor---macros
branch
from
a8664c7 to
8714b82
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
crates/smb-msg/src/message.rs(1 hunks)crates/smb-msg/src/plain.rs(3 hunks)crates/smb-transport/src/tcp/msg.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (windows-latest, 1.85)
- GitHub Check: test (single_threaded)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: test (multi_threaded)
- GitHub Check: test (async)
🔇 Additional comments (2)
crates/smb-transport/src/tcp/msg.rs (2)
4-4: LGTM! Derives enable test assertions.Adding
PartialEqandEqis appropriate and enables the new macro-driven tests to perform equality assertions.
19-27: LGTM! Test refactor is correct.The test macro usage correctly validates that
stream_protocol_length: 0x123456serializes to"00 12 34 56"(magic byte\x00followed by 24-bit big-endian value). This aligns with the PR's objective to migrate to macro-driven tests.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/smb/src/connection.rs (1)
399-416: Update QUIC/RDMA pushes to use the newNegotiateContexthelpers.After the NegotiateContext refactor, the struct literal here needs
data_lengthand_reserved, so this block no longer compiles. Instead of hand-building the context, reuse the newFrom<TransportCapabilities>/From<RdmaTransformCapabilities>conversions (or explicitly set the missing fields).#[cfg(feature = "quic")] if matches!(self.config.transport, TransportConfig::Quic(_)) { - ctx_list.push(NegotiateContext { - context_type: NegotiateContextType::TransportCapabilities, - data: NegotiateContextValue::TransportCapabilities( - TransportCapabilities::new().with_accept_transport_layer_security(true), - ), - }); + ctx_list.push( + TransportCapabilities::new() + .with_accept_transport_layer_security(true) + .into(), + ); } // TODO: Add to config if cfg!(feature = "rdma") { - ctx_list.push(NegotiateContext { - context_type: NegotiateContextType::RdmaTransformCapabilities, - data: NegotiateContextValue::RdmaTransformCapabilities( - RdmaTransformCapabilities { - transforms: vec![RdmaTransformId::None], - }, - ), - }); + ctx_list.push( + RdmaTransformCapabilities { + transforms: vec![RdmaTransformId::None], + } + .into(), + ); }
♻️ Duplicate comments (10)
crates/smb-msg/README.md (1)
12-17: Good documentation update with typo fix.The new "Usage" section provides helpful guidance for users and aligns well with the crate's feature configuration changes (server/client/both). The previous typo ("dcumentation") has been correctly fixed to "documentation" on Line 15.
crates/smb-transport/src/netbios/msg.rs (1)
283-284: Fix multi-line hex string literal.The hex string spans two lines, which embeds a literal newline character and whitespace into the string. This will break hex parsing and cause the test to fail.
Apply this diff to put the hex string on a single line:
- } => "20434b4644454e45434644454646434647454646434341434143414341434143410020454e - 45424544454345504550454c464146434550434e4542454744494542414100" + } => "20434b4644454e45434644454646434647454646434341434143414341434143410020454e45424544454345504550454c464146434550434e4542454744494542414100"Alternatively, use explicit string concatenation if you need to split for readability:
- } => "20434b4644454e45434644454646434647454646434341434143414341434143410020454e - 45424544454345504550454c464146434550434e4542454744494542414100" + } => concat!( + "20434b4644454e45434644454646434647454646434341434143414341434143410020454e", + "45424544454345504550454c464146434550434e4542454744494542414100" + )crates/smb-msg/src/compressed.rs (1)
94-95: Borrowcompression_algorithmwhen forwarding to the helper
add_original_size_to_total_lengthtakes&CompressionAlgorithm, but the writer args still passcompression_algorithmby value. The generated writer ends up callingadd_original_size_to_total_length(self.compression_algorithm)and fails to compile. Please forward a reference instead.- #[bw(write_with = PosMarker::write_size_plus, args(&length, add_original_size_to_total_length(compression_algorithm)))] + #[bw(write_with = PosMarker::write_size_plus, args(&length, add_original_size_to_total_length(&compression_algorithm)))]crates/smb-msg/src/dfsc.rs (1)
316-325: Fix DFS referral offset baseAs noted in the previous review, these roffset writers still add only the 4-byte
ReferralEntry::COMMON_PART_SIZE, so every V3/V4 referral offset we emit is 8 bytes too small—readers expect the 12-byteEntryV3Value::OFFSET_FROM_ENTRY_START, causing them to land in the fixed header instead of the strings. Please align the write side with the read side by using the V3 offset constant.Apply this diff:
- #[bw(write_with = PosMarker::write_roff_b_plus, args(&dfs_path_offset, &_start, ReferralEntry::COMMON_PART_SIZE as u64))] + #[bw(write_with = PosMarker::write_roff_b_plus, args(&dfs_path_offset, &_start, EntryV3Value::OFFSET_FROM_ENTRY_START as u64))] ... - #[bw(write_with = PosMarker::write_roff_b_plus, args(&dfs_alternate_path_offset, &_start, ReferralEntry::COMMON_PART_SIZE as u64))] + #[bw(write_with = PosMarker::write_roff_b_plus, args(&dfs_alternate_path_offset, &_start, EntryV3Value::OFFSET_FROM_ENTRY_START as u64))] ... - #[bw(write_with = PosMarker::write_roff_b_plus, args(&network_address_offset, &_start, ReferralEntry::COMMON_PART_SIZE as u64))] + #[bw(write_with = PosMarker::write_roff_b_plus, args(&network_address_offset, &_start, EntryV3Value::OFFSET_FROM_ENTRY_START as u64))]crates/smb-msg/src/create.rs (1)
262-270: Fix zero-length context seek underflow
_name_offset.value/_data_offset.valueare zero when their lengths are zero. SubtractingCHAINED_ITEM_PREFIX_SIZEfrom thoseu16s wraps tou64::MAX, so readers seek to the wrong place and corrupt every subsequent context. Gate the seek on the matching length and usesaturating_subbefore converting tou64.- #[br(seek_before = _name_offset.seek_from(_name_offset.value as u64 - CHAINED_ITEM_PREFIX_SIZE as u64))] + #[br(seek_before = _name_offset.seek_from_if( + _name_offset.value.saturating_sub(CHAINED_ITEM_PREFIX_SIZE as u16) as u64, + name_length > 0, + ))] @@ - #[br(seek_before = _name_offset.seek_from_if(_data_offset.value as u64 - CHAINED_ITEM_PREFIX_SIZE as u64, _data_length.value > 0))] + #[br(seek_before = _name_offset.seek_from_if( + _data_offset.value.saturating_sub(CHAINED_ITEM_PREFIX_SIZE as u16) as u64, + _data_length.value > 0, + ))]crates/smb-msg/src/oplock.rs (1)
110-123: Tests are identical because they're type aliases.These two tests are identical because
LeaseBreakAckandLeaseBreakAckResponseare type aliases (line 90). This was already flagged in a previous review. The Gitleaks warnings on lines 112 and 119 are false positives (test GUIDs).crates/smb-dtyp/src/binrw_util/boolean.rs (1)
14-16: Fix missingsize_ofqualification.The
size_ofcall needs to be fully qualified. This issue was already flagged in a previous review.Apply this diff:
- const _VALIDATE_SIZE_OF: [u8; 1] = [0; size_of::<Self>()]; + const _VALIDATE_SIZE_OF: [u8; 1] = [0; std::mem::size_of::<Self>()];crates/smb-msg/src/header.rs (1)
244-263: Verify the confusing byte-order conversion in the test.The migration to
test_binrw!macro is good and consistent with the PR's objectives. However, lines 259-261 still use the confusingu128::from_le_bytes(u128::to_be_bytes(...))pattern that was flagged in previous reviews. This performs a byte reversal but obscures intent.Consider one of these clearer alternatives (as previously suggested):
- If
0x63f825deae02952fa3d8c8aaf46e7c99represents the numeric value:- signature: u128::from_le_bytes(u128::to_be_bytes( - 0x63f825deae02952fa3d8c8aaf46e7c99 - )), + signature: 0x63f825deae02952fa3d8c8aaf46e7c99u128,
- If the hex digits represent a specific big-endian byte sequence:
- signature: u128::from_le_bytes(u128::to_be_bytes( - 0x63f825deae02952fa3d8c8aaf46e7c99 - )), + signature: u128::from_be_bytes([ + 0x63, 0xf8, 0x25, 0xde, 0xae, 0x02, 0x95, 0x2f, + 0xa3, 0xd8, 0xc8, 0xaa, 0xf4, 0x6e, 0x7c, 0x99 + ]),crates/smb-rpc/src/ndr64/ptr.rs (1)
268-281: Minor formatting inconsistency in test block.The test logic is correct, but lines 273-274 split the
=>and opening bracket[across two lines, while the previous test (line 260) keeps them together on one line.Consider aligning the formatting with the first test:
TestNdrU32Ptr => with_data: TestNdrU32Ptr { unalign: 0x2, null_ptr: Some(0xdeadbeef).into(), other: 0x12345678, - } => - [ + } => [ 0x2, // unaligned bytecrates/smb-rpc/src/interface/srvsvc.rs (1)
310-310: Inconsistent import style (previously flagged).Line 310 uses the fully-qualified path
smb_tests::test_binrw_write!, but line 242 already importssmb_tests::*. For consistency, usetest_binrw_write!directly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (38)
crates/smb-dtyp/src/binrw_util/boolean.rs(1 hunks)crates/smb-dtyp/src/binrw_util/fixed_string.rs(1 hunks)crates/smb-dtyp/src/binrw_util/multi_sz.rs(1 hunks)crates/smb-dtyp/src/guid.rs(5 hunks)crates/smb-dtyp/src/security/sid.rs(2 hunks)crates/smb-dtyp/src/security/tests.rs(2 hunks)crates/smb-fscc/src/common_info.rs(1 hunks)crates/smb-fscc/src/directory_info.rs(2 hunks)crates/smb-fscc/src/filesystem_info.rs(2 hunks)crates/smb-fscc/src/query_file_info.rs(3 hunks)crates/smb-fscc/src/set_file_info.rs(2 hunks)crates/smb-msg/README.md(1 hunks)crates/smb-msg/src/cancel.rs(2 hunks)crates/smb-msg/src/compressed.rs(7 hunks)crates/smb-msg/src/create.rs(19 hunks)crates/smb-msg/src/dfsc.rs(5 hunks)crates/smb-msg/src/echo.rs(1 hunks)crates/smb-msg/src/encrypted.rs(2 hunks)crates/smb-msg/src/file.rs(7 hunks)crates/smb-msg/src/header.rs(1 hunks)crates/smb-msg/src/info/set.rs(2 hunks)crates/smb-msg/src/ioctl/fsctl.rs(3 hunks)crates/smb-msg/src/ioctl/msg.rs(4 hunks)crates/smb-msg/src/negotiate.rs(4 hunks)crates/smb-msg/src/notify.rs(4 hunks)crates/smb-msg/src/oplock.rs(1 hunks)crates/smb-msg/src/session_setup.rs(3 hunks)crates/smb-msg/src/tree_connect.rs(5 hunks)crates/smb-rpc/src/interface/srvsvc.rs(3 hunks)crates/smb-rpc/src/ndr64.rs(1 hunks)crates/smb-rpc/src/ndr64/align.rs(1 hunks)crates/smb-rpc/src/ndr64/arrays.rs(2 hunks)crates/smb-rpc/src/ndr64/ptr.rs(2 hunks)crates/smb-tests/src/binrw.rs(1 hunks)crates/smb-tests/src/lib.rs(1 hunks)crates/smb-transport/src/netbios/msg.rs(2 hunks)crates/smb-transport/src/tcp/msg.rs(2 hunks)crates/smb/src/connection.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
crates/smb-dtyp/src/binrw_util/fixed_string.rs (1)
crates/smb-dtyp/src/binrw_util/sized_string.rs (1)
display_utf16(282-290)
crates/smb-msg/src/ioctl/msg.rs (2)
crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_aoff(344-363)crates/smb-msg/src/ioctl/common.rs (2)
from(26-28)from(32-36)
crates/smb-msg/src/notify.rs (2)
crates/smb-fscc/src/chained_list.rs (1)
is_empty(134-136)crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_aoff_size(573-597)
crates/smb-dtyp/src/security/tests.rs (1)
crates/smb-dtyp/src/security/sid.rs (1)
from_str(58-92)
crates/smb-msg/src/tree_connect.rs (1)
crates/smb-dtyp/src/binrw_util/pos_marker.rs (2)
write_aoff_m(367-386)new(15-20)
crates/smb-msg/src/dfsc.rs (1)
crates/smb-dtyp/src/binrw_util/pos_marker.rs (4)
write_options(92-105)write_size_plus(627-645)write_roff_b_plus(451-474)new(15-20)
crates/smb-msg/src/session_setup.rs (1)
crates/smb-msg/src/tree_connect.rs (1)
new(210-216)
crates/smb-msg/src/ioctl/fsctl.rs (2)
crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_size(602-620)crates/smb-dtyp/src/binrw_util/sized_string.rs (1)
args(95-95)
crates/smb-dtyp/src/binrw_util/multi_sz.rs (1)
crates/smb-dtyp/src/binrw_util/sized_string.rs (11)
std(16-16)std(62-62)std(64-64)read_options(90-116)write_options(126-135)deref(147-149)deref_mut(153-155)from(139-141)from(166-171)from(230-234)from(246-250)
crates/smb-dtyp/src/binrw_util/boolean.rs (2)
crates/smb-dtyp/src/binrw_util/fixed_string.rs (1)
std(39-39)crates/smb-dtyp/src/security/sid.rs (2)
std(40-40)std(41-41)
crates/smb-msg/src/create.rs (3)
crates/smb-dtyp/src/binrw_util/pos_marker.rs (6)
write_roff_size(421-445)seek_from(24-26)write_roff_plus(222-240)write_roff_size_b_plus(481-506)seek_from_if(29-35)new(15-20)crates/smb-dtyp/src/binrw_util/helpers.rs (1)
binread_if_has_data(29-43)crates/smb-dtyp/src/guid.rs (1)
as_u128(83-90)
crates/smb-msg/src/compressed.rs (1)
crates/smb-dtyp/src/binrw_util/pos_marker.rs (1)
write_size_plus(627-645)
🪛 Gitleaks (8.28.0)
crates/smb-msg/src/oplock.rs
[high] 102-102: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 112-112: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 119-119: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (windows-latest, 1.85)
- GitHub Check: test (multi_threaded)
- GitHub Check: test (async)
- GitHub Check: test (single_threaded)
🔇 Additional comments (65)
crates/smb-msg/src/cancel.rs (2)
6-6: LGTM: PartialEq and Eq derives added.The addition of
PartialEqandEqderives is appropriate and beneficial for testing. Since both fields areu16(which already implements these traits), the derives will work correctly.
19-23: LGTM: Test refactoring with test_binrw! macro.The migration to the
test_binrw!macro aligns with the PR objectives and reduces test boilerplate. The expected binary representation"04000000"correctly corresponds to aCancelRequestwith_structure_size=4and_reserved=0in little-endian format.crates/smb-transport/src/netbios/msg.rs (4)
60-60: LGTM: Appropriate trait derivations added.Adding
PartialEqandEqderives is correct since bothNetBiosNamefields already implement these traits. This enables equality comparisons for test assertions.
270-272: LGTM: Test imports correctly added.The import of
smb_tests::*provides access to thetest_binrw!macro used in the refactored tests below.
274-277: LGTM: NetBiosName test correctly formatted.The test uses the
test_binrw!macro appropriately with a single-line hex string expectation.
287-293: LGTM: NBSSPacketHeader test correctly formatted.The test uses the
test_binrw!macro appropriately with proper struct field initialization and single-line hex string expectation.crates/smb-msg/src/tree_connect.rs (5)
27-27: LGTM: Derive additions improve testability.Adding
PartialEqandEqderives enables comparison operations and aligns with the broader derive expansions across the PR.
63-63: Defensive change prevents panic on malformed packets.Changing from
unwrap()tounwrap_or(0)is safer. Whenextension_presentis true,tree_connect_context_countshould always beSome(...), so the fallback to0handles malformed packets gracefully rather than panicking.
70-70: LGTM: Derive additions improve testability.Adding
PartialEqandEqderives is consistent with the pattern across the file and enables proper testing.
88-88: LGTM: Derive additions improve testability.Adding
PartialEqandEqderives to the macro-generated structure is appropriate and enables comparison operations for testing.
42-51: The SMB2 TREE_CONNECT request extension specifies a Reserved field of 10 bytes (MUST be set to zero). The implementation correctly reflects this: the_reservedfield is defined asOption<[u8; 10]>and initialized via#[bw(calc = Some([0u8; 10]))], ensuring the field matches both the size and zero-value requirement. The gating conditions are also appropriate.crates/smb-dtyp/src/guid.rs (2)
202-204: LGTM! Clean migration to macro-based testing.The use of the
test_binrw!macro for binary round-trip validation is appropriate and aligns with the PR's objective to migrate to macro-based test generation.
93-110: Update documentation to clarify that the macro supports both compile-time literals and runtime expressions.The macro signature was correctly changed to accept
$s:expr(line 104), and the underlyingparse_uuidis aconst fn(line 31), which enables compile-time evaluation when possible. However, the documentation at line 93 claims the macro creates GUIDs "at compile time," which is misleading. The test at line 197 confirms the macro intentionally supports runtime expressions likeformat!("{{{TEST_GUID_STR}}}").as_str().Update the documentation to reflect both capabilities:
-/// A macro to create a `Guid` from a string literal at compile time. +/// A macro to create a `Guid` from a string expression. +/// Compile-time evaluation occurs when the input is a compile-time constant.crates/smb-fscc/src/set_file_info.rs (2)
176-176: LGTM!The switch to wildcard imports for the test module aligns with the macro-based test harness migration.
33-33: No issues found — Write variant removal is clean.Verification confirms:
- SetFileInfo enum contains no Write variant (lines 18-35)
- Zero references to SetFileInfo::Write across the codebase
- No downstream dependencies affected
crates/smb-fscc/src/directory_info.rs (2)
309-309: LGTM!Test import standardization is consistent with the broader test harness refactoring.
32-32: No downstream impact detected from Read variant removal.Comprehensive verification across the codebase confirms zero references to
QueryDirectoryInfo::Read. The removal is safe with no internal breaking changes.crates/smb-fscc/src/query_file_info.rs (3)
420-420: LGTM!Test import change is consistent with the broader refactoring.
41-41: Removal ofReadvariant verified clean - no breaking references.The
Readvariant has been successfully removed from theQueryFileInfoenum (crates/smb-fscc/src/query_file_info.rs:20-38) with no downstream impact. Codebase searches found zero references toQueryFileInfo::Read, and all consumers use generic trait bounds (QueryFileInfoValue) or reference remaining variants. The removal is safe.
401-406: No issues found — code is correct and consistent.The original review comment incorrectly flagged a potential issue that does not exist. binrw::NullString.len() does NOT include the null terminator, which means:
- Write:
ea_name.len()stores the string length without the terminator (e.g., 4 for "test")- Read:
take_seek(ea_name_length + 1)correctly reads the content plus the null terminator byte (e.g., 5 bytes total)- Result: NullString parses the null-terminated data and stores only the content, maintaining consistency
This is further validated by the assertion in
common_info.rsline 52 (#[br(assert(ea_name.len() == ea_name_length as usize))]), which confirms the lengths match after deserialization.Likely an incorrect or invalid review comment.
crates/smb-fscc/src/filesystem_info.rs (3)
379-379: LGTM!Test import change aligns with the macro-based test migration.
24-24: Verified: No downstream impact from Read variant removal.The
Readvariant has been successfully removed from theQueryFileSystemInfoenum, and verification confirms no code references it anywhere in the codebase. The breaking API change is cleanly applied with no remaining dependencies to address.
32-32: The review comment is incorrect and should be disregarded.The
SetFileSystemInfoenum contains only two variants:FsControlandFsObjectId. There is noWritevariant in the current code, and exhaustive codebase searches found zero references toSetFileSystemInfo::Write. The review comment's premise—that aWritevariant was removed—does not match the actual codebase.Likely an incorrect or invalid review comment.
crates/smb-transport/src/tcp/msg.rs (2)
4-4: LGTM: Added derives for equality comparisons.The addition of
PartialEqandEqenables direct equality comparisons onSmbTcpMessageHeader, which is useful for testing and general type usage.
19-27: LGTM: Test migration to macro-based approach.The test migration from manual Cursor-based assertions to the
test_binrw!macro is consistent with the PR-wide test refactor and improves maintainability.crates/smb-msg/src/oplock.rs (1)
96-108: LGTM: Test migration successful.The test migration to the
test_binrw!macro is consistent with the PR-wide refactor. The Gitleaks warning on line 102 is a false positive—the value is a test GUID, not a secret.crates/smb-dtyp/src/binrw_util/fixed_string.rs (2)
132-132: LGTM: Module path updated correctly.The module path change from
super::sized_wide_stringtosuper::sized_stringaligns with the reorganization wheredisplay_utf16is now provided via thesized_stringmodule.
139-139: LGTM: Import broadened for test macros.The import change to
use smb_tests::*is consistent with the PR-wide test migration pattern.crates/smb-msg/src/echo.rs (1)
20-30: LGTM: Test migration to macro-based approach.The test migration from manual Cursor-based assertions to the
test_binrw!macro is clean and consistent with the PR-wide refactor.crates/smb-dtyp/src/binrw_util/boolean.rs (2)
18-43: Verify hardcoded endian is intentional.Both
BinReadandBinWriteimplementations hardcodeEndian::Littleand ignore theendianparameter. SinceBooleanis a single byte, endianness doesn't matter functionally, but this inconsistency with the method signature might be worth documenting or verifying as intentional.
57-74: LGTM: Good test coverage for Boolean type.The tests appropriately validate true/false serialization and the MS-FSCC 2.1.8 behavior where any non-zero value is read as true.
crates/smb-msg/src/notify.rs (3)
13-13: LGTM: Added derives for equality comparisons.The addition of
PartialEqandEqis consistent with the PR-wide pattern of enabling equality comparisons on public message types.
90-91: LGTM: Conditional serialization and offset-aware writes.The additions optimize serialization by skipping empty buffers and properly using
PosMarker::write_aoff_sizefor offset-aware writes. This is consistent with similar serialization improvements across the PR.
141-209: LGTM: Comprehensive test migration.The test migration to macro-based tests is thorough, covering pending notifications (empty buffer), notifications with data, and a complex Azure scenario with multiple file notifications.
crates/smb-msg/src/file.rs (2)
13-13: LGTM: Added derives for equality comparisons.The addition of
PartialEqandEqtoFlushRequest,ReadRequest,WriteRequest,ReadFlags, andWriteFlagsis consistent with the PR-wide pattern and enables direct equality comparisons for testing and general usage.Also applies to: 39-39, 111-111, 137-137, 205-205
220-272: LGTM: Test migration with consistent macro invocation.The test migration is clean and all macro invocations consistently use the unqualified form after the wildcard import. The previous review comment about inconsistent macro invocation style appears to have been addressed.
crates/smb-dtyp/src/binrw_util/multi_sz.rs (3)
12-54: LGTM: Solid implementation of MultiWSz type.The
MultiWSztype correctly implements the REG_MULTI_SZ format with:
- Proper reading until an empty string terminator is encountered
- Writing all strings followed by an empty string terminator
- Clean BinRead/BinWrite implementations
56-74: LGTM: Good ergonomic trait implementations.The
Deref/DerefMutandFromIteratorimplementations provide convenient access to the underlyingVec<NullWideString>and easy construction from string iterators.
89-101: LGTM: Test validates binary format correctly.The test properly validates the MultiWSz binary format with multiple strings and the terminating empty string.
crates/smb-msg/src/header.rs (1)
240-240: LGTM: Test import update.The switch to
use smb_tests::*;aligns with the project-wide test harness migration strategy.crates/smb-fscc/src/common_info.rs (1)
366-366: LGTM: Simplified test import.The glob import
use smb_tests::*;is consistent with the test harness changes across the codebase.crates/smb-dtyp/src/security/sid.rs (3)
114-114: LGTM: Test import update.The glob import aligns with the project-wide test harness migration.
127-131: Good addition: Invalid SID string tests.Adding validation for invalid SID formats strengthens the test coverage and ensures proper error handling for malformed inputs.
133-136: LGTM: Binary test migration.The migration to
test_binrw!macro is consistent with the project-wide testing strategy.crates/smb-rpc/src/ndr64/align.rs (2)
90-90: LGTM: Test import update.The glob import aligns with the project-wide test harness migration.
94-115: LGTM: Test refactor with macro.The migration to
test_binrw!is clean and the test structure clearly demonstrates NDR64 alignment behavior with inline comments documenting each byte segment.crates/smb-msg/src/encrypted.rs (2)
63-63: LGTM: Test import update.The glob import aligns with the project-wide test harness migration.
67-95: LGTM: Test migration to macro-based approach.Both test cases (
e0ande1) are cleanly migrated to thetest_binrw!macro with clear structure and hex-encoded expected outputs.crates/smb-rpc/src/ndr64/ptr.rs (2)
243-243: LGTM: Test import update.The glob import aligns with the project-wide test harness migration.
255-266: LGTM: Null pointer test case.The test clearly demonstrates NdrPtr behavior with a null pointer, including alignment padding and the absence of data.
crates/smb-msg/src/ioctl/msg.rs (4)
191-191: LGTM: Test import update.The glob import aligns with the project-wide test harness migration.
197-216: LGTM: Test migration to macro-based approach.The migration to
test_request!andtest_response!macros with hex-encoded constants is clean and consistent with the project-wide testing strategy.Also applies to: 219-232
166-166: Verification confirms write_with hooks are correctly implemented per MS-SMB2 specification.The offset calculations and buffer serialization are already validated:
MS-SMB2 compliance: The assertion at line 152-153 enforces the spec requirement:
output_offset.value == 0 || output_offset.value == input_offset.value + input_countOffset calculation correctness: The PosMarker::write_aoff hook correctly manages offsets by capturing stream position before writing buffers, validated by the existing
test_response!macro which performs a serialization roundtrip against known hex buffer content.Binding correctness: The
args(&input_offset)andargs(&output_offset)syntax correctly passes PosMarker references as per binrw conventions for write_with hooks.The implementation is sound and tested.
145-145: No issues found—change is correct.The validation change is a bug fix aligning with SMB2 specification requirements. SMB2 specifies that InputCount SHOULD be 0 in most server responses, with pass-through exceptions noted. The original code incorrectly asserted
out_buffer.is_empty(), which would prevent normal IOCTL responses from having output data. The corrected assertionin_buffer.is_empty()properly validates that the response does not echo input, as per specification. The comment about pass-through operation exceptions is acknowledged but not yet implemented in code (consistent with the TODO at line 101 about non-fsctl ioctl support).crates/smb-tests/src/lib.rs (1)
1-3: LGTM: API simplification for test utilities.The change from
pub mod binrw;to a private module withpub use binrw::*;flattens the public API, allowing downstream test code to access binrw utilities directly viasmb_tests::*instead ofsmb_tests::binrw::*. This aligns with the glob import pattern used throughout the test modules in this PR.Note: This is a breaking change to the crate's public API, but since smb-tests is a dev-dependency used internally, the impact is contained to this workspace.
crates/smb-msg/src/info/set.rs (2)
12-12: LGTM! Added trait derivations enhance testability.Adding
PartialEqandEqderives enables direct struct comparison in tests, which aligns well with the macro-based test migration.
106-123: LGTM! Test migration to macro-based approach is clean.The test refactoring successfully adopts the new macro-driven test harness while maintaining the same test coverage.
crates/smb-dtyp/src/security/tests.rs (1)
2-94: LGTM! Test migration maintains coverage while reducing boilerplate.The conversion to
test_binrw!macro successfully consolidates the test definitions and expected outputs.crates/smb-rpc/src/ndr64.rs (1)
17-31: LGTM! Test refactoring correctly moves struct to module scope.The
TestNdrStringPtrstruct is properly moved to module scope, which is required for the macro-based test approach.crates/smb-rpc/src/ndr64/arrays.rs (2)
234-263: LGTM! Test refactoring properly moves structs to module scope.The test structures are correctly moved to module scope to support the macro-based test approach. Note that the TODO comment on line 261 was pre-existing and has already been addressed in a previous review comment.
265-281: LGTM! Test macro invocation is well-structured.The
test_binrw!macro properly exercises theWithArraystruct with array data.crates/smb-rpc/src/interface/srvsvc.rs (1)
242-308: LGTM! Test refactoring to macro-based approach is comprehensive.The
test_binrw_read!macro properly exercises theNetrShareEnumOutstruct with detailed test data.crates/smb-tests/src/binrw.rs (1)
31-31: LGTM! Corrected macro path to match function location.The path change correctly references
__hex_stream_decodeat the crate root, aligning with the function's actual location.crates/smb-msg/src/ioctl/fsctl.rs (2)
521-523: LGTM! Size tracking properly restored (previous issue resolved).The
write_with = PosMarker::write_sizeattribute correctly ensures thatsnap_shot_array_sizeis updated during serialization, addressing the previously flagged issue.
675-781: LGTM! Comprehensive test migration to macro-based approach.All tests have been successfully refactored to use the
test_binrw!macro, maintaining coverage while reducing boilerplate. The TODO at line 780 appropriately notes future test expansion.
| impl IntoIterator for MultiWSz { | ||
| type Item = String; | ||
| type IntoIter = std::vec::IntoIter<String>; | ||
|
|
||
| fn into_iter(self) -> Self::IntoIter { | ||
| self.0 | ||
| .into_iter() | ||
| .map(|s| s.to_string()) | ||
| .collect::<Vec<String>>() | ||
| .into_iter() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Optional: Simplify IntoIterator implementation.
The current implementation collects into a Vec<String> then immediately calls into_iter(), which creates an unnecessary intermediate allocation.
Consider this more efficient implementation:
impl IntoIterator for MultiWSz {
type Item = String;
- type IntoIter = std::vec::IntoIter<String>;
+ type IntoIter = std::iter::Map<std::vec::IntoIter<NullWideString>, fn(NullWideString) -> String>;
fn into_iter(self) -> Self::IntoIter {
- self.0
- .into_iter()
- .map(|s| s.to_string())
- .collect::<Vec<String>>()
- .into_iter()
+ self.0.into_iter().map(|s| s.to_string())
}
}Or, if the complex type is undesirable, use a boxed iterator:
impl IntoIterator for MultiWSz {
type Item = String;
- type IntoIter = std::vec::IntoIter<String>;
+ type IntoIter = std::iter::Map<std::vec::IntoIter<NullWideString>, fn(NullWideString) -> String>;
fn into_iter(self) -> Self::IntoIter {
- self.0
- .into_iter()
- .map(|s| s.to_string())
- .collect::<Vec<String>>()
- .into_iter()
+ self.0.into_iter().map(|s| s.to_string())
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/smb-dtyp/src/binrw_util/multi_sz.rs around lines 76–87, the
IntoIterator impl allocates a temporary Vec by collecting then calling
into_iter(); change the associated type to Box<dyn Iterator<Item = String>> and
return Box::new(self.0.into_iter().map(|s| s.to_string())) so you stream-map
without the intermediate allocation; update the signature accordingly (type
IntoIter = Box<dyn Iterator<Item = String>> and fn into_iter(self) ->
Self::IntoIter { Box::new(self.0.into_iter().map(|s| s.to_string())) }).
| /// Returns the GUID as a `u128` value. | ||
| pub fn as_u128(&self) -> u128 { | ||
| let mut bytes = [0u8; 16]; | ||
| { | ||
| let mut cursor = Cursor::new(&mut bytes[..]); | ||
| self.write(&mut cursor).unwrap(); | ||
| } | ||
| u128::from_le_bytes(bytes) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider a more direct implementation for efficiency.
The current implementation serializes the GUID to bytes via binrw and then interprets the bytes as a u128. While correct, this approach is less efficient than directly constructing the u128 from the struct's fields.
Apply this diff for a more efficient implementation:
- /// Returns the GUID as a `u128` value.
- pub fn as_u128(&self) -> u128 {
- let mut bytes = [0u8; 16];
- {
- let mut cursor = Cursor::new(&mut bytes[..]);
- self.write(&mut cursor).unwrap();
- }
- u128::from_le_bytes(bytes)
- }
+ /// Returns the GUID as a `u128` value.
+ pub fn as_u128(&self) -> u128 {
+ // Construct u128 directly from fields (little-endian)
+ (self.0 as u128)
+ | ((self.1 as u128) << 32)
+ | ((self.2 as u128) << 48)
+ | ((self.3[0] as u128) << 64)
+ | ((self.3[1] as u128) << 72)
+ | ((self.3[2] as u128) << 80)
+ | ((self.3[3] as u128) << 88)
+ | ((self.3[4] as u128) << 96)
+ | ((self.3[5] as u128) << 104)
+ | ((self.3[6] as u128) << 112)
+ | ((self.3[7] as u128) << 120)
+ }Committable suggestion skipped: line range outside the PR's diff.
| 0xf1u8, 0xad, 0x5e, 0x06, 0xaf, 0x6d, 0x43, 0x15, 0xb0, 0x4f, 0x10, 0xe6, 0x90, 0x84, 0xc9, | ||
| 0xae, | ||
| ]; | ||
| const TEST_GUID_BYTES: &'static str = "f1ad5e06af6d4315b04f10e69084c9ae"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove redundant 'static lifetime annotation.
The 'static lifetime annotation is redundant for const string literals, as it's implied by the const declaration.
- const TEST_GUID_BYTES: &'static str = "f1ad5e06af6d4315b04f10e69084c9ae";
+ const TEST_GUID_BYTES: &str = "f1ad5e06af6d4315b04f10e69084c9ae";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const TEST_GUID_BYTES: &'static str = "f1ad5e06af6d4315b04f10e69084c9ae"; | |
| const TEST_GUID_BYTES: &str = "f1ad5e06af6d4315b04f10e69084c9ae"; |
🤖 Prompt for AI Agents
In crates/smb-dtyp/src/guid.rs around line 184, the const declaration uses a
redundant 'static lifetime on a string literal: remove the "'static" lifetime
annotation from the const definition so it reads as a plain const string literal
(e.g., const TEST_GUID_BYTES: &str = "..."); update only that declaration and
ensure the rest of the code references the const unchanged.
| impl From<$name> for NegotiateContext { | ||
| fn from(val: $name) -> Self { | ||
| NegotiateContext { | ||
| context_type: NegotiateContextType::$name, | ||
| data: NegotiateContextValue::$name(val), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix NegotiateContext construction in the macro.
NegotiateContext now has the mandatory fields data_length: PosMarker<u16> and _reserved: u32. This literal only sets context_type and data, so it no longer compiles and would also leave _reserved uninitialised even if it did. Please populate the new fields (a zero _reserved and PosMarker::default() for data_length) when building the struct.
impl From<$name> for NegotiateContext {
fn from(val: $name) -> Self {
NegotiateContext {
context_type: NegotiateContextType::$name,
+ data_length: PosMarker::default(),
+ _reserved: 0,
data: NegotiateContextValue::$name(val),
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| impl From<$name> for NegotiateContext { | |
| fn from(val: $name) -> Self { | |
| NegotiateContext { | |
| context_type: NegotiateContextType::$name, | |
| data: NegotiateContextValue::$name(val), | |
| } | |
| } | |
| } | |
| impl From<$name> for NegotiateContext { | |
| fn from(val: $name) -> Self { | |
| NegotiateContext { | |
| context_type: NegotiateContextType::$name, | |
| data_length: PosMarker::default(), | |
| _reserved: 0, | |
| data: NegotiateContextValue::$name(val), | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/smb-msg/src/negotiate.rs around lines 281-288, the From<$name> for
NegotiateContext implementation only sets context_type and data but
NegotiateContext now requires data_length: PosMarker<u16> and _reserved: u32;
update the struct literal to include data_length: PosMarker::default() and
_reserved: 0 so the macro-generated constructor compiles and leaves no
uninitialized fields.
| use smb_tests::*; | ||
|
|
||
| use crate::*; | ||
|
|
||
| use super::*; | ||
|
|
||
| #[test] | ||
| pub fn test_tree_connect_req_write() { | ||
| let result = encode_content(TreeConnectRequest::new(&r"\\127.0.0.1\MyShare").into()); | ||
| assert_eq!( | ||
| result, | ||
| [ | ||
| 0x9, 0x0, 0x0, 0x0, 0x48, 0x0, 0x26, 0x0, 0x5c, 0x0, 0x5c, 0x0, 0x31, 0x0, 0x32, | ||
| 0x0, 0x37, 0x0, 0x2e, 0x0, 0x30, 0x0, 0x2e, 0x0, 0x30, 0x0, 0x2e, 0x0, 0x31, 0x0, | ||
| 0x5c, 0x0, 0x4d, 0x0, 0x79, 0x0, 0x53, 0x0, 0x68, 0x0, 0x61, 0x0, 0x72, 0x0, 0x65, | ||
| 0x0 | ||
| ] | ||
| ); | ||
| // TODO(test): Add tests with tree connect contexts. | ||
| test_request! { | ||
| TreeConnect { | ||
| flags: TreeConnectRequestFlags::new(), | ||
| buffer: r"\\adc.aviv.local\IPC$".into(), | ||
| tree_connect_contexts: vec![], | ||
| } => "0900000048002a005c005c006100640063002e0061007600690076002e006c006f00630061006c005c004900500043002400" | ||
| } | ||
|
|
||
| #[test] | ||
| pub fn test_tree_connect_res_parse() { | ||
| let mut cursor = Cursor::new(&[ | ||
| 0x10, 0x0, 0x1, 0x0, 0x0, 0x8, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xff, 0x1, 0x1f, 0x0, | ||
| ]); | ||
| let content_parsed = TreeConnectResponse::read_le(&mut cursor).unwrap(); | ||
| assert_eq!( | ||
| content_parsed, | ||
| TreeConnectResponse { | ||
| share_type: ShareType::Disk, | ||
| share_flags: ShareFlags::new().with_access_based_directory_enum(true), | ||
| capabilities: TreeCapabilities::new(), | ||
| maximal_access: 0x001f01ff, | ||
| } | ||
| ) | ||
| test_binrw! { | ||
| struct TreeConnectResponse { | ||
| share_type: ShareType::Disk, | ||
| share_flags: ShareFlags::new().with_access_based_directory_enum(true), | ||
| capabilities: TreeCapabilities::new(), | ||
| maximal_access: 0x001f01ff, | ||
| } => "100001000008000000000000ff011f00" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM: Test migration to macros aligns with PR objectives.
The test migration to test_request! and test_binrw! macros is clean and the test data appears valid. The hex-encoded expected outputs are properly formatted.
Regarding the TODO at line 332: tests with tree connect contexts would provide better coverage of the extension handling logic, particularly for the newly added tree_connect_context_count and _reserved fields.
Would you like me to help generate test cases with tree connect contexts, or open a new issue to track this task?
🤖 Prompt for AI Agents
crates/smb-msg/src/tree_connect.rs lines 326-348: the TODO indicates missing
tests exercising tree connect contexts and the new
tree_connect_context_count/_reserved fields; add one or more test_request! cases
that construct TreeConnect requests including non-empty tree_connect_contexts
(with varying context types and payloads) and assert the expected encoded hex,
and add corresponding test_binrw! responses that include parsed tree connect
contexts to validate decoding; ensure the tests set tree_connect_context_count
correctly, populate the _reserved field as required by the spec, and cover edge
cases (zero contexts, single context, multiple contexts) so both encoding and
decoding paths for the contexts are verified.
smb-msgstructssmb-msgsmb-dtyp(WIP)Summary by CodeRabbit
New Features
Documentation