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 unsoundness in PdInfo::as_struct() #18

Closed
wants to merge 4 commits into from

Conversation

Sympatron
Copy link
Contributor

@Sympatron Sympatron commented Sep 28, 2024

Since PdInfo.scbk's lifetime is bound to PdInfo, osdp_pd_info_t may outlive it. Currently it would still hold a reference to PdInfo's scbk which would be unsound.
This fixes this by effectively cloning scbk so that it has a 'static lifetime.

Another option would be to change PdInfoBuilder::secure_channel_key to accept a &'static [u8;16] instead. This would get rid of the unsoundness too and would not require a heap allocation per PD. This would change the API and might make it slightly more difficult to use, but may be worth it.

What do you think?

Edit:
I just realized that (almost) the same goes for the name and cap fields. And I am pretty sure that cap needs to be either NULL or end with a (-1, 0, 0) sentinel so I changed that, too.

@Sympatron Sympatron force-pushed the unsound-pdinfo branch 2 times, most recently from 21e9c32 to e10b0a6 Compare September 28, 2024 16:40
@sidcha
Copy link
Member

sidcha commented Oct 1, 2024

Thanks for the PR and yes both issues are valid issues. Even with the fix, we are allocating and leaking memory in these cases because LibOSDP makes make a copy of all the members and doesn't care about osdp_pd_info_t after the setup call. TBH, I recognised this issue sometime back and didn't get to it due to lack of energy :D

Perhaps the C structures should only hold references to the Rust types that way they get dropped correctly. Ofc, this would mean significant change to different parts of this project :) Do you have any opinions on this matter?

@Sympatron
Copy link
Contributor Author

Sympatron commented Oct 2, 2024

You are right - LibOSDP does copy cap and scbk so they need to be dropped afterwards. But it does not copy name so that needs to stay. It will still be a leak if PeripheralDevice or ControlPanel are dropped, but that is still better than UB IMO.

To prevent the leak of scbk and cap I added drop_osdp_pd_info() which is called right after setup. This is probably not the most elegant solution. More elegant would be to write a repr(transparent) wrapper with an appropriate drop implementation.

What do you think?

I will rebase this PR after #17 is merged.

@sidcha
Copy link
Member

sidcha commented Oct 2, 2024

But it does not copy name

This is an oversight. The general rule is to not make any assumptions on the lifetime of user provided data. I will fix this the upcoming release in LibOSDP.

This is probably not the most elegant solution.

I haven't reviewed this PR yet but the idea of drop_osdp_pd_info() does sound bad.

@Sympatron Sympatron force-pushed the unsound-pdinfo branch 2 times, most recently from 363eac5 to 6250044 Compare October 2, 2024 10:18
@Sympatron
Copy link
Contributor Author

the idea of drop_osdp_pd_info() does sound bad

As I said, it was not pretty. I changed it to a wrapper type instead. That is cleaner IMO.
I'm not sure sure about it's name though.

@sidcha
Copy link
Member

sidcha commented Oct 11, 2024

Merged this PR locally after some minor fixups (GitHub did not allow me to push the modifications to this PR for some reason).

Thanks for the PR :)

@sidcha sidcha closed this Oct 11, 2024
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.

2 participants