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

move some unsafe code inside an unsafe{} boundary #483

Merged

Conversation

apoelstra
Copy link
Member

An internal function had a non-unsafe signature but could be called
with data that would cause it to exhibit UB. Move the unsafety inside
of the function so that the function signature now enforces soundness.

Fixes #481

@apoelstra
Copy link
Member Author

Not sure if this is as tight as you were hoping for @Kixunil

tcharding
tcharding previously approved these changes Aug 11, 2022
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW looks ok to me.

ACK 1242e71

@apoelstra
Copy link
Member Author

Interesting. I saw that clippy failure but ignored it since it had nothing to do with my code ... I guess it's a change in clippy and we need to fix it for CI to pass.

@tcharding
Copy link
Member

I don't think it was a change in clippy because we use clippy.toml to set the version. I think this was caused by the same "github ci doesn't re-run when master changes" thing we've been hitting a bunch lately. cough bors :)

sanket1729
sanket1729 previously approved these changes Aug 11, 2022
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 29ce1cc

Kixunil
Kixunil previously approved these changes Aug 11, 2022
Copy link
Collaborator

@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 29ce1cc

But maybe you want commits in the opposite order?

let noncedata_ptr = match noncedata {
Some(arr) => arr.as_c_ptr() as *const _,
None => ptr::null(),
};

This comment was marked as resolved.

@tcharding
Copy link
Member

tcharding commented Aug 11, 2022

Now I'm confused, I did a clippy PR #484 that includes additional fixes to this PR but this is green? And how is it green if the clippy fixes are done as second patch?

@apoelstra
Copy link
Member Author

CI only runs on the top commit -- and the top commit of this PR has the clippy fixes.

An internal function had a non-unsafe signature but could be called
with data that would cause it to exhibit UB. Move the unsafety inside
of the function so that the function signature now enforces soundness.

Fixes rust-bitcoin#481
@apoelstra apoelstra dismissed stale reviews from Kixunil and sanket1729 via 0f29348 August 12, 2022 16:03
@apoelstra apoelstra force-pushed the 2022-08--cleanup-noncedata-pointer branch from 29ce1cc to 0f29348 Compare August 12, 2022 16:03
@apoelstra
Copy link
Member Author

Merged #484 and rebased. Now there are no clippy fixes in this PR.

Copy link
Member

@elichai elichai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@apoelstra apoelstra merged commit b00b194 into rust-bitcoin:master Aug 13, 2022
@apoelstra apoelstra deleted the 2022-08--cleanup-noncedata-pointer branch August 13, 2022 14:28
@apoelstra
Copy link
Member Author

Merged -- the one remaining commit was identical to the one ACKed by Sanket and Kixunil, and while elichai did not specify a commit, there is really only one that was ever in this PR.

@tcharding
Copy link
Member

CI only runs on the top commit

Oh I'm a goose.

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.

Internal "unsoundness" in sign_ecdsa_with_noncedata_pointer
5 participants