Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory ownership issue in the init function #29

Merged
merged 2 commits into from
Jun 17, 2024

Conversation

diondokter
Copy link
Contributor

So I've been debugging this problem for many days now. Over at my nrf-modem I made this same patch.
It turns out that the params given to the init function need to live forever. Seems like the params aren't copied over, but are still referenced later.

No idea why nobody caught this earlier.
But this fixes it.

If anybody reviews and merges I can publish the new release.

Note though that I have some difficulties to compile this crate now. I think the bindgen version for nrfxlib-sys is quite old now.

@diondokter diondokter merged commit 3481077 into nrf-rs:develop Jun 17, 2024
1 check passed
@diondokter diondokter deleted the fix-memory-ownership branch June 17, 2024 14:43
@jonathanpallant
Copy link
Member

I think the init() function might be unsound - it needs a boolean guard to stop you calling it twice. Otherwise it will re-initialise a heap that might be in use, and over-write a configuration structure that the nrfxlib library might already be using.

@diondokter
Copy link
Contributor Author

I think the init() function might be unsound - it needs a boolean guard to stop you calling it twice. Otherwise it will re-initialise a heap that might be in use, and over-write a configuration structure that the nrfxlib library might already be using.

I guess you're right. Though calling it twice right now will error anyways. But it becomes unsound just before the error. This is an issue in my version as well.

But look, I'm not that interested in maintaining this crate. I'd rather put the effort in my own. I made this PR mostly for anyone using this because the bug was so nasty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants