Add opc_comn crate and pointer/memory macros to opc_classic_utils #23
Conversation
- Update Cargo.toml workspace dependencies - Clean up build.rs files in binding crates - Update Cargo.lock with latest dependency versions
…lity Key improvements: 1. Fix Drop implementation for Caller* types - CallerAllocatedArray: preserve pointer integrity, only clear length - CallerAllocatedPtr: maintain pointer validity - CallerAllocatedWString: keep pointer intact - Align with COM memory management conventions 2. Add convenience macros - write_caller_allocated_ptr!: write to caller-allocated pointers - write_caller_allocated_array!: write to caller-allocated arrays - alloc_callee_wstring!: allocate callee-allocated wide strings - All macros include comprehensive documentation and examples 3. Add opc_comn module - Implement OPC Common interface - Provide basic COM functionality support 4. Enhance documentation and examples - Update README.md with macro usage instructions - Add macro_usage.rs example file - Add detailed documentation for all macros 5. Code quality improvements - Pass all clippy checks - Pass all unit tests - Fix memory management issues These improvements make OPC Classic API development safer and more convenient.
WalkthroughThis change introduces a new crate, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant OpcServerBase
participant ImplT as T: OpcCommon + OpcShutdown
Client->>OpcServerBase: SetLocaleID(dwlcid)
OpcServerBase->>ImplT: set_locale_id(dwlcid)
ImplT-->>OpcServerBase: Result
OpcServerBase-->>Client: Result
Client->>OpcServerBase: GetLocaleID()
OpcServerBase->>ImplT: get_locale_id()
ImplT-->>OpcServerBase: Result<u32>
OpcServerBase-->>Client: Result<u32>
Client->>OpcServerBase: QueryAvailableLocaleIDs()
OpcServerBase->>ImplT: query_available_locale_ids()
ImplT-->>OpcServerBase: Result<Vec<u32>>
OpcServerBase-->>Client: Result
Client->>OpcServerBase: GetErrorString(dwerror)
OpcServerBase->>ImplT: get_error_string(dwerror)
ImplT-->>OpcServerBase: Result<String>
OpcServerBase-->>Client: Result<PWSTR>
Client->>OpcServerBase: SetClientName(szname)
OpcServerBase->>ImplT: set_client_name(szname)
ImplT-->>OpcServerBase: Result
OpcServerBase-->>Client: Result
Client->>OpcServerBase: ShutdownRequest(szreason)
OpcServerBase->>ImplT: shutdown_request(szreason)
ImplT-->>OpcServerBase: Result
OpcServerBase-->>Client: Result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learningsopc_classic_utils/src/memory/wstring.rs (4)Learnt from: Ronbb Learnt from: Ronbb Learnt from: Ronbb Learnt from: Ronbb opc_comn/src/client/opc_server_list.rs (5)Learnt from: Ronbb Learnt from: Ronbb Learnt from: Ronbb Learnt from: Ronbb Learnt from: Ronbb 🧬 Code Graph Analysis (1)opc_comn/src/client/opc_server_list.rs (2)
⏰ 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). (1)
🔇 Additional comments (19)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
opc_comn/.gitignore (1)
1-1: Consolidate ignore rules at the workspace rootIgnoring
target/is useful, but placing the rule in every crate quickly becomes repetitive.
If you already have a workspace-level.gitignore, consider keeping the rule there and deleting this file to reduce duplication.
Otherwise, no harm leaving it here.Cargo.toml (1)
6-7: Workspace member entry OK
opc_comnis correctly added to the member list and the dependency table.
Ensure every crate that depends on it uses{ workspace = true }rather than another direct path to avoid version drift.opc_comn/README.md (1)
1-9: Minor wording & spelling polish
- “OPC Comn” → “OPC Common” for clarity.
- Consider collapsing the two bold warnings into a single paragraph to reduce redundancy.
-# OPC Comn +# OPC Common @@ -**Warning: This library is still under development and may not be fully functional. Use at your own risk.** - -**Note: The API is unstable, tests are incomplete, and there may be unsafe memory operations.** +**Warning:** This library is under active development; the API may change, tests are incomplete, and some unsafe memory operations are still being reviewed. Use at your own risk.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.toml(2 hunks)opc_classic_utils/README.md(1 hunks)opc_classic_utils/examples/macro_usage.rs(1 hunks)opc_classic_utils/src/memory/array.rs(2 hunks)opc_classic_utils/src/memory/ptr.rs(2 hunks)opc_classic_utils/src/memory/wstring.rs(5 hunks)opc_comn/.gitignore(1 hunks)opc_comn/Cargo.toml(1 hunks)opc_comn/README.md(1 hunks)opc_comn/src/lib.rs(1 hunks)opc_comn/src/opc_common.rs(1 hunks)opc_da/Cargo.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
opc_comn/src/lib.rs (1)
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
opc_comn/README.md (3)
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file opc_da/src/client/tests.rs, do not add retry logic in test functions as per the user's preference.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
opc_da/Cargo.toml (2)
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
opc_comn/Cargo.toml (1)
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
opc_classic_utils/src/memory/array.rs (6)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like copy_to_pointer are unnecessary and should be omitted.
Learnt from: Ronbb
PR: #10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In opc_da/src/client/iterator.rs, when initializing the cache array in GroupIterator, use cache: [const { None }; 16] instead of cache: [None; 16] because Flipper requires this change.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/item_properties.rs:58-62
Timestamp: 2024-12-11T01:41:07.039Z
Learning: When count is of type u32, it's safe to set array lengths without additional bounds checking.
opc_classic_utils/src/memory/ptr.rs (4)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like copy_to_pointer are unnecessary and should be omitted.
opc_classic_utils/README.md (1)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
opc_classic_utils/examples/macro_usage.rs (1)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file opc_da/src/client/tests.rs, do not add retry logic in test functions as per the user's preference.
opc_comn/src/opc_common.rs (4)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
opc_classic_utils/src/memory/wstring.rs (3)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
🧬 Code Graph Analysis (2)
opc_classic_utils/src/memory/array.rs (2)
opc_classic_utils/src/memory/ptr.rs (2)
as_ptr(58-60)as_ptr(178-180)opc_classic_utils/src/memory/ptr_array.rs (2)
as_ptr(78-80)as_ptr(253-255)
opc_classic_utils/src/memory/ptr.rs (3)
opc_classic_utils/src/memory/array.rs (2)
from_raw(27-29)from_raw(241-243)opc_classic_utils/src/memory/wstring.rs (2)
from_raw(30-32)from_raw(206-208)opc_classic_utils/src/memory/ptr_array.rs (2)
from_raw(27-29)from_raw(248-250)
⏰ 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). (1)
- GitHub Check: analyzing
🔇 Additional comments (19)
opc_da/Cargo.toml (1)
8-8: Keyword addition looks goodAdding
opcdaimproves discoverability and still keeps the list under the crates.io limit of five keywords.
No further action required.opc_comn/Cargo.toml (1)
1-20: LGTM! Well-structured Cargo.toml for OPC Common crate.The configuration is appropriate for an OPC Classic implementation:
- Rust 2024 edition is current and suitable
- Windows MSVC target aligns with OPC's Windows-specific nature
- Workspace dependencies maintain consistency across the project
- MIT license and descriptive metadata are well-chosen
opc_classic_utils/src/memory/ptr.rs (2)
106-108: Good alignment with COM memory management conventions.The updated Drop implementation correctly preserves the pointer for the callee to access and free, which is the expected behavior for caller-allocated memory in COM interfaces.
284-295: Excellent macro design with proper safety and error handling.The
write_caller_allocated_ptr!macro provides a clean abstraction that:
- Safely wraps raw pointers using
CallerAllocatedPtr::from_raw- Handles null pointer cases with appropriate COM error (
E_INVALIDARG)- Maintains unsafe context to remind users of safety requirements
- Includes comprehensive documentation with usage examples
The implementation correctly delegates to the underlying pointer management while providing ergonomic error handling.
opc_classic_utils/README.md (1)
90-124: Excellent documentation for the new convenience macros.The new "Convenience Macros" section provides:
- Clear purpose statements for each macro
- Practical usage examples with proper error handling
- Consistent formatting that matches the existing documentation style
- Good coverage of the three main COM memory management scenarios
This documentation will help users adopt the new macros effectively while understanding the underlying memory management patterns.
opc_classic_utils/src/memory/array.rs (2)
165-169: Consistent Drop behavior with caller-allocated memory conventions.The updated Drop implementation correctly preserves the pointer while clearing the length, maintaining consistency with the
CallerAllocatedPtr<T>changes and COM memory management patterns.
404-411: Well-designed macro with excellent composition.The
write_caller_allocated_array!macro demonstrates good software engineering principles:
- Reuses existing functionality (
write_caller_allocated_ptr!andCallerAllocatedArray::from_slice)- Maintains consistent error handling patterns
- Provides comprehensive documentation including memory management details
- Handles the complex array allocation and pointer writing in a simple interface
The composition approach reduces code duplication and maintains consistency across the macro family.
opc_classic_utils/examples/macro_usage.rs (1)
1-115: Comprehensive and well-structured example demonstrating macro usage.This example file excels in several areas:
Educational Value:
- Progresses from simple macro usage to complex OPC scenarios
- Each example is clearly commented and demonstrates different use cases
- Shows practical applications like locale ID handling and error string allocation
Code Quality:
- Proper error handling with
?operator throughout- Clean separation between demonstration and testing code
- Clear console output for interactive learning
Testing Coverage:
- Unit tests validate each macro's core functionality
- Tests cover successful cases and verify expected outcomes
- Follows good testing practices without unnecessary complexity
The OPC Common interface simulation (Example 4) is particularly valuable as it shows how the macros work together in realistic scenarios.
opc_comn/src/opc_common.rs (6)
1-4: LGTM! Well-structured COM interface wrapper.The generic struct with the 'static bound is appropriate for COM objects that require stable memory locations.
7-13: LGTM! Clean delegation pattern.Both
SetLocaleIDandGetLocaleIDcorrectly delegate to their corresponding trait methods.
15-25: LGTM! Proper handling of COM output parameters.The method correctly uses utility macros to safely write count and array data to caller-allocated pointers, with appropriate error handling for the length conversion.
27-32: LGTM! Correct COM string allocation pattern.The method properly uses the
alloc_callee_wstring!macro to allocate and return a wide string following COM conventions.
34-40: LGTM! Safe handling of wide string input.The method correctly converts the wide string pointer to a Rust String with appropriate error handling, returning
E_POINTERfor invalid inputs.
43-51: LGTM! Clean trait design.The trait provides a safe Rust interface with appropriate error handling through
Resulttypes, abstracting away raw pointer handling from implementors.opc_classic_utils/src/memory/wstring.rs (5)
5-5: LGTM! Required import for new functionality.Adding
PWSTRimport is necessary for the newas_pwstr()methods.
132-135: LGTM! Consistent API for mutable pointer access.The
as_pwstr()methods complement the existingas_pcwstr()methods, providing access to mutable wide string pointers when needed.Also applies to: 284-287
141-142: LGTM! Clarifying comments improve code understanding.The updated comments clearly explain why the pointer is preserved in the Drop implementation, which is correct for caller-allocated memory patterns.
217-221: LGTM! Convenient string conversion method.The
from_stringmethod provides API consistency withCallerAllocatedWStringand correctly delegates to theFromStrimplementation.
341-397: LGTM! Well-designed and documented macro.The
alloc_callee_wstring!macro provides a clean API for COM string allocation with comprehensive documentation and examples. The implementation correctly leverages theFromStrtrait.
| impl std::str::FromStr for CalleeAllocatedWString { | ||
| type Err = windows::core::Error; | ||
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| let wide_string: Vec<u16> = OsStr::new(s) | ||
| .encode_wide() | ||
| .chain(std::iter::once(0)) | ||
| .collect(); | ||
| let len = wide_string.len() - 1; // Exclude null terminator for allocation | ||
| let ptr = unsafe { CoTaskMemAlloc(len * std::mem::size_of::<u16>()) }; | ||
| if ptr.is_null() { | ||
| return Err(windows::core::Error::from_win32()); | ||
| } | ||
| unsafe { | ||
| std::ptr::copy_nonoverlapping(wide_string.as_ptr(), ptr.cast(), wide_string.len()); | ||
| } | ||
| Ok(unsafe { Self::new(ptr.cast()) }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix buffer overflow in memory allocation.
The memory allocation on line 330 doesn't account for the null terminator, causing a buffer overflow when copying the entire wide_string (which includes the null terminator).
Apply this fix:
- let ptr = unsafe { CoTaskMemAlloc(len * std::mem::size_of::<u16>()) };
+ let ptr = unsafe { CoTaskMemAlloc((len + 1) * std::mem::size_of::<u16>()) };The allocation should include space for the null terminator since wide_string.len() includes it but len doesn't.
📝 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 std::str::FromStr for CalleeAllocatedWString { | |
| type Err = windows::core::Error; | |
| fn from_str(s: &str) -> Result<Self, Self::Err> { | |
| let wide_string: Vec<u16> = OsStr::new(s) | |
| .encode_wide() | |
| .chain(std::iter::once(0)) | |
| .collect(); | |
| let len = wide_string.len() - 1; // Exclude null terminator for allocation | |
| let ptr = unsafe { CoTaskMemAlloc(len * std::mem::size_of::<u16>()) }; | |
| if ptr.is_null() { | |
| return Err(windows::core::Error::from_win32()); | |
| } | |
| unsafe { | |
| std::ptr::copy_nonoverlapping(wide_string.as_ptr(), ptr.cast(), wide_string.len()); | |
| } | |
| Ok(unsafe { Self::new(ptr.cast()) }) | |
| } | |
| } | |
| impl std::str::FromStr for CalleeAllocatedWString { | |
| type Err = windows::core::Error; | |
| fn from_str(s: &str) -> Result<Self, Self::Err> { | |
| let wide_string: Vec<u16> = OsStr::new(s) | |
| .encode_wide() | |
| .chain(std::iter::once(0)) | |
| .collect(); | |
| let len = wide_string.len() - 1; // Exclude null terminator for allocation | |
| let ptr = unsafe { CoTaskMemAlloc((len + 1) * std::mem::size_of::<u16>()) }; | |
| if ptr.is_null() { | |
| return Err(windows::core::Error::from_win32()); | |
| } | |
| unsafe { | |
| std::ptr::copy_nonoverlapping(wide_string.as_ptr(), ptr.cast(), wide_string.len()); | |
| } | |
| Ok(unsafe { Self::new(ptr.cast()) }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In opc_classic_utils/src/memory/wstring.rs lines 321 to 339, the memory
allocation size for the wide string does not include space for the null
terminator, causing a buffer overflow during copy. Fix this by allocating memory
for wide_string.len() elements instead of len, ensuring the null terminator is
included in the allocated buffer size.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
clippy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
opc_classic_utils/src/memory/ptr_array.rs (1)
276-279: Fix documentation inconsistencyThe comment states "freed by the caller" but this is a
CalleeAllocatedPtrArraywhere memory should be "freed by the callee"./// Allocates memory for a pointer array using `CoTaskMemAlloc` and creates a `CalleeAllocatedPtrArray` /// -/// This allocates memory that will be freed by the caller (COM function). -/// The callee is responsible for ensuring the caller will free this memory. +/// This allocates memory that will be freed by the caller. +/// The callee (COM function) is responsible for returning this memory to the caller who will free it.opc_da/src/opc_data_callback.rs (1)
24-50: Consider using slices instead of Vec to avoid unnecessary allocationsThe current implementation converts slices to Vec, which causes unnecessary heap allocations. Since the data is already in memory, the trait methods could accept slices instead.
Consider modifying the trait to accept slices:
pub trait OPCDataCallback { fn on_data_change( &self, transaction_id: u32, group: u32, quality: HRESULT, error: HRESULT, client_items: &[u32], values: &[windows::Win32::System::Variant::VARIANT], qualities: &[u16], timestamps: &[windows::Win32::Foundation::FILETIME], errors: &[HRESULT], ) -> Result<()>; // ... other methods }This would eliminate the
.to_vec()calls and improve performance for large datasets.opc_da/src/opc_browse.rs (1)
93-95: Browse continuation point not implemented.The
pszcontinuationpointparameter is always set to null, which means browse operations cannot be continued if there are more elements thandwmaxelementsreturned.Consider implementing continuation point support if the underlying trait supports it, or document this limitation in the trait definition.
opc_da/src/opc_group.rs (1)
185-202: Unused trait methods in OPCGroupStateMgt2.The trait defines
get_state2andset_state2methods that are never called in the implementation. These seem to be extended versions that include the keep-alive parameter.Consider whether these methods are needed. If they're part of a future implementation, add a TODO comment. Otherwise, remove them to simplify the trait.
opc_da/src/opc_sync_io.rs (3)
1-3: Consider using specific imports instead of wildcards.For better code clarity and to avoid namespace pollution, consider importing only the specific types and functions needed.
26-41: Consider using memory management macros for consistency.The codebase provides memory management macros in
opc_classic_utils. Consider usingwrite_caller_allocated_array!for allocating and writing the item states array.- // Allocate and write item states - if !ppitemvalues.is_null() { - let size = std::mem::size_of::<tagOPCITEMSTATE>() * item_states.len(); - let ptr = unsafe { windows::Win32::System::Com::CoTaskMemAlloc(size) }; - if ptr.is_null() { - return Err(windows::core::Error::from_win32()); - } - - unsafe { - std::ptr::copy_nonoverlapping( - item_states.as_ptr(), - ptr.cast(), - item_states.len() - ); - *ppitemvalues = ptr.cast(); - } - } + // Allocate and write item states + write_caller_allocated_array!(ppitemvalues, item_states)?;
122-138: Use memory management macros for array allocations in ReadMaxAge.Similar to the previous suggestion, consider using the memory management macros for these allocations as well.
- // Allocate and write values - if !ppvvalues.is_null() { - let size = std::mem::size_of::<windows::Win32::System::Variant::VARIANT>() * values.len(); - let ptr = unsafe { windows::Win32::System::Com::CoTaskMemAlloc(size) }; - if ptr.is_null() { - return Err(windows::core::Error::from_win32()); - } - - unsafe { - std::ptr::copy_nonoverlapping( - values.as_ptr(), - ptr.cast(), - values.len() - ); - *ppvvalues = ptr.cast(); - } - } + // Allocate and write values + write_caller_allocated_array!(ppvvalues, values)?;Apply similar changes to qualities and timestamps allocations.
Also applies to: 140-156, 158-174
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
opc_classic_utils/src/memory/array.rs(2 hunks)opc_classic_utils/src/memory/ptr.rs(3 hunks)opc_classic_utils/src/memory/ptr_array.rs(2 hunks)opc_classic_utils/src/memory/wstring.rs(6 hunks)opc_comn/src/lib.rs(1 hunks)opc_comn/src/opc_common.rs(1 hunks)opc_da/src/lib.rs(1 hunks)opc_da/src/opc_async_io.rs(1 hunks)opc_da/src/opc_browse.rs(1 hunks)opc_da/src/opc_data_callback.rs(1 hunks)opc_da/src/opc_group.rs(1 hunks)opc_da/src/opc_item.rs(1 hunks)opc_da/src/opc_server.rs(1 hunks)opc_da/src/opc_sync_io.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- opc_da/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- opc_comn/src/lib.rs
- opc_classic_utils/src/memory/wstring.rs
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
Learnt from: Ronbb
PR: Ronbb/rust_opc#10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In `opc_da/src/client/iterator.rs`, when initializing the `cache` array in `GroupIterator`, use `cache: [const { None }; 16]` instead of `cache: [None; 16]` because Flipper requires this change.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the `opc_da/src/com/server.rs` file (Rust), the enum variant `OPC_NS_HIERARCHIAL` is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In `opc_da/src/traits/group.rs`, the `GroupTrait` trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
opc_classic_utils/src/memory/ptr.rs (4)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like copy_to_pointer are unnecessary and should be omitted.
opc_da/src/opc_server.rs (7)
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/unified/group.rs:28-39
Timestamp: 2024-12-10T13:51:14.061Z
Learning: In the add_items method in opc_da/src/client/unified/group.rs, the OPC DA protocol guarantees that the results and errors arrays returned will always be of the same length, so additional validation for array length mismatch is not necessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file opc_da/src/client/tests.rs, do not add retry logic in test functions as per the user's preference.
opc_classic_utils/src/memory/array.rs (6)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/item_properties.rs:58-62
Timestamp: 2024-12-11T01:41:07.039Z
Learning: When count is of type u32, it's safe to set array lengths without additional bounds checking.
Learnt from: Ronbb
PR: #10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In opc_da/src/client/iterator.rs, when initializing the cache array in GroupIterator, use cache: [const { None }; 16] instead of cache: [None; 16] because Flipper requires this change.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like copy_to_pointer are unnecessary and should be omitted.
opc_classic_utils/src/memory/ptr_array.rs (5)
Learnt from: Ronbb
PR: #10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In opc_da/src/client/iterator.rs, when initializing the cache array in GroupIterator, use cache: [const { None }; 16] instead of cache: [None; 16] because Flipper requires this change.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like copy_to_pointer are unnecessary and should be omitted.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
opc_comn/src/opc_common.rs (5)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/async_io2.rs:94-95
Timestamp: 2024-12-11T01:29:33.264Z
Learning: In AsyncIo2Trait within opc_da/src/client/traits/async_io2.rs, when directly returning a Result from a method like Refresh2, it's acceptable to return the result as is without additional wrapping.
opc_da/src/opc_item.rs (8)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/item_mgt.rs:191-194
Timestamp: 2024-12-10T03:00:26.689Z
Learning: In opc_da/src/client/traits/item_mgt.rs, the CreateEnumerator method returns a Result, so there is no need to handle None values.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/unified/group.rs:28-39
Timestamp: 2024-12-10T13:51:14.061Z
Learning: In the add_items method in opc_da/src/client/unified/group.rs, the OPC DA protocol guarantees that the results and errors arrays returned will always be of the same length, so additional validation for array length mismatch is not necessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/async_io2.rs:94-95
Timestamp: 2024-12-11T01:29:33.264Z
Learning: In AsyncIo2Trait within opc_da/src/client/traits/async_io2.rs, when directly returning a Result from a method like Refresh2, it's acceptable to return the result as is without additional wrapping.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
opc_da/src/opc_data_callback.rs (4)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/async_io2.rs:94-95
Timestamp: 2024-12-11T01:29:33.264Z
Learning: In AsyncIo2Trait within opc_da/src/client/traits/async_io2.rs, when directly returning a Result from a method like Refresh2, it's acceptable to return the result as is without additional wrapping.
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
opc_da/src/opc_async_io.rs (7)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/async_io2.rs:94-95
Timestamp: 2024-12-11T01:29:33.264Z
Learning: In AsyncIo2Trait within opc_da/src/client/traits/async_io2.rs, when directly returning a Result from a method like Refresh2, it's acceptable to return the result as is without additional wrapping.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file opc_da/src/client/tests.rs, do not add retry logic in test functions as per the user's preference.
opc_da/src/opc_browse.rs (5)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/item_mgt.rs:191-194
Timestamp: 2024-12-10T03:00:26.689Z
Learning: In opc_da/src/client/traits/item_mgt.rs, the CreateEnumerator method returns a Result, so there is no need to handle None values.
opc_da/src/opc_sync_io.rs (5)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/async_io2.rs:94-95
Timestamp: 2024-12-11T01:29:33.264Z
Learning: In AsyncIo2Trait within opc_da/src/client/traits/async_io2.rs, when directly returning a Result from a method like Refresh2, it's acceptable to return the result as is without additional wrapping.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
opc_da/src/opc_group.rs (7)
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/unified/group.rs:28-39
Timestamp: 2024-12-10T13:51:14.061Z
Learning: In the add_items method in opc_da/src/client/unified/group.rs, the OPC DA protocol guarantees that the results and errors arrays returned will always be of the same length, so additional validation for array length mismatch is not necessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/async_io2.rs:94-95
Timestamp: 2024-12-11T01:29:33.264Z
Learning: In AsyncIo2Trait within opc_da/src/client/traits/async_io2.rs, when directly returning a Result from a method like Refresh2, it's acceptable to return the result as is without additional wrapping.
🧬 Code Graph Analysis (5)
opc_classic_utils/src/memory/array.rs (2)
opc_classic_utils/src/memory/ptr_array.rs (12)
as_ptr(80-82)as_ptr(305-307)from_raw(29-31)from_raw(250-252)len(96-98)len(321-323)is_null(106-108)is_null(331-333)new(24-26)new(243-245)as_mut_slice(128-134)as_mut_slice(353-359)opc_classic_utils/src/memory/ptr.rs (8)
as_ptr(58-60)as_ptr(175-177)from_raw(27-29)from_raw(155-157)is_null(72-74)is_null(189-191)new(22-24)new(148-150)
opc_da/src/opc_async_io.rs (1)
opc_da/src/opc_sync_io.rs (16)
Read(11-62)std(27-27)std(45-45)std(82-82)std(124-124)std(142-142)std(160-160)std(178-178)std(215-215)Write(64-99)read(239-239)write(241-241)ReadMaxAge(103-195)WriteVQT(197-232)read_max_age(245-245)write_vqt(247-247)
opc_da/src/opc_browse.rs (3)
opc_classic_utils/src/memory/ptr_array.rs (3)
std(45-45)std(288-288)from_slice(255-274)opc_classic_utils/src/memory/wstring.rs (4)
std(52-52)std(337-337)from_pcwstr(35-39)from_pcwstr(218-222)opc_classic_utils/src/memory/array.rs (2)
std(43-43)from_slice(60-76)
opc_da/src/opc_sync_io.rs (1)
opc_da/src/opc_async_io.rs (18)
Read(11-47)Read(111-146)std(30-30)std(71-71)std(129-129)std(170-170)std(234-234)std(275-275)Write(49-88)Write(148-187)ReadMaxAge(212-251)WriteVQT(253-292)read(307-307)read(317-317)write(309-309)write(319-319)read_max_age(331-331)write_vqt(333-333)
opc_da/src/opc_group.rs (1)
opc_classic_utils/src/memory/wstring.rs (2)
from_pcwstr(35-39)from_pcwstr(218-222)
⏰ 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). (1)
- GitHub Check: analyzing
🔇 Additional comments (11)
opc_classic_utils/src/memory/ptr.rs (3)
103-109: LGTM! Clear documentation of memory ownership semanticsThe updated Drop implementation and comments clearly document that the pointer is intentionally preserved for the callee to use, which aligns with the caller-allocated memory pattern in COM.
162-162: Good improvement to API flexibilityRemoving the
T: Copyconstraint fromfrom_valuemakes the method more flexible while maintaining safety throughcopy_nonoverlapping.
281-292: Well-designed convenience macroThe
write_caller_allocated_ptr!macro provides a safe and ergonomic way to write values to caller-allocated pointers with proper error handling.opc_da/src/opc_server.rs (2)
11-58: LGTM! Well-structured COM interface implementationThe
AddGroupimplementation properly handles:
- Safe string conversion from PCWSTR
- Optional pointer parameters with null checks
- Output parameter writing using the convenience macros
- Proper error propagation
82-86: LGTM! Correct memory management for COM return valuesThe
GetStatusmethod correctly allocates COM memory for the return value and transfers ownership to the caller.opc_classic_utils/src/memory/array.rs (3)
162-170: LGTM! Consistent memory ownership semanticsThe Drop implementation correctly preserves the pointer for the callee while clearing the length, consistent with the CallerAllocatedPtr behavior.
403-411: Elegant macro compositionThe
write_caller_allocated_array!macro cleanly composes existing functionality by combiningCallerAllocatedArray::from_slicewithwrite_caller_allocated_ptr!.
457-478: Well-designed direct copy macroThe
copy_to_caller_array!macro provides safe direct memory copying with proper null checks and error handling. Good use ofcopy_from_slicefor efficient copying.opc_da/src/opc_group.rs (1)
87-105: Unused return values from set_state method.The
set_statemethod returns 6 values but onlyrevised_update_rateis used. The other values are ignored, which might indicate missing functionality.Either use all returned values or modify the trait to only return what's needed:
- let ( - revised_update_rate, - new_active, - new_time_bias, - new_percent_deadband, - new_locale_id, - new_client_group, - ) = self.0.set_state( + let revised_update_rate = self.0.set_state( requested_update_rate, active, time_bias, percent_deadband, locale_id, client_group, )?;Alternatively, if the COM interface requires these values to be returned, they should be written back to the caller through the corresponding output pointers.
opc_comn/src/opc_common.rs (1)
93-119: Optimize array handling and reduce code duplication.The current implementation creates unnecessary intermediate vectors and duplicates code between
OPCServerListandOPCServerList2.Extract a helper function:
fn guid_array_to_vec(ptr: *const windows_core::GUID, count: u32) -> Vec<windows_core::GUID> { if ptr.is_null() || count == 0 { Vec::new() } else { unsafe { std::slice::from_raw_parts(ptr, count as usize).to_vec() } } }Then simplify both implementations:
- let implemented = if !rgcatidimpl.is_null() { - unsafe { - opc_classic_utils::CallerAllocatedArray::new( - rgcatidimpl as *mut windows_core::GUID, - cimplemented as usize, - ) - .as_slice() - .unwrap_or(&[]) - .to_vec() - } - } else { - Vec::new() - }; + let implemented = guid_array_to_vec(rgcatidimpl, cimplemented);Also applies to: 171-197
⛔ Skipped due to learnings
Learnt from: Ronbb PR: Ronbb/rust_opc#9 File: opc_da/src/client/unified/group.rs:28-39 Timestamp: 2024-12-10T13:51:14.061Z Learning: In the `add_items` method in `opc_da/src/client/unified/group.rs`, the OPC DA protocol guarantees that the `results` and `errors` arrays returned will always be of the same length, so additional validation for array length mismatch is not necessary.Learnt from: Ronbb PR: Ronbb/rust_opc#8 File: opc_da/src/com/server.rs:714-716 Timestamp: 2024-11-29T12:06:30.463Z Learning: In the `opc_da/src/com/server.rs` file (Rust), the enum variant `OPC_NS_HIERARCHIAL` is correctly spelled as per the OPC specification, even though it appears misspelled.Learnt from: Ronbb PR: Ronbb/rust_opc#10 File: opc_da/src/client/iterator.rs:114-114 Timestamp: 2024-12-13T18:47:42.521Z Learning: In `opc_da/src/client/iterator.rs`, when initializing the `cache` array in `GroupIterator`, use `cache: [const { None }; 16]` instead of `cache: [None; 16]` because Flipper requires this change.Learnt from: Ronbb PR: Ronbb/rust_opc#9 File: opc_da/src/client/tests.rs:8-35 Timestamp: 2024-12-11T01:49:14.219Z Learning: In the file `opc_da/src/client/tests.rs`, do not add retry logic in test functions as per the user's preference.Learnt from: Ronbb PR: Ronbb/rust_opc#9 File: opc_da/src/client/traits/item_mgt.rs:191-194 Timestamp: 2024-12-10T03:00:26.689Z Learning: In `opc_da/src/client/traits/item_mgt.rs`, the `CreateEnumerator` method returns a `Result`, so there is no need to handle `None` values.Learnt from: Ronbb PR: Ronbb/rust_opc#9 File: opc_da/src/client/tests.rs:0-0 Timestamp: 2024-12-11T01:25:38.423Z Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.Learnt from: Ronbb PR: Ronbb/rust_opc#8 File: opc_da/src/com/utils.rs:61-87 Timestamp: 2024-11-28T15:39:26.903Z Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.Learnt from: Ronbb PR: Ronbb/rust_opc#9 File: opc_da/src/client/v3/mod.rs:67-78 Timestamp: 2024-12-10T14:06:24.207Z Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.Learnt from: Ronbb PR: Ronbb/rust_opc#8 File: opc_da/src/traits/group.rs:1-122 Timestamp: 2024-12-02T14:43:35.670Z Learning: In `opc_da/src/traits/group.rs`, the `GroupTrait` trait contains only method declarations without implementations.opc_da/src/opc_sync_io.rs (1)
235-249: Clean trait design!The trait definitions follow a clear hierarchy with
OPCSyncIO2extendingOPCSyncIO, and the method signatures are well-designed for Rust usage.
| /// Creates a new `CalleeAllocatedPtrArray` from a slice | ||
| pub fn from_slice(slice: &[T]) -> Result<Self, windows::core::Error> { | ||
| if slice.is_empty() { | ||
| return Ok(Self { | ||
| ptr: ptr::null_mut(), | ||
| len: 0, | ||
| }); | ||
| } | ||
| let array = Self::allocate(slice.len())?; | ||
|
|
||
| // convert items to CalleeAllocatedPtr write to array | ||
| for (i, item) in slice.iter().enumerate() { | ||
| let item_ptr = CalleeAllocatedPtr::from_value(item)?; | ||
| if item_ptr.is_null() { | ||
| return Err(windows::core::Error::from_win32()); | ||
| } | ||
| unsafe { *array.ptr.add(i) = item_ptr.as_ptr() }; | ||
| } | ||
|
|
||
| Ok(array) | ||
| } |
There was a problem hiding this comment.
Fix memory leak in error handling
The from_slice method has a memory leak issue. If CalleeAllocatedPtr::from_value fails after successfully allocating some items, those previously allocated pointers won't be freed. Additionally, the error check at line 268 doesn't properly propagate the error from from_value.
Apply this fix to handle errors properly and prevent memory leaks:
pub fn from_slice(slice: &[T]) -> Result<Self, windows::core::Error> {
if slice.is_empty() {
return Ok(Self {
ptr: ptr::null_mut(),
len: 0,
});
}
let array = Self::allocate(slice.len())?;
// convert items to CalleeAllocatedPtr write to array
for (i, item) in slice.iter().enumerate() {
- let item_ptr = CalleeAllocatedPtr::from_value(item)?;
- if item_ptr.is_null() {
- return Err(windows::core::Error::from_win32());
+ match CalleeAllocatedPtr::from_value(item) {
+ Ok(item_ptr) => {
+ unsafe { *array.ptr.add(i) = item_ptr.into_raw() };
+ }
+ Err(e) => {
+ // Clean up previously allocated items
+ unsafe {
+ for j in 0..i {
+ let ptr = *array.ptr.add(j);
+ if !ptr.is_null() {
+ CoTaskMemFree(Some(ptr.cast()));
+ }
+ }
+ // Clean up the array itself
+ CoTaskMemFree(Some(array.ptr.cast()));
+ }
+ return Err(e);
+ }
}
- unsafe { *array.ptr.add(i) = item_ptr.as_ptr() };
}
Ok(array)
}📝 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.
| /// Creates a new `CalleeAllocatedPtrArray` from a slice | |
| pub fn from_slice(slice: &[T]) -> Result<Self, windows::core::Error> { | |
| if slice.is_empty() { | |
| return Ok(Self { | |
| ptr: ptr::null_mut(), | |
| len: 0, | |
| }); | |
| } | |
| let array = Self::allocate(slice.len())?; | |
| // convert items to CalleeAllocatedPtr write to array | |
| for (i, item) in slice.iter().enumerate() { | |
| let item_ptr = CalleeAllocatedPtr::from_value(item)?; | |
| if item_ptr.is_null() { | |
| return Err(windows::core::Error::from_win32()); | |
| } | |
| unsafe { *array.ptr.add(i) = item_ptr.as_ptr() }; | |
| } | |
| Ok(array) | |
| } | |
| /// Creates a new `CalleeAllocatedPtrArray` from a slice | |
| pub fn from_slice(slice: &[T]) -> Result<Self, windows::core::Error> { | |
| if slice.is_empty() { | |
| return Ok(Self { | |
| ptr: ptr::null_mut(), | |
| len: 0, | |
| }); | |
| } | |
| let array = Self::allocate(slice.len())?; | |
| // convert items to CalleeAllocatedPtr write to array | |
| for (i, item) in slice.iter().enumerate() { | |
| match CalleeAllocatedPtr::from_value(item) { | |
| Ok(item_ptr) => { | |
| unsafe { *array.ptr.add(i) = item_ptr.into_raw() }; | |
| } | |
| Err(e) => { | |
| // Clean up previously allocated items | |
| unsafe { | |
| for j in 0..i { | |
| let ptr = *array.ptr.add(j); | |
| if !ptr.is_null() { | |
| CoTaskMemFree(Some(ptr.cast())); | |
| } | |
| } | |
| // Clean up the array itself | |
| CoTaskMemFree(Some(array.ptr.cast())); | |
| } | |
| return Err(e); | |
| } | |
| } | |
| } | |
| Ok(array) | |
| } |
🤖 Prompt for AI Agents
In opc_classic_utils/src/memory/ptr_array.rs lines 254 to 274, the from_slice
method leaks memory if CalleeAllocatedPtr::from_value fails after some
allocations because previously allocated pointers are not freed. To fix this,
track all successfully allocated pointers and on any error, free them before
returning the error. Also, propagate the actual error from from_value instead of
creating a generic one. Implement proper cleanup in the error path to prevent
leaks.
| if !pperrors.is_null() { | ||
| let size = std::mem::size_of::<HRESULT>() * errors.len(); | ||
| let ptr = unsafe { windows::Win32::System::Com::CoTaskMemAlloc(size) }; | ||
| if ptr.is_null() { | ||
| return Err(windows::core::Error::from_win32()); | ||
| } | ||
|
|
||
| unsafe { | ||
| std::ptr::copy_nonoverlapping( | ||
| errors.as_ptr(), | ||
| ptr.cast(), | ||
| errors.len() | ||
| ); | ||
| *pperrors = ptr.cast(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract repeated error allocation logic into a helper function.
The error array allocation pattern is duplicated across multiple methods. Consider extracting this into a helper function to improve maintainability and reduce code duplication.
Add a helper function:
fn allocate_and_write_errors(pperrors: *mut *mut HRESULT, errors: &[HRESULT]) -> Result<()> {
if !pperrors.is_null() {
let size = std::mem::size_of::<HRESULT>() * errors.len();
let ptr = unsafe { windows::Win32::System::Com::CoTaskMemAlloc(size) };
if ptr.is_null() {
return Err(windows::core::Error::from_win32());
}
unsafe {
std::ptr::copy_nonoverlapping(
errors.as_ptr(),
ptr.cast(),
errors.len()
);
*pperrors = ptr.cast();
}
}
Ok(())
}Then simplify the methods:
- // Allocate and write errors
- if !pperrors.is_null() {
- let size = std::mem::size_of::<HRESULT>() * errors.len();
- let ptr = unsafe { windows::Win32::System::Com::CoTaskMemAlloc(size) };
- if ptr.is_null() {
- return Err(windows::core::Error::from_win32());
- }
-
- unsafe {
- std::ptr::copy_nonoverlapping(
- errors.as_ptr(),
- ptr.cast(),
- errors.len()
- );
- *pperrors = ptr.cast();
- }
- }
+ allocate_and_write_errors(pperrors, &errors)?;Also applies to: 70-85
🤖 Prompt for AI Agents
In opc_da/src/opc_async_io.rs around lines 29 to 44 and also lines 70 to 85, the
code for allocating and writing error arrays is duplicated. Extract this
repeated logic into a new helper function named allocate_and_write_errors that
takes pperrors and errors as parameters, performs the allocation and copying,
and returns a Result. Then replace the duplicated code in these methods by calls
to this helper function to improve maintainability and reduce duplication.
| pub trait OPCAsyncIO { | ||
| fn read(&self, connection: u32, source: tagOPCDATASOURCE, server_handles: Vec<u32>) -> Result<(u32, Vec<HRESULT>)>; | ||
|
|
||
| fn write(&self, connection: u32, server_handles: Vec<u32>, values: Vec<windows::Win32::System::Variant::VARIANT>) -> Result<(u32, Vec<HRESULT>)>; | ||
|
|
||
| fn refresh(&self, connection: u32, source: tagOPCDATASOURCE) -> Result<u32>; | ||
|
|
||
| fn cancel(&self, transaction_id: u32) -> Result<()>; | ||
| } | ||
|
|
||
| pub trait OPCAsyncIO2: OPCAsyncIO { | ||
| fn read(&self, count: u32, server_handles: Vec<u32>, transaction_id: u32) -> Result<(u32, Vec<HRESULT>)>; | ||
|
|
||
| fn write(&self, count: u32, server_handles: Vec<u32>, values: Vec<windows::Win32::System::Variant::VARIANT>, transaction_id: u32) -> Result<(u32, Vec<HRESULT>)>; | ||
|
|
||
| fn refresh2(&self, source: tagOPCDATASOURCE, transaction_id: u32) -> Result<u32>; | ||
|
|
||
| fn cancel2(&self, cancel_id: u32) -> Result<()>; | ||
|
|
||
| fn set_enable(&self, enable: bool) -> Result<()>; | ||
|
|
||
| fn get_enable(&self) -> Result<bool>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential naming conflict in trait hierarchy.
The OPCAsyncIO and OPCAsyncIO2 traits both define methods named read, write, refresh, and cancel with different signatures. Since OPCAsyncIO2 extends OPCAsyncIO, this creates ambiguity and requires fully qualified syntax when calling these methods.
Consider renaming the methods in one of the traits to avoid confusion:
pub trait OPCAsyncIO {
- fn read(&self, connection: u32, source: tagOPCDATASOURCE, server_handles: Vec<u32>) -> Result<(u32, Vec<HRESULT>)>;
+ fn read_async(&self, connection: u32, source: tagOPCDATASOURCE, server_handles: Vec<u32>) -> Result<(u32, Vec<HRESULT>)>;
- fn write(&self, connection: u32, server_handles: Vec<u32>, values: Vec<windows::Win32::System::Variant::VARIANT>) -> Result<(u32, Vec<HRESULT>)>;
+ fn write_async(&self, connection: u32, server_handles: Vec<u32>, values: Vec<windows::Win32::System::Variant::VARIANT>) -> Result<(u32, Vec<HRESULT>)>;
- fn refresh(&self, connection: u32, source: tagOPCDATASOURCE) -> Result<u32>;
+ fn refresh_async(&self, connection: u32, source: tagOPCDATASOURCE) -> Result<u32>;
- fn cancel(&self, transaction_id: u32) -> Result<()>;
+ fn cancel_async(&self, transaction_id: u32) -> Result<()>;
}🤖 Prompt for AI Agents
In opc_da/src/opc_async_io.rs between lines 306 and 328, the traits OPCAsyncIO
and OPCAsyncIO2 both define methods named read, write, refresh, and cancel but
with different parameters, causing naming conflicts and ambiguity. To fix this,
rename the methods in OPCAsyncIO2 to distinct names that do not overlap with
OPCAsyncIO, such as read2, write2, refresh2 (already done), and cancel2, or use
more descriptive names to clearly differentiate them and avoid the need for
fully qualified syntax when calling these methods.
| if !ppitemproperties.is_null() { | ||
| let properties_array = CallerAllocatedArray::from_slice(&properties)?; | ||
| unsafe { *ppitemproperties = properties_array.as_ptr() }; | ||
| } |
There was a problem hiding this comment.
Critical: Potential use-after-free with CallerAllocatedArray.
The CallerAllocatedArray is created but goes out of scope after as_ptr() is called, potentially leading to use-after-free.
The CallerAllocatedArray should transfer ownership. Check if it has an into_raw() method or similar:
- let properties_array = CallerAllocatedArray::from_slice(&properties)?;
- unsafe { *ppitemproperties = properties_array.as_ptr() };
+ let properties_array = CallerAllocatedArray::from_slice(&properties)?;
+ unsafe { *ppitemproperties = properties_array.into_raw() };🤖 Prompt for AI Agents
In opc_da/src/opc_browse.rs around lines 38 to 41, the CallerAllocatedArray is
created and its pointer assigned, but the array goes out of scope immediately
causing a potential use-after-free. To fix this, transfer ownership of the
CallerAllocatedArray by using its into_raw() method or equivalent instead of
as_ptr(), ensuring the allocated memory remains valid after the function
returns.
| access_path: unsafe { | ||
| CallerAllocatedWString::from_pwstr(item.szAccessPath) | ||
| .to_string() | ||
| .unwrap_or_default() | ||
| }, | ||
| item_id: unsafe { | ||
| CallerAllocatedWString::from_pwstr(item.szItemID) | ||
| .to_string() | ||
| .unwrap_or_default() | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent string conversion methods between AddItems and ValidateItems.
AddItems uses from_pwstr while ValidateItems uses from_pcwstr with explicit null checks. This inconsistency could lead to maintenance issues.
Standardize the approach across both methods. Since ValidateItems has proper null checks, apply the same pattern to AddItems:
access_path: unsafe {
- CallerAllocatedWString::from_pwstr(item.szAccessPath)
- .to_string()
- .unwrap_or_default()
+ if item.szAccessPath.is_null() {
+ String::new()
+ } else {
+ CallerAllocatedWString::from_pcwstr(PCWSTR(item.szAccessPath.0))
+ .to_string()
+ .unwrap_or_default()
+ }
},
item_id: unsafe {
- CallerAllocatedWString::from_pwstr(item.szItemID)
- .to_string()
- .unwrap_or_default()
+ if item.szItemID.is_null() {
+ String::new()
+ } else {
+ CallerAllocatedWString::from_pcwstr(PCWSTR(item.szItemID.0))
+ .to_string()
+ .unwrap_or_default()
+ }
},Also applies to: 74-89
🤖 Prompt for AI Agents
In opc_da/src/opc_item.rs around lines 23 to 32 and also lines 74 to 89, the
string conversion methods are inconsistent between AddItems and ValidateItems;
AddItems uses from_pwstr without null checks, while ValidateItems uses
from_pcwstr with explicit null checks. To fix this, standardize by replacing
from_pwstr with from_pcwstr in AddItems and add explicit null checks before
conversion, mirroring the pattern used in ValidateItems to ensure consistent and
safe string handling.
| // Allocate and write results | ||
| let results_ptr = CalleeAllocatedPtrArray::from_slice(&results)?; | ||
| unsafe { *ppaddresults = results_ptr.as_ptr() }; | ||
|
|
||
| // Allocate and write errors | ||
| let errors_ptr = CalleeAllocatedPtrArray::from_slice(&errors)?; | ||
| unsafe { *pperrors = errors_ptr.as_ptr() }; | ||
|
|
There was a problem hiding this comment.
Critical: Memory leak due to incorrect CalleeAllocatedPtrArray usage.
The CalleeAllocatedPtrArray is created but the ownership is not properly transferred. The as_ptr() method returns a raw pointer, but the CalleeAllocatedPtrArray will be dropped at the end of the scope, leaving dangling pointers.
Use into_raw() instead of as_ptr():
- let results_ptr = CalleeAllocatedPtrArray::from_slice(&results)?;
- unsafe { *ppaddresults = results_ptr.as_ptr() };
+ let results_ptr = CalleeAllocatedPtrArray::from_slice(&results)?;
+ unsafe { *ppaddresults = results_ptr.into_raw() };
- let errors_ptr = CalleeAllocatedPtrArray::from_slice(&errors)?;
- unsafe { *pperrors = errors_ptr.as_ptr() };
+ let errors_ptr = CalleeAllocatedPtrArray::from_slice(&errors)?;
+ unsafe { *pperrors = errors_ptr.into_raw() };🤖 Prompt for AI Agents
In opc_da/src/opc_item.rs around lines 49 to 56, the code uses
CalleeAllocatedPtrArray::from_slice to create arrays and assigns raw pointers
using as_ptr(), but this causes a memory leak because the
CalleeAllocatedPtrArray is dropped and the pointers become dangling. To fix
this, replace as_ptr() with into_raw() to transfer ownership of the allocated
memory properly and prevent it from being freed prematurely.
| if !pperrors.is_null() { | ||
| let size = std::mem::size_of::<HRESULT>() * errors.len(); | ||
| let ptr = unsafe { windows::Win32::System::Com::CoTaskMemAlloc(size) }; | ||
| if ptr.is_null() { | ||
| return Err(windows::core::Error::from_win32()); | ||
| } | ||
|
|
||
| unsafe { | ||
| std::ptr::copy_nonoverlapping( | ||
| errors.as_ptr(), | ||
| ptr.cast(), | ||
| errors.len() | ||
| ); | ||
| *pperrors = ptr.cast(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract repeated error allocation logic.
The error allocation pattern is duplicated four times. Consider extracting it into a helper function or using the write_caller_allocated_array! macro.
Extract the error allocation logic into a helper function:
fn write_errors(pperrors: *mut *mut HRESULT, errors: &[HRESULT]) -> Result<()> {
write_caller_allocated_array!(pperrors, errors)?;
Ok(())
}Then replace all occurrences with:
- // Allocate and write errors
- if !pperrors.is_null() {
- let size = std::mem::size_of::<HRESULT>() * errors.len();
- let ptr = unsafe { windows::Win32::System::Com::CoTaskMemAlloc(size) };
- if ptr.is_null() {
- return Err(windows::core::Error::from_win32());
- }
-
- unsafe {
- std::ptr::copy_nonoverlapping(
- errors.as_ptr(),
- ptr.cast(),
- errors.len()
- );
- *pperrors = ptr.cast();
- }
- }
+ // Allocate and write errors
+ write_errors(pperrors, &errors)?;Also applies to: 81-96, 177-192, 214-229
🤖 Prompt for AI Agents
In opc_da/src/opc_sync_io.rs around lines 44 to 59, the error allocation logic
is duplicated multiple times (also at lines 81-96, 177-192, and 214-229). To fix
this, extract the repeated code into a helper function named write_errors that
takes pperrors and errors as parameters and uses the
write_caller_allocated_array! macro to handle the allocation and copying. Then
replace all the duplicated blocks with calls to this new helper function to
reduce code repetition and improve maintainability.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
opc_da/src/opc_item.rs (2)
23-32: Inconsistent string conversion methods between AddItems and ValidateItems.
49-56: Critical: Type mismatch errors preventing compilation.The code has two issues:
- Lines 51, 55: Using
as_ptr()will cause memory leaks (as noted in past review)- The
as_ptr()method returns*mut *mut Tbut the assignment expects*mut TApply this fix to resolve both the memory leak and type mismatch:
- let results_ptr = CalleeAllocatedPtrArray::from_slice(&results)?; - unsafe { *ppaddresults = results_ptr.as_ptr() }; + unsafe { *ppaddresults = CalleeAllocatedPtrArray::from_slice(&results)?.into_raw() }; - let errors_ptr = CalleeAllocatedPtrArray::from_slice(&errors)?; - unsafe { *pperrors = errors_ptr.as_ptr() }; + unsafe { *pperrors = CalleeAllocatedPtrArray::from_slice(&errors)?.into_raw() };opc_da/src/opc_browse.rs (1)
38-41: Critical: Potential use-after-free with CallerAllocatedArray.opc_classic_utils/src/memory/wstring.rs (1)
328-346: Fix buffer overflow in memory allocation.
🧹 Nitpick comments (4)
opc_comn/src/server/traits/opc_shutdown.rs (1)
1-3: Add documentation for the trait and method.Consider adding documentation to explain the purpose of this trait and when
shutdown_requestshould be called.+/// Trait for handling OPC server shutdown requests. pub trait OpcShutdown { + /// Handles a shutdown request with the specified reason. + /// + /// # Arguments + /// * `reason` - The reason for the shutdown request + /// + /// # Returns + /// * `Ok(())` if the shutdown request was handled successfully + /// * `Err(...)` if an error occurred fn shutdown_request(&self, reason: String) -> windows_core::Result<()>; }opc_classic_utils/examples/convenience_functions.rs (1)
78-128: API change looks good, but consider documenting the lossy conversion behavior.The consistent change from
to_string()toto_string_lossy()aligns with the API updates. However, users of these examples should understand when lossy conversion might occur (e.g., when encountering invalid UTF-16 sequences).Consider adding a comment explaining the lossy conversion:
println!("\n2. Converting wide strings back to Rust strings:"); // Convert back to String unsafe { + // Note: to_string_lossy() replaces invalid UTF-16 sequences with replacement characters let converted1 = wstring1.to_string_lossy().unwrap();opc_comn/src/server/opc_server_base.rs (1)
48-56: Consider extracting common PCWSTR to String conversion logic.Both
SetClientNameandShutdownRequestuse identical code for convertingPCWSTRtoString. Consider extracting this into a helper function.+fn pcwstr_to_string(pcwstr: &windows_core::PCWSTR) -> windows_core::Result<String> { + unsafe { + opc_classic_utils::CallerAllocatedWString::from_pcwstr(*pcwstr) + .to_string_lossy() + .ok_or(windows::Win32::Foundation::E_POINTER) + } +} + impl<T: super::traits::opc_common::OpcCommon + super::traits::opc_shutdown::OpcShutdown> opc_comn_bindings::IOPCCommon_Impl for OpcServerBase_Impl<T> { // ... fn SetClientName(&self, szname: &windows_core::PCWSTR) -> windows_core::Result<()> { - self.0.set_client_name(unsafe { - opc_classic_utils::CallerAllocatedWString::from_pcwstr(*szname) - .to_string_lossy() - .ok_or(windows::Win32::Foundation::E_POINTER) - }?) + self.0.set_client_name(pcwstr_to_string(szname)?) } } impl<T: super::traits::opc_common::OpcCommon + super::traits::opc_shutdown::OpcShutdown> opc_comn_bindings::IOPCShutdown_Impl for OpcServerBase_Impl<T> { fn ShutdownRequest(&self, szreason: &windows_core::PCWSTR) -> windows_core::Result<()> { - let reason = unsafe { - opc_classic_utils::CallerAllocatedWString::from_pcwstr(*szreason) - .to_string_lossy() - .ok_or(windows::Win32::Foundation::E_POINTER) - }?; - - self.0.shutdown_request(reason) + self.0.shutdown_request(pcwstr_to_string(szreason)?) } }opc_da/src/opc_group.rs (1)
87-104: Fix unused variable warnings.The destructured variables are not used after assignment.
Apply this fix to suppress warnings:
- let ( - revised_update_rate, - new_active, - new_time_bias, - new_percent_deadband, - new_locale_id, - new_client_group, - ) = self.0.set_state( + let ( + revised_update_rate, + _new_active, + _new_time_bias, + _new_percent_deadband, + _new_locale_id, + _new_client_group, + ) = self.0.set_state(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
opc_classic_utils/examples/convenience_functions.rs(3 hunks)opc_classic_utils/src/memory/tests.rs(4 hunks)opc_classic_utils/src/memory/wstring.rs(7 hunks)opc_comn/src/client/mod.rs(1 hunks)opc_comn/src/lib.rs(1 hunks)opc_comn/src/server/mod.rs(1 hunks)opc_comn/src/server/opc_server_base.rs(1 hunks)opc_comn/src/server/traits/mod.rs(1 hunks)opc_comn/src/server/traits/opc_common.rs(1 hunks)opc_comn/src/server/traits/opc_shutdown.rs(1 hunks)opc_da/src/opc_browse.rs(1 hunks)opc_da/src/opc_group.rs(1 hunks)opc_da/src/opc_item.rs(1 hunks)opc_da/src/opc_server.rs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- opc_comn/src/client/mod.rs
- opc_comn/src/server/mod.rs
- opc_comn/src/server/traits/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- opc_comn/src/lib.rs
- opc_da/src/opc_server.rs
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
Learnt from: Ronbb
PR: Ronbb/rust_opc#10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In `opc_da/src/client/iterator.rs`, when initializing the `cache` array in `GroupIterator`, use `cache: [const { None }; 16]` instead of `cache: [None; 16]` because Flipper requires this change.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In `opc_da/src/traits/group.rs`, the `GroupTrait` trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the `opc_da/src/com/server.rs` file (Rust), the enum variant `OPC_NS_HIERARCHIAL` is correctly spelled as per the OPC specification, even though it appears misspelled.
opc_comn/src/server/traits/opc_shutdown.rs (1)
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
opc_classic_utils/examples/convenience_functions.rs (3)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file opc_da/src/client/tests.rs, do not add retry logic in test functions as per the user's preference.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/item_properties.rs:58-62
Timestamp: 2024-12-11T01:41:07.039Z
Learning: When count is of type u32, it's safe to set array lengths without additional bounds checking.
opc_classic_utils/src/memory/tests.rs (2)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file opc_da/src/client/tests.rs, do not add retry logic in test functions as per the user's preference.
opc_comn/src/server/traits/opc_common.rs (4)
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/async_io2.rs:94-95
Timestamp: 2024-12-11T01:29:33.264Z
Learning: In AsyncIo2Trait within opc_da/src/client/traits/async_io2.rs, when directly returning a Result from a method like Refresh2, it's acceptable to return the result as is without additional wrapping.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/item_mgt.rs:191-194
Timestamp: 2024-12-10T03:00:26.689Z
Learning: In opc_da/src/client/traits/item_mgt.rs, the CreateEnumerator method returns a Result, so there is no need to handle None values.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
opc_comn/src/server/opc_server_base.rs (4)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
opc_da/src/opc_group.rs (7)
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/unified/group.rs:28-39
Timestamp: 2024-12-10T13:51:14.061Z
Learning: In the add_items method in opc_da/src/client/unified/group.rs, the OPC DA protocol guarantees that the results and errors arrays returned will always be of the same length, so additional validation for array length mismatch is not necessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/async_io2.rs:94-95
Timestamp: 2024-12-11T01:29:33.264Z
Learning: In AsyncIo2Trait within opc_da/src/client/traits/async_io2.rs, when directly returning a Result from a method like Refresh2, it's acceptable to return the result as is without additional wrapping.
opc_da/src/opc_browse.rs (8)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/item_mgt.rs:191-194
Timestamp: 2024-12-10T03:00:26.689Z
Learning: In opc_da/src/client/traits/item_mgt.rs, the CreateEnumerator method returns a Result, so there is no need to handle None values.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/unified/group.rs:28-39
Timestamp: 2024-12-10T13:51:14.061Z
Learning: In the add_items method in opc_da/src/client/unified/group.rs, the OPC DA protocol guarantees that the results and errors arrays returned will always be of the same length, so additional validation for array length mismatch is not necessary.
Learnt from: Ronbb
PR: #10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In opc_da/src/client/iterator.rs, when initializing the cache array in GroupIterator, use cache: [const { None }; 16] instead of cache: [None; 16] because Flipper requires this change.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
opc_da/src/opc_item.rs (10)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/item_mgt.rs:191-194
Timestamp: 2024-12-10T03:00:26.689Z
Learning: In opc_da/src/client/traits/item_mgt.rs, the CreateEnumerator method returns a Result, so there is no need to handle None values.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/unified/group.rs:28-39
Timestamp: 2024-12-10T13:51:14.061Z
Learning: In the add_items method in opc_da/src/client/unified/group.rs, the OPC DA protocol guarantees that the results and errors arrays returned will always be of the same length, so additional validation for array length mismatch is not necessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In opc_da/src/client/iterator.rs, when initializing the cache array in GroupIterator, use cache: [const { None }; 16] instead of cache: [None; 16] because Flipper requires this change.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/async_io2.rs:94-95
Timestamp: 2024-12-11T01:29:33.264Z
Learning: In AsyncIo2Trait within opc_da/src/client/traits/async_io2.rs, when directly returning a Result from a method like Refresh2, it's acceptable to return the result as is without additional wrapping.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
opc_classic_utils/src/memory/wstring.rs (4)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like copy_to_pointer are unnecessary and should be omitted.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
🧬 Code Graph Analysis (3)
opc_comn/src/server/traits/opc_common.rs (1)
opc_da/src/opc_server.rs (1)
get_error_string(109-109)
opc_comn/src/server/opc_server_base.rs (3)
opc_da/src/opc_server.rs (2)
GetErrorString(60-62)get_error_string(109-109)opc_comn/src/server/traits/opc_common.rs (1)
get_error_string(5-5)opc_classic_utils/src/memory/wstring.rs (2)
from_pcwstr(35-39)from_pcwstr(218-222)
opc_da/src/opc_group.rs (1)
opc_classic_utils/src/memory/wstring.rs (2)
from_pcwstr(35-39)from_pcwstr(218-222)
🪛 GitHub Actions: Rust
opc_da/src/opc_group.rs
[warning] 89-89: Unused variable: new_active. Consider prefixing with underscore if intentional.
[warning] 90-90: Unused variable: new_time_bias. Consider prefixing with underscore if intentional.
[warning] 91-91: Unused variable: new_percent_deadband. Consider prefixing with underscore if intentional.
[warning] 92-92: Unused variable: new_locale_id. Consider prefixing with underscore if intentional.
[warning] 93-93: Unused variable: new_client_group. Consider prefixing with underscore if intentional.
opc_da/src/opc_item.rs
[error] 51-51: Rust compiler error E0308: mismatched types. Expected *mut tagOPCITEMRESULT, found *mut *mut tagOPCITEMRESULT.
[error] 55-55: Rust compiler error E0308: mismatched types. Expected *mut HRESULT, found *mut *mut HRESULT.
[error] 109-109: Rust compiler error E0308: mismatched types. Expected *mut tagOPCITEMRESULT, found tuple (*mut *mut tagOPCITEMRESULT, usize).
[error] 113-113: Rust compiler error E0308: mismatched types. Expected *mut HRESULT, found tuple (*mut *mut HRESULT, usize).
[error] 130-130: Rust compiler error E0308: mismatched types. Expected *mut HRESULT, found tuple (*mut *mut HRESULT, usize).
[error] 150-150: Rust compiler error E0308: mismatched types. Expected *mut HRESULT, found tuple (*mut *mut HRESULT, usize).
[error] 171-171: Rust compiler error E0308: mismatched types. Expected *mut HRESULT, found tuple (*mut *mut HRESULT, usize).
[error] 193-193: Rust compiler error E0308: mismatched types. Expected *mut HRESULT, found tuple (*mut *mut HRESULT, usize).
⏰ 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). (1)
- GitHub Check: analyzing
🔇 Additional comments (6)
opc_classic_utils/src/memory/tests.rs (1)
175-185: Good test coverage for the updated API.The tests correctly verify both
CallerAllocatedWString::to_string_lossy()andCalleeAllocatedWString::to_string(). The differentiation between the two APIs is maintained in the tests.opc_comn/src/server/opc_server_base.rs (1)
36-42: Good null pointer handling in SetClientName.The implementation correctly checks for null strings and returns
E_POINTERwhen appropriate. The use ofto_string_lossy()is consistent with the API changes.opc_classic_utils/src/memory/wstring.rs (2)
41-46: LGTM! Consistent PWSTR support added.The new
from_pwstrandas_pwstrmethods follow the established patterns for PCWSTR handling.Also applies to: 139-143, 291-295
145-151: Good clarification on memory ownership.The updated comments clearly document that the callee is responsible for freeing the memory.
opc_da/src/opc_group.rs (2)
11-39: Well-implemented GetState using new memory macros.Good use of the
write_caller_allocated_ptr!andalloc_callee_wstring!macros with proper null checking.
108-127: Good string handling with proper error checking.Both
SetNameandCloneGroupproperly handle string conversion with null checks and appropriate error returns.
| pub trait OpcCommon { | ||
| fn set_locale_id(&self, dwlcid: u32) -> windows_core::Result<()>; | ||
| fn get_locale_id(&self) -> windows_core::Result<u32>; | ||
| fn query_available_locale_ids(&self) -> windows_core::Result<Vec<u32>>; | ||
| fn get_error_string(&self, dwerror: windows_core::HRESULT) -> windows_core::Result<String>; | ||
| fn set_client_name(&self, szname: String) -> windows_core::Result<()>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and consider locale parameter for error strings.
The trait lacks documentation. Also, the get_error_string method differs from the similar method in opc_da/src/opc_server.rs which includes a locale parameter.
+/// Trait for OPC Common functionality including locale and client management.
pub trait OpcCommon {
+ /// Sets the locale identifier for the server.
fn set_locale_id(&self, dwlcid: u32) -> windows_core::Result<()>;
+
+ /// Gets the current locale identifier.
fn get_locale_id(&self) -> windows_core::Result<u32>;
+
+ /// Returns a list of available locale identifiers.
fn query_available_locale_ids(&self) -> windows_core::Result<Vec<u32>>;
+
+ /// Gets a descriptive error string for the given error code.
+ /// Consider whether locale should be a parameter here for consistency with opc_da.
fn get_error_string(&self, dwerror: windows_core::HRESULT) -> windows_core::Result<String>;
+
+ /// Sets the client name.
fn set_client_name(&self, szname: String) -> windows_core::Result<()>;
}📝 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.
| pub trait OpcCommon { | |
| fn set_locale_id(&self, dwlcid: u32) -> windows_core::Result<()>; | |
| fn get_locale_id(&self) -> windows_core::Result<u32>; | |
| fn query_available_locale_ids(&self) -> windows_core::Result<Vec<u32>>; | |
| fn get_error_string(&self, dwerror: windows_core::HRESULT) -> windows_core::Result<String>; | |
| fn set_client_name(&self, szname: String) -> windows_core::Result<()>; | |
| } | |
| /// Trait for OPC Common functionality including locale and client management. | |
| pub trait OpcCommon { | |
| /// Sets the locale identifier for the server. | |
| fn set_locale_id(&self, dwlcid: u32) -> windows_core::Result<()>; | |
| /// Gets the current locale identifier. | |
| fn get_locale_id(&self) -> windows_core::Result<u32>; | |
| /// Returns a list of available locale identifiers. | |
| fn query_available_locale_ids(&self) -> windows_core::Result<Vec<u32>>; | |
| /// Gets a descriptive error string for the given error code. | |
| /// Consider whether locale should be a parameter here for consistency with opc_da. | |
| fn get_error_string(&self, dwerror: windows_core::HRESULT) -> windows_core::Result<String>; | |
| /// Sets the client name. | |
| fn set_client_name(&self, szname: String) -> windows_core::Result<()>; | |
| } |
🤖 Prompt for AI Agents
In opc_comn/src/server/traits/opc_common.rs lines 1 to 7, add Rust doc comments
to the OpcCommon trait and its methods to explain their purpose and usage.
Modify the get_error_string method signature to include a locale parameter
similar to the one in opc_da/src/opc_server.rs, allowing error strings to be
localized. Ensure the documentation reflects this change and clarifies the role
of the locale parameter.
| let elements_array = CallerAllocatedArray::from_slice(&elements)?; | ||
| unsafe { *ppbrowseelements = elements_array.as_ptr() }; | ||
| } |
There was a problem hiding this comment.
Critical: Potential use-after-free with CallerAllocatedArray.
The CallerAllocatedArray goes out of scope after as_ptr() is called, potentially leading to use-after-free.
Apply the same fix pattern:
- let elements_array = CallerAllocatedArray::from_slice(&elements)?;
- unsafe { *ppbrowseelements = elements_array.as_ptr() };
+ unsafe { *ppbrowseelements = CallerAllocatedArray::from_slice(&elements)?.into_raw() };🤖 Prompt for AI Agents
In opc_da/src/opc_browse.rs around lines 103 to 105, the CallerAllocatedArray is
created and its pointer assigned to ppbrowseelements, but the array goes out of
scope immediately causing a use-after-free. To fix this, store the
CallerAllocatedArray in a variable that lives long enough (e.g., a struct field
or a variable with extended scope) so that the memory remains valid while
ppbrowseelements is used, preventing premature deallocation.
opc_da/src/opc_item.rs
Outdated
| let results_ptr = CalleeAllocatedPtrArray::from_slice(&results)?; | ||
| unsafe { *ppvalidationresults = results_ptr.into_raw() }; | ||
|
|
||
| // Allocate and write errors | ||
| let errors_ptr = CalleeAllocatedPtrArray::from_slice(&errors)?; | ||
| unsafe { *pperrors = errors_ptr.into_raw() }; | ||
|
|
There was a problem hiding this comment.
Critical: Type mismatch - into_raw() returns tuple instead of pointer.
The into_raw() method returns a tuple (*mut *mut T, usize) but the assignment expects just the pointer.
Apply this fix:
- let results_ptr = CalleeAllocatedPtrArray::from_slice(&results)?;
- unsafe { *ppvalidationresults = results_ptr.into_raw() };
+ unsafe {
+ let (ptr, _) = CalleeAllocatedPtrArray::from_slice(&results)?.into_raw();
+ *ppvalidationresults = ptr;
+ }
- let errors_ptr = CalleeAllocatedPtrArray::from_slice(&errors)?;
- unsafe { *pperrors = errors_ptr.into_raw() };
+ unsafe {
+ let (ptr, _) = CalleeAllocatedPtrArray::from_slice(&errors)?.into_raw();
+ *pperrors = ptr;
+ }📝 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.
| let results_ptr = CalleeAllocatedPtrArray::from_slice(&results)?; | |
| unsafe { *ppvalidationresults = results_ptr.into_raw() }; | |
| // Allocate and write errors | |
| let errors_ptr = CalleeAllocatedPtrArray::from_slice(&errors)?; | |
| unsafe { *pperrors = errors_ptr.into_raw() }; | |
| unsafe { | |
| let (ptr, _) = CalleeAllocatedPtrArray::from_slice(&results)?.into_raw(); | |
| *ppvalidationresults = ptr; | |
| } | |
| // Allocate and write errors | |
| unsafe { | |
| let (ptr, _) = CalleeAllocatedPtrArray::from_slice(&errors)?.into_raw(); | |
| *pperrors = ptr; | |
| } |
🧰 Tools
🪛 GitHub Actions: Rust
[error] 109-109: Rust compiler error E0308: mismatched types. Expected *mut tagOPCITEMRESULT, found tuple (*mut *mut tagOPCITEMRESULT, usize).
[error] 113-113: Rust compiler error E0308: mismatched types. Expected *mut HRESULT, found tuple (*mut *mut HRESULT, usize).
🤖 Prompt for AI Agents
In opc_da/src/opc_item.rs around lines 108 to 114, the code assigns the result
of into_raw() directly to pointers, but into_raw() returns a tuple of a pointer
and a length. To fix this, destructure the tuple returned by into_raw() and
assign only the pointer part to *ppvalidationresults and *pperrors respectively,
ignoring the length value.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
opc_classic_utils/src/memory/wstring.rs (1)
328-346: Fix buffer overflow in memory allocation.The memory allocation on line 337 doesn't account for the null terminator, causing a buffer overflow when copying the entire
wide_string(which includes the null terminator).Apply this fix:
- let ptr = unsafe { CoTaskMemAlloc(len * std::mem::size_of::<u16>()) }; + let ptr = unsafe { CoTaskMemAlloc((len + 1) * std::mem::size_of::<u16>()) };The allocation should include space for the null terminator since
wide_string.len()includes it butlendoesn't.opc_da/src/opc_item.rs (4)
23-32: Add null checks for string conversions.
AddItemsusesfrom_pwstrwithout null checks whileValidateItemsproperly checks for null pointers. This inconsistency could lead to undefined behavior if null pointers are passed.Apply the same null-checking pattern from
ValidateItems:access_path: unsafe { - CallerAllocatedWString::from_pwstr(item.szAccessPath) - .to_string_lossy() - .unwrap_or_default() + if item.szAccessPath.is_null() { + String::new() + } else { + CallerAllocatedWString::from_pcwstr(PCWSTR(item.szAccessPath.0)) + .to_string_lossy() + .unwrap_or_default() + } }, item_id: unsafe { - CallerAllocatedWString::from_pwstr(item.szItemID) - .to_string_lossy() - .unwrap_or_default() + if item.szItemID.is_null() { + String::new() + } else { + CallerAllocatedWString::from_pcwstr(PCWSTR(item.szItemID.0)) + .to_string_lossy() + .unwrap_or_default() + } },
49-56: Critical: Memory leak due to incorrect CalleeAllocatedPtrArray usage.The
CalleeAllocatedPtrArrayis created but the ownership is not properly transferred. Theas_ptr()method returns a raw pointer, but theCalleeAllocatedPtrArraywill be dropped at the end of the scope, leaving dangling pointers.Use
into_raw()instead ofas_ptr():- let results_ptr = CalleeAllocatedPtrArray::from_slice(&results)?; - unsafe { *ppaddresults = results_ptr.as_ptr() }; + let results_ptr = CalleeAllocatedPtrArray::from_slice(&results)?; + unsafe { *ppaddresults = results_ptr.into_raw() }; - let errors_ptr = CalleeAllocatedPtrArray::from_slice(&errors)?; - unsafe { *pperrors = errors_ptr.as_ptr() }; + let errors_ptr = CalleeAllocatedPtrArray::from_slice(&errors)?; + unsafe { *pperrors = errors_ptr.into_raw() };
108-114: Critical: Type mismatch -into_raw()returns tuple instead of pointer.The
into_raw()method returns a tuple(*mut *mut T, usize)but the assignment expects just the pointer.Apply this fix:
- let results_ptr = CalleeAllocatedPtrArray::from_slice(&results)?; - unsafe { *ppvalidationresults = results_ptr.into_raw() }; + unsafe { + let (ptr, _) = CalleeAllocatedPtrArray::from_slice(&results)?.into_raw(); + *ppvalidationresults = ptr; + } - let errors_ptr = CalleeAllocatedPtrArray::from_slice(&errors)?; - unsafe { *pperrors = errors_ptr.into_raw() }; + unsafe { + let (ptr, _) = CalleeAllocatedPtrArray::from_slice(&errors)?.into_raw(); + *pperrors = ptr; + }
129-131: Critical: Multiple type mismatches withinto_raw()returning tuples.The same tuple type mismatch occurs in multiple functions.
Apply this pattern to all affected functions:
# In RemoveItems (lines 129-131): - let errors_ptr = CalleeAllocatedPtrArray::from_slice(&errors)?; - unsafe { *pperrors = errors_ptr.into_raw() }; + unsafe { + let (ptr, _) = CalleeAllocatedPtrArray::from_slice(&errors)?.into_raw(); + *pperrors = ptr; + } # Apply the same pattern to: # - SetActiveState (lines 149-151) # - SetClientHandles (lines 170-172) # - SetDatatypes (lines 192-194)opc_da/src/opc_browse.rs (2)
38-41: Critical: Potential use-after-free with CallerAllocatedArray.The
CallerAllocatedArrayis created but goes out of scope afteras_ptr()is called, potentially leading to use-after-free.The
CallerAllocatedArrayshould transfer ownership. Check if it has aninto_raw()method or similar:- let properties_array = CallerAllocatedArray::from_slice(&properties)?; - unsafe { *ppitemproperties = properties_array.as_ptr() }; + let properties_array = CallerAllocatedArray::from_slice(&properties)?; + unsafe { *ppitemproperties = properties_array.into_raw() };
103-105: Critical: Potential use-after-free with CallerAllocatedArray.The
CallerAllocatedArraygoes out of scope afteras_ptr()is called, potentially leading to use-after-free.Apply the same fix pattern:
- let elements_array = CallerAllocatedArray::from_slice(&elements)?; - unsafe { *ppbrowseelements = elements_array.as_ptr() }; + unsafe { *ppbrowseelements = CallerAllocatedArray::from_slice(&elements)?.into_raw() };
🧹 Nitpick comments (1)
opc_classic_utils/src/memory/wstring.rs (1)
83-83: Inconsistent safety documentation.The method is marked
unsafebut uses the safe naming conventionto_string_lossy(not prefixed withunsafe_). Consider renaming tounsafe_to_string_lossyfor clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
opc_classic_utils/examples/convenience_functions.rs(3 hunks)opc_classic_utils/src/memory/tests.rs(4 hunks)opc_classic_utils/src/memory/wstring.rs(7 hunks)opc_comn/src/client/mod.rs(1 hunks)opc_comn/src/lib.rs(1 hunks)opc_comn/src/server/mod.rs(1 hunks)opc_comn/src/server/opc_server_base.rs(1 hunks)opc_comn/src/server/traits/mod.rs(1 hunks)opc_comn/src/server/traits/opc_common.rs(1 hunks)opc_comn/src/server/traits/opc_shutdown.rs(1 hunks)opc_da/src/opc_browse.rs(1 hunks)opc_da/src/opc_group.rs(1 hunks)opc_da/src/opc_item.rs(1 hunks)opc_da/src/opc_server.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- opc_comn/src/server/traits/opc_shutdown.rs
- opc_comn/src/client/mod.rs
- opc_comn/src/lib.rs
- opc_comn/src/server/traits/mod.rs
- opc_comn/src/server/traits/opc_common.rs
- opc_classic_utils/examples/convenience_functions.rs
- opc_comn/src/server/mod.rs
- opc_classic_utils/src/memory/tests.rs
- opc_da/src/opc_server.rs
- opc_comn/src/server/opc_server_base.rs
- opc_da/src/opc_group.rs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
Learnt from: Ronbb
PR: Ronbb/rust_opc#10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In `opc_da/src/client/iterator.rs`, when initializing the `cache` array in `GroupIterator`, use `cache: [const { None }; 16]` instead of `cache: [None; 16]` because Flipper requires this change.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In `opc_da/src/traits/group.rs`, the `GroupTrait` trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the `opc_da/src/com/server.rs` file (Rust), the enum variant `OPC_NS_HIERARCHIAL` is correctly spelled as per the OPC specification, even though it appears misspelled.
opc_da/src/opc_browse.rs (8)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/item_mgt.rs:191-194
Timestamp: 2024-12-10T03:00:26.689Z
Learning: In opc_da/src/client/traits/item_mgt.rs, the CreateEnumerator method returns a Result, so there is no need to handle None values.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/unified/group.rs:28-39
Timestamp: 2024-12-10T13:51:14.061Z
Learning: In the add_items method in opc_da/src/client/unified/group.rs, the OPC DA protocol guarantees that the results and errors arrays returned will always be of the same length, so additional validation for array length mismatch is not necessary.
Learnt from: Ronbb
PR: #10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In opc_da/src/client/iterator.rs, when initializing the cache array in GroupIterator, use cache: [const { None }; 16] instead of cache: [None; 16] because Flipper requires this change.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
opc_classic_utils/src/memory/wstring.rs (4)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like copy_to_pointer are unnecessary and should be omitted.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
opc_da/src/opc_item.rs (12)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/item_mgt.rs:191-194
Timestamp: 2024-12-10T03:00:26.689Z
Learning: In opc_da/src/client/traits/item_mgt.rs, the CreateEnumerator method returns a Result, so there is no need to handle None values.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/unified/group.rs:28-39
Timestamp: 2024-12-10T13:51:14.061Z
Learning: In the add_items method in opc_da/src/client/unified/group.rs, the OPC DA protocol guarantees that the results and errors arrays returned will always be of the same length, so additional validation for array length mismatch is not necessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In opc_da/src/client/iterator.rs, when initializing the cache array in GroupIterator, use cache: [const { None }; 16] instead of cache: [None; 16] because Flipper requires this change.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like copy_to_pointer are unnecessary and should be omitted.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/async_io2.rs:94-95
Timestamp: 2024-12-11T01:29:33.264Z
Learning: In AsyncIo2Trait within opc_da/src/client/traits/async_io2.rs, when directly returning a Result from a method like Refresh2, it's acceptable to return the result as is without additional wrapping.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file opc_da/src/client/tests.rs, do not add retry logic in test functions as per the user's preference.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
⏰ 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). (1)
- GitHub Check: analyzing
🔇 Additional comments (8)
opc_classic_utils/src/memory/wstring.rs (6)
41-46: Good addition for PWSTR support.The new
from_pwstrmethod provides symmetry withfrom_pcwstrand enables direct construction from mutable wide string pointers.
139-142: LGTM! Consistent API additions.Adding
as_pwstr()methods to both string types provides a consistent API for Windows interop.
148-149: Good documentation update for Drop behavior.The comment clearly explains that the pointer is kept intact for the callee, which aligns with the caller-allocated memory ownership model.
224-228: Useful convenience method.The
from_stringmethod provides a convenient way to create CalleeAllocatedWString from owned strings.
291-294: Consistent API addition.Adding
as_pwstr()to CalleeAllocatedWString maintains API consistency between both string types.
348-404: Well-designed macro with comprehensive documentation.The
alloc_callee_wstring!macro provides a clean API for creating COM-allocated wide strings. The documentation thoroughly covers usage, memory management responsibilities, and typical use cases.opc_da/src/opc_item.rs (1)
203-247: Well-designed trait interface.The
ItemDefinitionstruct andOPCItemtrait provide a clean abstraction for OPC item management with proper error handling throughResulttypes.opc_da/src/opc_browse.rs (1)
111-129: Clean trait design.The
OPCBrowsetrait provides a well-structured interface for OPC browsing functionality with appropriate error handling.
| fn browse( | ||
| &self, | ||
| item_id: String, | ||
| max_elements_returned: u32, | ||
| browse_filter: tagOPCBROWSEFILTER, | ||
| element_name_filter: String, | ||
| vendor_filter: String, | ||
| return_all_properties: bool, | ||
| return_property_values: bool, | ||
| property_ids: Vec<u32>, | ||
| ) -> Result<(bool, u32, Vec<tagOPCBROWSEELEMENT>)>; |
Check warning
Code scanning / clippy
this function has too many arguments (9/7) Warning
| fn on_data_change( | ||
| &self, | ||
| transaction_id: u32, | ||
| group: u32, | ||
| quality: HRESULT, | ||
| error: HRESULT, | ||
| client_items: Vec<u32>, | ||
| values: Vec<windows::Win32::System::Variant::VARIANT>, | ||
| qualities: Vec<u16>, | ||
| timestamps: Vec<windows::Win32::Foundation::FILETIME>, | ||
| errors: Vec<HRESULT>, | ||
| ) -> Result<()>; |
Check warning
Code scanning / clippy
this function has too many arguments (10/7) Warning
| fn on_read_complete( | ||
| &self, | ||
| transaction_id: u32, | ||
| group: u32, | ||
| quality: HRESULT, | ||
| error: HRESULT, | ||
| client_items: Vec<u32>, | ||
| values: Vec<windows::Win32::System::Variant::VARIANT>, | ||
| qualities: Vec<u16>, | ||
| timestamps: Vec<windows::Win32::Foundation::FILETIME>, | ||
| errors: Vec<HRESULT>, | ||
| ) -> Result<()>; |
Check warning
Code scanning / clippy
this function has too many arguments (10/7) Warning
| impl<T: traits::OPCGroup + traits::OPCGroupStateMgt2> IOPCGroupStateMgt_Impl for OPCGroup_Impl<T> { | ||
| fn GetState( | ||
| &self, | ||
| pupdateRate: *mut u32, |
Check warning
Code scanning / clippy
variable pupdateRate should have a snake case name Warning
| fn set_state2( | ||
| &self, | ||
| requested_update_rate: Option<u32>, | ||
| active: Option<bool>, | ||
| time_bias: Option<i32>, | ||
| percent_deadband: Option<f32>, | ||
| locale_id: Option<u32>, | ||
| client_group: Option<u32>, | ||
| keep_alive: Option<u32>, | ||
| ) -> Result<(u32, bool, i32, f32, u32, u32, u32)>; |
Check warning
Code scanning / clippy
this function has too many arguments (8/7) Warning
| dwlcid: u32, | ||
| phservergroup: *mut u32, | ||
| previsedupdaterate: *mut u32, | ||
| riid: *const GUID, |
Check warning
Code scanning / clippy
unused variable: riid Warning
| fn add_group( | ||
| &self, | ||
| name: String, | ||
| active: bool, | ||
| requested_update_rate: u32, | ||
| client_group: u32, | ||
| time_bias: Option<i32>, | ||
| percent_deadband: Option<f32>, | ||
| locale_id: u32, | ||
| ) -> Result<(u32, u32, IUnknown)>; |
Check warning
Code scanning / clippy
this function has too many arguments (8/7) Warning
| @@ -0,0 +1,230 @@ | |||
| use opc_classic_utils::*; | |||
Check warning
Code scanning / clippy
unused import: opc_classic_utils::* Warning
| ) -> Result<( | ||
| Vec<windows::Win32::System::Variant::VARIANT>, | ||
| Vec<u16>, | ||
| Vec<windows::Win32::Foundation::FILETIME>, | ||
| Vec<HRESULT>, | ||
| )>; |
Check warning
Code scanning / clippy
very complex type used. Consider factoring parts into type definitions Warning
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
opc_da/src/opc_item.rs (1)
23-32: String conversion inconsistency between AddItems and ValidateItems.This issue was previously identified.
AddItemsusesfrom_pwstrwithout null checks whileValidateItemsusesfrom_pcwstrwith explicit null checks. This inconsistency could lead to crashes if null pointers are passed.Apply the same null-checking pattern used in
ValidateItems:access_path: unsafe { - CallerAllocatedWString::from_pwstr(item.szAccessPath) - .to_string_lossy() - .unwrap_or_default() + if item.szAccessPath.is_null() { + String::new() + } else { + CallerAllocatedWString::from_pcwstr(PCWSTR(item.szAccessPath.0)) + .to_string_lossy() + .unwrap_or_default() + } }, item_id: unsafe { - CallerAllocatedWString::from_pwstr(item.szItemID) - .to_string_lossy() - .unwrap_or_default() + if item.szItemID.is_null() { + String::new() + } else { + CallerAllocatedWString::from_pcwstr(PCWSTR(item.szItemID.0)) + .to_string_lossy() + .unwrap_or_default() + } },
🧹 Nitpick comments (1)
opc_da/src/opc_server.rs (1)
10-58: Consider extracting repeated string conversion pattern.The string conversion pattern from
PCWSTRis repeated in bothAddGroupandGetGroupByName. Consider extracting it into a helper function to reduce duplication and improve maintainability.fn pcwstr_to_string(pcwstr: PCWSTR) -> Result<String> { unsafe { CallerAllocatedWString::from_pcwstr(pcwstr) .to_string_lossy() .ok_or(windows::Win32::Foundation::E_POINTER) } }Then use it as:
- let name = unsafe { - CallerAllocatedWString::from_pcwstr(*szname) - .to_string_lossy() - .ok_or(windows::Win32::Foundation::E_POINTER) - }?; + let name = pcwstr_to_string(*szname)?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
opc_classic_utils/src/memory/array.rs(2 hunks)opc_da/src/opc_item.rs(1 hunks)opc_da/src/opc_server.rs(1 hunks)opc_da/src/opc_sync_io.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- opc_da/src/opc_sync_io.rs
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
Learnt from: Ronbb
PR: Ronbb/rust_opc#10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In `opc_da/src/client/iterator.rs`, when initializing the `cache` array in `GroupIterator`, use `cache: [const { None }; 16]` instead of `cache: [None; 16]` because Flipper requires this change.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the `opc_da/src/com/server.rs` file (Rust), the enum variant `OPC_NS_HIERARCHIAL` is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In `opc_da/src/traits/group.rs`, the `GroupTrait` trait contains only method declarations without implementations.
opc_classic_utils/src/memory/array.rs (6)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/item_properties.rs:58-62
Timestamp: 2024-12-11T01:41:07.039Z
Learning: When count is of type u32, it's safe to set array lengths without additional bounds checking.
Learnt from: Ronbb
PR: #10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In opc_da/src/client/iterator.rs, when initializing the cache array in GroupIterator, use cache: [const { None }; 16] instead of cache: [None; 16] because Flipper requires this change.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like copy_to_pointer are unnecessary and should be omitted.
opc_da/src/opc_item.rs (12)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/item_mgt.rs:191-194
Timestamp: 2024-12-10T03:00:26.689Z
Learning: In opc_da/src/client/traits/item_mgt.rs, the CreateEnumerator method returns a Result, so there is no need to handle None values.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/unified/group.rs:28-39
Timestamp: 2024-12-10T13:51:14.061Z
Learning: In the add_items method in opc_da/src/client/unified/group.rs, the OPC DA protocol guarantees that the results and errors arrays returned will always be of the same length, so additional validation for array length mismatch is not necessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In opc_da/src/client/iterator.rs, when initializing the cache array in GroupIterator, use cache: [const { None }; 16] instead of cache: [None; 16] because Flipper requires this change.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like copy_to_pointer are unnecessary and should be omitted.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/async_io2.rs:94-95
Timestamp: 2024-12-11T01:29:33.264Z
Learning: In AsyncIo2Trait within opc_da/src/client/traits/async_io2.rs, when directly returning a Result from a method like Refresh2, it's acceptable to return the result as is without additional wrapping.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file opc_da/src/client/tests.rs, do not add retry logic in test functions as per the user's preference.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
opc_da/src/opc_server.rs (7)
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/unified/group.rs:28-39
Timestamp: 2024-12-10T13:51:14.061Z
Learning: In the add_items method in opc_da/src/client/unified/group.rs, the OPC DA protocol guarantees that the results and errors arrays returned will always be of the same length, so additional validation for array length mismatch is not necessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:8-35
Timestamp: 2024-12-11T01:49:14.219Z
Learning: In the file opc_da/src/client/tests.rs, do not add retry logic in test functions as per the user's preference.
🧬 Code Graph Analysis (2)
opc_classic_utils/src/memory/array.rs (2)
opc_classic_utils/src/memory/ptr_array.rs (13)
from_slice(255-274)as_ptr(80-82)as_ptr(305-307)allocate(37-59)allocate(280-302)len(96-98)len(321-323)is_null(106-108)is_null(331-333)new(24-26)new(243-245)as_mut_slice(128-134)as_mut_slice(353-359)opc_classic_utils/src/memory/ptr.rs (7)
as_ptr(58-60)as_ptr(175-177)allocate(35-41)is_null(72-74)is_null(189-191)new(22-24)new(148-150)
opc_da/src/opc_server.rs (3)
opc_classic_utils/src/memory/wstring.rs (2)
from_pcwstr(35-39)from_pcwstr(220-224)opc_comn/src/server/opc_server_base.rs (1)
GetErrorString(29-34)opc_classic_utils/src/memory/ptr.rs (2)
from_value(46-55)from_value(162-172)
⏰ 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). (1)
- GitHub Check: analyzing
🔇 Additional comments (3)
opc_classic_utils/src/memory/array.rs (1)
162-169: Potential use-after-drop safety issue in Drop implementation.While the comment indicates that the pointer should be preserved for the callee, only setting
self.len = 0doesn't prevent the struct from being used after drop. The pointer remains valid in the struct, which could lead to accidental use-after-drop.Consider adding debug assertions or explicit documentation warning about this behavior:
impl<T> Drop for CallerAllocatedArray<T> { fn drop(&mut self) { // Do NOT free the memory - the callee is responsible for this // Just clear the length to prevent use-after-free // Keep the pointer intact for the callee to use self.len = 0; // Note: We don't clear self.ptr because the callee needs it + + // SAFETY: After drop, this struct should not be used. The pointer + // is intentionally preserved for the callee to access and free. + #[cfg(debug_assertions)] + { + // In debug builds, we could track dropped instances + } } }⛔ Skipped due to learnings
Learnt from: Ronbb PR: Ronbb/rust_opc#9 File: opc_da/src/client/memory.rs:96-106 Timestamp: 2024-12-09T15:29:03.494Z Learning: In Rust, calling `std::mem::forget` on a value that implements `Copy`, such as `PWSTR`, has no effect because the value is copied and the original is still dropped. Therefore, using `std::mem::forget` in such cases does not prevent the value from being dropped or prevent a potential double-free.Learnt from: Ronbb PR: Ronbb/rust_opc#8 File: opc_da/src/com/utils.rs:61-87 Timestamp: 2024-11-28T15:39:26.903Z Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.Learnt from: Ronbb PR: Ronbb/rust_opc#9 File: opc_da/src/client/v3/mod.rs:67-78 Timestamp: 2024-12-10T14:06:24.207Z Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.Learnt from: Ronbb PR: Ronbb/rust_opc#10 File: opc_da/src/client/iterator.rs:114-114 Timestamp: 2024-12-13T18:47:42.521Z Learning: In `opc_da/src/client/iterator.rs`, when initializing the `cache` array in `GroupIterator`, use `cache: [const { None }; 16]` instead of `cache: [None; 16]` because Flipper requires this change.Learnt from: Ronbb PR: Ronbb/rust_opc#9 File: opc_da/src/client/v3/mod.rs:9-15 Timestamp: 2024-12-10T14:03:29.606Z Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.opc_da/src/opc_server.rs (1)
60-91: Well-implemented COM interface methods.The implementation correctly:
- Uses
alloc_callee_wstring!for returning COM-allocated strings- Properly transfers ownership with
into_raw()for allocated pointers- Handles COM interface wrapping appropriately
opc_da/src/opc_item.rs (1)
195-239: Well-designed trait interface for OPC item management.The
ItemDefinitionstruct andOPCItemtrait provide a clean abstraction over the COM interface, properly separating concerns and making the API more Rust-idiomatic.
| /// Copies data directly to a caller-allocated array | ||
| /// | ||
| /// This macro simplifies copying data directly to a caller-allocated array | ||
| /// without allocating new memory. It's used when the caller has already | ||
| /// allocated the array and we just need to copy data into it. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `$dst` - A raw pointer (`*mut T`) to the destination array | ||
| /// * `$src` - A slice (`&[T]`) containing the source data | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// Returns `Result<(), windows::core::Error>`: | ||
| /// * `Ok(())` - Data was successfully copied | ||
| /// * `Err(E_INVALIDARG)` - The destination pointer is null | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The caller must ensure that: | ||
| /// * `$dst` is a valid pointer to caller-allocated memory | ||
| /// * The destination array has sufficient space for the source data | ||
| /// * The array elements are `Copy` types | ||
| /// | ||
| /// # Example | ||
| /// | ||
| /// ```rust | ||
| /// use opc_classic_utils::copy_to_caller_array; | ||
| /// | ||
| /// let mut array: [u32; 5] = [0; 5]; | ||
| /// let data = vec![1u32, 2u32, 3u32, 4u32, 5u32]; | ||
| /// | ||
| /// // Copy data directly to the caller-allocated array | ||
| /// copy_to_caller_array!(array.as_mut_ptr(), &data)?; | ||
| /// assert_eq!(array, [1, 2, 3, 4, 5]); | ||
| /// # Ok::<(), windows::core::Error>(()) | ||
| /// ``` | ||
| /// | ||
| /// # Typical Use Cases | ||
| /// | ||
| /// * Copying data to caller-allocated output parameters | ||
| /// * OPC Classic API array output parameters | ||
| /// * Direct memory copying without allocation | ||
| #[macro_export] | ||
| macro_rules! copy_to_caller_array { | ||
| ($dst:expr, $src:expr) => { | ||
| unsafe { | ||
| let src = $src; | ||
| let mut dst_ptr = opc_classic_utils::CallerAllocatedArray::allocate(src.len())?; | ||
| if dst_ptr.is_null() { | ||
| return Err(windows::core::Error::new( | ||
| windows::Win32::Foundation::E_INVALIDARG, | ||
| "Destination pointer is null", | ||
| )); | ||
| } | ||
| *$dst = dst_ptr.as_ptr(); | ||
| if let Some(dst_slice) = dst_ptr.as_mut_slice() { | ||
| dst_slice.copy_from_slice(src); | ||
| Ok(()) | ||
| } else { | ||
| Err(windows::core::Error::new( | ||
| windows::Win32::Foundation::E_INVALIDARG, | ||
| "Failed to get mutable slice", | ||
| )) | ||
| } | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Critical: Misleading macro name and implementation for copy_to_caller_array!
The macro name and documentation claim it "copies data directly to a caller-allocated array without allocating new memory", but the implementation actually allocates new memory using CallerAllocatedArray::allocate() at line 461.
This is fundamentally incorrect and misleading. The macro should either:
- Be renamed to reflect that it allocates memory (e.g.,
alloc_and_copy_to_caller_array!) - Be rewritten to actually copy to existing memory without allocation
The current implementation could lead to serious bugs where developers expect to copy to existing memory but instead allocate new memory, potentially causing memory leaks.
If the intent is to allocate memory, consider this corrected implementation with a more accurate name:
-/// Copies data directly to a caller-allocated array
+/// Allocates memory and copies data to a caller-allocated array pointer
///
-/// This macro simplifies copying data directly to a caller-allocated array
-/// without allocating new memory. It's used when the caller has already
-/// allocated the array and we just need to copy data into it.
+/// This macro allocates new memory using CoTaskMemAlloc and copies data
+/// into it, then writes the pointer to the caller-provided location.
///
#[macro_export]
-macro_rules! copy_to_caller_array {
+macro_rules! alloc_and_copy_to_caller_array {
($dst:expr, $src:expr) => {
unsafe {
let src = $src;
- let mut dst_ptr = opc_classic_utils::CallerAllocatedArray::allocate(src.len())?;
- if dst_ptr.is_null() {
- return Err(windows::core::Error::new(
- windows::Win32::Foundation::E_INVALIDARG,
- "Destination pointer is null",
- ));
- }
- *$dst = dst_ptr.as_ptr();
- if let Some(dst_slice) = dst_ptr.as_mut_slice() {
+ let mut array = opc_classic_utils::CallerAllocatedArray::allocate(src.len())?;
+ if let Some(dst_slice) = array.as_mut_slice() {
dst_slice.copy_from_slice(src);
- Ok(())
- } else {
- Err(windows::core::Error::new(
- windows::Win32::Foundation::E_INVALIDARG,
- "Failed to get mutable slice",
- ))
}
+ *$dst = array.into_raw().0;
+ Ok(())
}
};
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In opc_classic_utils/src/memory/array.rs between lines 413 and 480, the macro
copy_to_caller_array! is misleading because it claims to copy data to a
caller-allocated array without allocation, but it actually allocates new memory
using CallerAllocatedArray::allocate(). To fix this, either rename the macro to
reflect that it allocates memory (e.g., alloc_and_copy_to_caller_array!) or
rewrite the macro to copy data directly into the provided destination pointer
without any allocation. Ensure the implementation matches the macro name and
documentation to avoid confusion and potential memory issues.
opc_da/src/opc_item.rs
Outdated
| copy_to_caller_array!(ppaddresults, &results); | ||
|
|
||
| // Allocate and write errors | ||
| copy_to_caller_array!(pperrors, &errors)?; |
There was a problem hiding this comment.
Using misleadingly named copy_to_caller_array! macro.
The copy_to_caller_array! macro name suggests it copies to existing memory, but it actually allocates new memory. While this behavior might be correct for COM output parameters (where the callee allocates), the macro name is misleading and could cause confusion.
Consider using a more accurately named macro or the direct approach:
- copy_to_caller_array!(ppaddresults, &results);
+ // Allocate and write results
+ let array = CallerAllocatedArray::from_slice(&results)?;
+ *ppaddresults = array.into_raw().0;Or wait for the macro to be renamed to something like alloc_and_copy_to_caller_array! to better reflect its behavior.
Also applies to: 106-109, 125-125, 144-144, 164-164, 185-185
🤖 Prompt for AI Agents
In opc_da/src/opc_item.rs around lines 50 to 53, the macro copy_to_caller_array!
is misleadingly named because it allocates new memory instead of copying to
existing memory. To fix this, rename the macro to something more descriptive
like alloc_and_copy_to_caller_array! to clearly indicate that it performs
allocation and copying. Alternatively, replace the macro usage with explicit
allocation and copy code for clarity. Apply the same renaming or replacement to
the other occurrences at lines 106-109, 125, 144, 164, and 185.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
opc_classic_utils/src/memory/array.rs (1)
409-476: Critical: Misleading macro name and implementation forcopy_to_caller_array!The macro name and documentation claim it "copies data directly to a caller-allocated array without allocating new memory", but the implementation actually allocates new memory using
CallerAllocatedArray::allocate()at line 457.This is fundamentally incorrect and misleading. The macro should either:
- Be renamed to reflect that it allocates memory (e.g.,
alloc_and_copy_to_caller_array!)- Be rewritten to actually copy to existing memory without allocation
The current implementation could lead to serious bugs where developers expect to copy to existing memory but instead allocate new memory, potentially causing memory leaks.
opc_da/src/opc_item.rs (2)
23-32: Inconsistent string conversion methods between AddItems and ValidateItems.
AddItemsusesfrom_pwstrwhileValidateItemsusesfrom_pcwstrwith explicit null checks. This inconsistency could lead to maintenance issues.Standardize the approach across both methods. Since
ValidateItemshas proper null checks, apply the same pattern toAddItems:access_path: unsafe { - CallerAllocatedWString::from_pwstr(item.szAccessPath) - .to_string_lossy() - .unwrap_or_default() + if item.szAccessPath.is_null() { + String::new() + } else { + CallerAllocatedWString::from_pcwstr(PCWSTR(item.szAccessPath.0)) + .to_string_lossy() + .unwrap_or_default() + } }, item_id: unsafe { - CallerAllocatedWString::from_pwstr(item.szItemID) - .to_string_lossy() - .unwrap_or_default() + if item.szItemID.is_null() { + String::new() + } else { + CallerAllocatedWString::from_pcwstr(PCWSTR(item.szItemID.0)) + .to_string_lossy() + .unwrap_or_default() + } },Also applies to: 71-88
50-53: Using misleadingly namedcopy_to_caller_array!macro.The
copy_to_caller_array!macro name suggests it copies to existing memory, but it actually allocates new memory. While this behavior might be correct for COM output parameters (where the callee allocates), the macro name is misleading and could cause confusion.Consider using a more accurately named macro or the direct approach:
- copy_to_caller_array!(ppaddresults, &results)?; + // Allocate and write results + let array = CallerAllocatedArray::from_slice(&results)?; + *ppaddresults = array.into_raw().0;Or wait for the macro to be renamed to something like
alloc_and_copy_to_caller_array!to better reflect its behavior.Also applies to: 106-109, 125-125, 144-144, 164-164, 185-185
opc_classic_utils/src/memory/wstring.rs (1)
326-344: Fix buffer overflow in memory allocation.The memory allocation on line 335 doesn't account for the null terminator, causing a buffer overflow when copying the entire
wide_string(which includes the null terminator).Apply this fix:
- let ptr = unsafe { CoTaskMemAlloc(len * std::mem::size_of::<u16>()) }; + let ptr = unsafe { CoTaskMemAlloc((len + 1) * std::mem::size_of::<u16>()) };The allocation should include space for the null terminator since
wide_string.len()includes it butlendoesn't.
🧹 Nitpick comments (1)
opc_classic_utils/src/memory/wstring.rs (1)
346-402: Well-designed macro with dependency on FromStr fix.The
alloc_callee_wstring!macro provides excellent abstraction for wide string allocation with comprehensive documentation. However, it depends on theFromStrimplementation which has a buffer overflow issue that needs to be fixed first.Once the buffer overflow in
FromStris fixed, this macro will provide a safe and ergonomic way to allocate wide strings for COM interop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
opc_classic_utils/src/memory/array.rs(4 hunks)opc_classic_utils/src/memory/ptr.rs(5 hunks)opc_classic_utils/src/memory/ptr_array.rs(4 hunks)opc_classic_utils/src/memory/wstring.rs(10 hunks)opc_da/src/opc_data_callback.rs(1 hunks)opc_da/src/opc_item.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- opc_da/src/opc_data_callback.rs
- opc_classic_utils/src/memory/ptr_array.rs
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like `copy_to_pointer` are unnecessary and should be omitted.
Learnt from: Ronbb
PR: Ronbb/rust_opc#10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In `opc_da/src/client/iterator.rs`, when initializing the `cache` array in `GroupIterator`, use `cache: [const { None }; 16]` instead of `cache: [None; 16]` because Flipper requires this change.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the `Drop` trait via `IUnknown`, automatically releasing resources when they go out of scope, so manual implementation of `Drop` for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: Ronbb/rust_opc#9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In `windows-rs`, COM interfaces implement `IUnknown`, which automatically calls `Release` when dropped. Therefore, it's unnecessary to manually implement the `Drop` trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the `opc_da/src/com/server.rs` file (Rust), the enum variant `OPC_NS_HIERARCHIAL` is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: Ronbb/rust_opc#8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In `opc_da/src/traits/group.rs`, the `GroupTrait` trait contains only method declarations without implementations.
opc_classic_utils/src/memory/ptr.rs (4)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like copy_to_pointer are unnecessary and should be omitted.
opc_da/src/opc_item.rs (11)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/item_mgt.rs:191-194
Timestamp: 2024-12-10T03:00:26.689Z
Learning: In opc_da/src/client/traits/item_mgt.rs, the CreateEnumerator method returns a Result, so there is no need to handle None values.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/unified/group.rs:28-39
Timestamp: 2024-12-10T13:51:14.061Z
Learning: In the add_items method in opc_da/src/client/unified/group.rs, the OPC DA protocol guarantees that the results and errors arrays returned will always be of the same length, so additional validation for array length mismatch is not necessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/server.rs:714-716
Timestamp: 2024-11-29T12:06:30.463Z
Learning: In the opc_da/src/com/server.rs file (Rust), the enum variant OPC_NS_HIERARCHIAL is correctly spelled as per the OPC specification, even though it appears misspelled.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #8
File: opc_da/src/traits/group.rs:1-122
Timestamp: 2024-12-02T14:43:35.670Z
Learning: In opc_da/src/traits/group.rs, the GroupTrait trait contains only method declarations without implementations.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In opc_da/src/client/iterator.rs, when initializing the cache array in GroupIterator, use cache: [const { None }; 16] instead of cache: [None; 16] because Flipper requires this change.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like copy_to_pointer are unnecessary and should be omitted.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/async_io2.rs:94-95
Timestamp: 2024-12-11T01:29:33.264Z
Learning: In AsyncIo2Trait within opc_da/src/client/traits/async_io2.rs, when directly returning a Result from a method like Refresh2, it's acceptable to return the result as is without additional wrapping.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/tests.rs:0-0
Timestamp: 2024-12-11T01:25:38.423Z
Learning: In Rust OPC DA client tests, resources like groups do not require explicit cleanup as they will be released after the OPC DA server disconnects.
opc_classic_utils/src/memory/array.rs (6)
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like copy_to_pointer are unnecessary and should be omitted.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/traits/item_properties.rs:58-62
Timestamp: 2024-12-11T01:41:07.039Z
Learning: When count is of type u32, it's safe to set array lengths without additional bounds checking.
Learnt from: Ronbb
PR: #10
File: opc_da/src/client/iterator.rs:114-114
Timestamp: 2024-12-13T18:47:42.521Z
Learning: In opc_da/src/client/iterator.rs, when initializing the cache array in GroupIterator, use cache: [const { None }; 16] instead of cache: [None; 16] because Flipper requires this change.
opc_classic_utils/src/memory/wstring.rs (4)
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/memory.rs:96-106
Timestamp: 2024-12-09T15:29:03.494Z
Learning: In Rust, calling std::mem::forget on a value that implements Copy, such as PWSTR, has no effect because the value is copied and the original is still dropped. Therefore, using std::mem::forget in such cases does not prevent the value from being dropped or prevent a potential double-free.
Learnt from: Ronbb
PR: #8
File: opc_da/src/com/utils.rs:61-87
Timestamp: 2024-11-28T15:39:26.903Z
Learning: In this codebase, all data copying functions should use COM. Non-COM functions like copy_to_pointer are unnecessary and should be omitted.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:67-78
Timestamp: 2024-12-10T14:06:24.207Z
Learning: In windows-rs, COM interfaces implement the Drop trait via IUnknown, automatically releasing resources when they go out of scope, so manual implementation of Drop for structs holding COM interfaces is unnecessary.
Learnt from: Ronbb
PR: #9
File: opc_da/src/client/v3/mod.rs:9-15
Timestamp: 2024-12-10T14:03:29.606Z
Learning: In windows-rs, COM interfaces implement IUnknown, which automatically calls Release when dropped. Therefore, it's unnecessary to manually implement the Drop trait to release COM interfaces in structs that hold COM interfaces.
🧬 Code Graph Analysis (3)
opc_classic_utils/src/memory/ptr.rs (4)
opc_classic_utils/src/memory/array.rs (4)
into_raw(86-90)into_raw(251-255)from_raw(27-29)from_raw(239-241)opc_classic_utils/src/memory/ptr_array.rs (4)
into_raw(87-91)into_raw(308-312)from_raw(29-31)from_raw(246-248)opc_classic_utils/src/memory/wstring.rs (4)
into_raw(117-119)into_raw(275-277)from_raw(30-32)from_raw(213-215)opc_classic_utils/examples/memory_management_comparison.rs (4)
ptr(36-36)ptr(57-57)ptr(83-83)ptr(90-90)
opc_classic_utils/src/memory/array.rs (2)
opc_classic_utils/src/memory/ptr.rs (9)
into_raw(65-67)into_raw(180-182)as_ptr(58-60)as_ptr(173-175)allocate(35-41)is_null(70-72)is_null(185-187)new(22-24)new(146-148)opc_classic_utils/src/memory/ptr_array.rs (15)
into_raw(87-91)into_raw(308-312)from_slice(251-270)as_ptr(80-82)as_ptr(301-303)allocate(37-59)allocate(276-298)len(94-96)len(315-317)is_null(104-106)is_null(325-327)new(24-26)new(239-241)as_mut_slice(126-132)as_mut_slice(347-353)
opc_classic_utils/src/memory/wstring.rs (3)
opc_classic_utils/src/memory/ptr.rs (6)
into_raw(65-67)into_raw(180-182)drop(102-106)drop(217-224)new(22-24)new(146-148)opc_classic_utils/src/memory/array.rs (8)
into_raw(86-90)into_raw(251-255)drop(161-167)drop(326-334)new(22-24)new(232-234)len(93-95)len(258-260)opc_classic_utils/src/memory/ptr_array.rs (8)
into_raw(87-91)into_raw(308-312)drop(165-168)drop(386-402)new(24-26)new(239-241)len(94-96)len(315-317)
⏰ 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). (2)
- GitHub Check: analyzing
- GitHub Check: build
🔇 Additional comments (9)
opc_classic_utils/src/memory/ptr.rs (3)
65-67: Good simplification of ownership transfer semantics.The change from
mut selftoselfand removal of pointer nullification is correct. Sinceinto_rawconsumes the wrapper, there's no risk of use-after-free, and the ownership transfer is clear at the type level.Also applies to: 180-182
104-105: Clear documentation of memory ownership semantics.The added comments explicitly document why the pointer is kept intact, which helps prevent confusion about the memory management strategy for caller-allocated pointers.
235-288: Well-designed macro for safe pointer writes.The macro provides excellent safety abstractions:
- Proper null pointer validation with E_INVALIDARG
- Clear ownership semantics using CallerAllocatedPtr
- Comprehensive documentation with safety requirements
- Practical example demonstrating usage
The design correctly handles the COM pattern where the caller allocates but doesn't free the memory.
opc_classic_utils/src/memory/array.rs (3)
86-89: Consistent ownership transfer pattern.The simplified
into_rawmethods correctly implement move semantics by takingselfby value, matching the pattern established across all memory wrapper types in the crate.Also applies to: 251-254
163-167: Appropriate Drop implementation for caller-allocated arrays.Setting length to 0 while preserving the pointer is a good defensive practice that prevents use of an invalid length while still allowing the callee to access the allocated memory.
346-407: Well-designed array allocation macro.The
write_caller_allocated_array!macro correctly:
- Documents that it allocates memory
- Uses established patterns from other macros
- Provides clear memory management documentation
- Includes practical examples
opc_classic_utils/src/memory/wstring.rs (3)
41-46: Good addition of PWSTR support methods.The new
from_pwstrandas_pwstrmethods provide consistent mutable pointer support, complementing the existing PCWSTR methods with the same safety model.Also applies to: 131-134, 289-292
140-141: Consistent Drop implementation documentation.The added comments align with the pattern established across all memory wrapper types, clearly documenting the ownership semantics.
183-189: Useful conversion trait implementation.The
TryFrom<PCWSTR>implementation provides ergonomic conversions while maintaining API consistency by returning a Result, even though it always succeeds.
Summary by CodeRabbit
New Features
opc_comncrate with client and server modules, including base server functionality and traits for OPC Common and Shutdown interfaces.opc_classic_utils.Documentation
Bug Fixes
Style
Chores
.gitignorefiles to exclude build artifacts.