Repository navigation
fix: memory leak in ua::Array::move_into_raw - #356
Conversation
26e21fe to
6df8cb0
Compare
6df8cb0 to
eee9b8d
Compare
sgoll
left a comment
There was a problem hiding this comment.
Thank you for your pull request 🎉
I have only a few minor remarks, see below. And do you think it would be possible to add a test case that reproduces the memory leak—so that it fails without the fix and does not fail anymore now?1
Footnotes
-
I'm not sure it can be done easily, because it would involve showing that a destructor is not run, which with Rust code would be relatively easy (writing a sentinel value in
impl Drop), but not so with the foreign types we are dealing with here. But if anything came to mind… 😇 ↩
cef1145 to
a1f5ea0
Compare
See #358, it's establishing the testing infrastructure to enable ASAN in CI, which actually found another leak. Also, I added a test that sits behind |
|
@cartercanedy Thank you! Just so you know, @uklotzde and I are currently attending EuroRust, so it'll probably take until next week for us to review your PRs. |
|
Please add a changelog entry. |
There was a problem hiding this comment.
🟡 Changes recommended
The leak probe does not reclaim the transferred raw allocation and will report a leak.
1 open issue
What changed in this PR
Fixes an ownership leak in ua::Array::move_into_raw and updates internal iterator-based array construction.
Changes:
- Cleans up existing destination arrays before replacement.
- Replaces internal
from_iterusage withcollect_ua_array. - Adds an ASAN leak probe and documentation updates.
| File | Summary |
|---|---|
src/ua/data_types/node_attributes/variable_attributes.rs |
Updates array construction. |
src/ua/data_types/delete_subscriptions_request.rs |
Updates array construction. |
src/ua/data_types/delete_monitored_items_request.rs |
Updates array construction. |
src/ua/data_types/content_filter_element.rs |
Updates array construction. |
src/ua/data_types/browse_next_request.rs |
Updates array construction. |
src/ua/data_types/application_description.rs |
Updates array construction. |
src/ua/array.rs |
Fixes ownership transfer and adds iterator support and leak testing. |
src/ua.rs |
Re-exports the internal iterator helper. |
src/server.rs |
Updates URL collection. |
src/lib.rs |
Imports the iterator helper. |
CHANGELOG.md |
Documents the memory leak fix. |
Cargo.toml |
Registers the ASAN configuration. |
Review effort: Lite
09b8c1a to
70314d0
Compare
|
@sgoll Ping. |
Co-authored-by: Sebastian Goll <1277035+sgoll@users.noreply.github.com>

Closes #355
Summary
move_into_rawnow callsmove_from_raw_partson the destination array, which properly cleans up the array that's going to be clobberedfrom_iterimplementation into an internal-only trait that enables cleaner collection of iterators intoua::Arrayand updates existing callsitesua::Arrayshould eventually become a public implementation of theFromIteratortrait, relying onIterator::size_hintto perform optimistic preallocations while remaining sound