-
Notifications
You must be signed in to change notification settings - Fork 24
test: turn sspi-ffi-attacker into tests runnable with miri #275
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
Conversation
* slice creation based on the null pointer; * stacked borrows violition.
@@ -22,8 +22,10 @@ pub struct SecPkgInfoW { | |||
|
|||
pub type PSecPkgInfoW = *mut SecPkgInfoW; | |||
|
|||
pub struct RawSecPkgInfoW(pub *mut SecPkgInfoW); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was fun to investigate and fix. In short, conversions chain like this one *mut SecPkgInfoW -> &mut SecPkgInfoW -> *mut SecPkgInfoW
can create UB 😮
When we create &mut SecPkgInfoW
, Rust creates a SharedReadWrite tag (it relates to stacked borrows and so on). And when we convert it to the raw pointer and try to deallocate, we have UB.
This is why I removed the conversion into &mut SecPkgInfoW
and refactored it to use only raw pointers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
sspi-ffi-attacher
sspi-ffi-attacker
@@ -22,8 +22,10 @@ pub struct SecPkgInfoW { | |||
|
|||
pub type PSecPkgInfoW = *mut SecPkgInfoW; | |||
|
|||
pub struct RawSecPkgInfoW(pub *mut SecPkgInfoW); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question/nitpick: Should we use the PSecPkgInfoW
type alias? Remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not allowed to implement the From
trait for any raw pointers, because all raw pointers are foreign types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant this:
pub struct RawSecPkgInfoW(pub *mut SecPkgInfoW); | |
pub struct RawSecPkgInfoW(pub PSecPkgInfoW); |
I am under the impression that we don’t really use this alias.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! LGTM!
Merging now, the remaining comment is not very important |
sspi-ffi-attacker
Hi,
In the scope of this PR, I've refactored the
sspi-fii-attacker
:sspi-ffi-attacker
.ffi/sspi/sec_handle/tests
.Now we can run those scenarios with Miri which will help us catch UBs and other memory errors.
Additional context: #261 (comment)