Document unsafe code #524
Document unsafe code #524
Conversation
tcharding
force-pushed
the
11-22-document-unsafe
branch
from
a53b59c to
7e00f4b
Compare
secp256k1-sys/src/lib.rs
Outdated
| /// | ||
| /// # Safety | ||
| /// | ||
| /// For safety constraints see [`std::slice::from_raw_parts`] and [`std::str::from_utf8_unchecked`]. |
There was a problem hiding this comment.
In 6e19635:
I don't understand what std::str::from_utf8_unchecked has to do with anything here. slice::from_raw_parts is sorta relevant but it's really overkill because it's talking in terms of an anbitrary T but we're using C characters, which are one byte in size, have one-byte alignment, and have no trap representatons.
It would be clearer I think to just say that message must be a valid pointer to an initialized null-terminated C string.
There was a problem hiding this comment.
Ah, I see, the reason you're citing these is because in our default callback function we call from_utf8_unchecked after using strlen to compute a bound on the string in order to convert it to a u8 slice.
So we should add the additional requirement then that the string, up to the first null byte, must be valid UTF8.
Alternately we could call CStr::from_ptr (and copy its safety bounds which are sensible for this funciton) and then CStr::to_str which is safe but returns a Result, which we could just .unwrap_or("<invalid UTF8>" or something.
There was a problem hiding this comment.
Interestingly, doing this would eliminate our strlen funciton entirely, which is nice.
There was a problem hiding this comment.
Cool, that turned out nice. Pushed as a separate patch.
tcharding
force-pushed
the
11-22-document-unsafe
branch
from
1ce02bf to
4b1e8c3
Compare
|
The docs say that [0] https://doc.rust-lang.org/stable/core/ffi/struct.CStr.html#method.from_ptr |
|
[1] is not the right reference. Our actual code uses https://github.com/rust-bitcoin/rust-secp256k1/blob/master/secp256k1-sys/src/types.rs#L8-L10 because, as your docs show, the official I think we should just add a comment and do a cast/transmute here. |
|
Ah of course, masked behind us always importing |
|
Oh bother, the |
Functions that are `unsafe` should include a `# Safety` section. Because we have wrapper functions to handle symbol renaming we essentially have duplicate functions i.e., they require the same docs, instead of duplicating the docs put the symbol renamed function below the non-renamed function and add a docs linking to the non-renamed function. Also add attribute to stop the linter warning about the missing safety docs section. Remove the clippy attribute for `missing_safety_doc`.
Add a `# Safety` section to all unsafe traits, methods, and functions. Remove the clippy attribute for `missing_safety_doc`.
tcharding
force-pushed
the
11-22-document-unsafe
branch
from
4b1e8c3 to
6d74730
Compare
|
I've removed the |
Oh, derp. I feel like all this C interop stuff showed up 8 years later than I would've expected it to. |
|
Looks good now! |
Yeah it wasn't available in core at the time that I've wrote this code, and it's still higher than our MSRV |
6d747301e8dcdf5744ced973d2e2c36cdce21be4 secp256k1: Document safety constraints (Tobin C. Harding)
85681cece7e5b66a6e72abece678b9f0ae65a782 secp256k1-sys: Document safety constraints (Tobin C. Harding)
Pull request description:
Functions that are `unsafe` should include a `# Safety` section.
- Patch 1: Documents `unsafe` methods in `secp256k1-sys`. Please not this includes a minor refactor.
- Patch 2: Documents the `unsafe` `Context` trait
Together these two patches remove `#![allow(clippy::missing_safety_doc)]` from the repo.
Fix: #447
## Note to reviewers
The only function that was curly to understand the safety of was `secp256k1_context_create`, all the other stuff should be trivial to review.
ACKs for top commit:
apoelstra:
ACK 6d747301e8dcdf5744ced973d2e2c36cdce21be4
Tree-SHA512: 247216c3f9e655fe8c2854b71613b31b6241318e877e83a1e4873ce84e481975a832d05cd748577f437f88b166ff287a537d26c012568e7378caed458ec55867
6d747301e8dcdf5744ced973d2e2c36cdce21be4 secp256k1: Document safety constraints (Tobin C. Harding)
85681cece7e5b66a6e72abece678b9f0ae65a782 secp256k1-sys: Document safety constraints (Tobin C. Harding)
Pull request description:
Functions that are `unsafe` should include a `# Safety` section.
- Patch 1: Documents `unsafe` methods in `secp256k1-sys`. Please not this includes a minor refactor.
- Patch 2: Documents the `unsafe` `Context` trait
Together these two patches remove `#![allow(clippy::missing_safety_doc)]` from the repo.
Fix: #447
## Note to reviewers
The only function that was curly to understand the safety of was `secp256k1_context_create`, all the other stuff should be trivial to review.
ACKs for top commit:
apoelstra:
ACK 6d747301e8dcdf5744ced973d2e2c36cdce21be4
Tree-SHA512: 247216c3f9e655fe8c2854b71613b31b6241318e877e83a1e4873ce84e481975a832d05cd748577f437f88b166ff287a537d26c012568e7378caed458ec55867
3 participants
Functions that are
unsafeshould include a# Safetysection.unsafemethods insecp256k1-sys. Please not this includes a minor refactor.unsafeContexttraitTogether these two patches remove
#![allow(clippy::missing_safety_doc)]from the repo.Fix: #447
Note to reviewers
The only function that was curly to understand the safety of was
secp256k1_context_create, all the other stuff should be trivial to review.