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

Add no-std support #100

Merged
merged 6 commits into from
May 10, 2019
Merged

Add no-std support #100

merged 6 commits into from
May 10, 2019

Conversation

elichai
Copy link
Member

@elichai elichai commented Feb 18, 2019

Hi,
This make this library support no-std.
first I replaced everything from std to get it from core.
then I had 2 things that I had to change and would love you input:

  1. the C types from std::os::raw, I see few options. one which I did is just declare it manually which I don't really love, another is to make a feature if it will be received from std or libc.

  2. Replacing the Vec in the DER serialization. I talked with Maxwell and he told me that the signature can be anywhere between 8 and 72 bytes, so the only way I thought I can achieve this without breaking the API is using the ArrayVec crate which initializes the Vector on the stack,
    A different approach will be accepting &mut [u8; 72] and returning a slice to the correct signature that will be a part of that mutable.

@elichai
Copy link
Member Author

elichai commented Feb 18, 2019

Ok, I don't like the types.rs approach.
I would prefer to have a feature to switch between libc and libstd, but it's up to you.

@elichai
Copy link
Member Author

elichai commented Feb 18, 2019

So this might influence your thoughts on types.rs vs libc feature,
core::ffi::c_void was introduced only in rust 1.30 rust-lang/rust#53910
so to support that we either need to add that enum to types.rs too or just declare c_uchar as c_void (as I did)

@apoelstra
Copy link
Member

apoelstra commented Feb 26, 2019

  • I also don't like types.rs; I don't see how it can be correct across multiple architectures. What would a feature-flag look like?
  • I also really don't want to add any more dependencies to this library, including arrayvec. We used arrayvec in the past and removed it because it was an extra dependency. I'd prefer we return a ([u8; 74], usize) or use an outptr.
  • Context creation still uses malloc in the C function secp256k1_context_create. Is this okay for nostd users or do we need to change to use the preallocation API?

@elichai
Copy link
Member Author

elichai commented Feb 26, 2019

  1. I'll try to think of a better solution, maybe feature based and let you know what I come up with.

  2. If you don't care about breaking the API I think the better solution would be to accept a &mut [u8; 74] into the function and return a &[u8] of the exact slice that contains the DER which will be part of the original mut slice,
    this way all the allocations are on the user behalf and he receives back exactly the right thing without needing to slice it afterwards, what do you think?

  3. I think malloc is support everywhere liballoc is supported. so in my case for example, I'm putting this in SGX so I need no-std but I can allow mallocs so it really depends on the use case. some embedded devices even have no support for liballoc and there it will be a problem(AFAIK).

  4. After some testing there's a problem with the fact that the C code usesfprintf,
    after I commented all the uses out I managed to create keys and sign a message successfully.

@elichai
Copy link
Member Author

elichai commented Feb 26, 2019

Ok, so about 1, I see a few possibilities.

  • get libc back with default-features=false
  • forget about these types or use the types.rs because there isn't different types on different architecture with the std::os::raw::* it's just that on some architectures these types are missing.
  • Just copy the whole std::os::raw file https://doc.rust-lang.org/src/std/os/raw/mod.rs.html#11-108

@apoelstra
Copy link
Member

It's really surprising to me that there aren't different types for different architectures.

Regarding accepting an &mut [u8; 74] and returning an exact slice, sure, that sounds fine to me. But if std is enabled I'd like to keep the existing API, it's much more ergonomic :).

Regarding the printf calls, is the issue with compilation or can it be fixed by providing different error callbacks at runtime?

@elichai
Copy link
Member Author

elichai commented Feb 26, 2019

It's in runtime.
And I guess it's possible to provide two versions of the same function, although that will be hard to see in docs.

Maybe adding a new serliaze_mut or serialize_no_vec or something similiar and the regular one won't be available without std

@elichai
Copy link
Member Author

elichai commented Feb 26, 2019

Wait. Not on compilation, it's in runtime. Sorry my mistake.

@apoelstra
Copy link
Member

Yeah, good idea naming the alternate serialize function something different.

And if there are runtime issues this is fine; during context creation we can use the FFI functions secp256k1_context_set_illegal_callback and secp256k1_context_set_error_callback to replace the offending printf-calling functions with different ones. This way we won't need to patch the C code.

@elichai
Copy link
Member Author

elichai commented Feb 27, 2019

@apoelstra Thanks for the feedback.
I did the first one, I can give the second one a try and see if I get it right.

BTW how does this work? https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/lib.rs#L338
does the C code do the allocations?
because AFAIK with_capacity() doesn't allocate anything, it just tells it that the first time there'll be an allocation it will be with that capacity

@apoelstra
Copy link
Member

No, the C code does not do allocations. My understanding was that with_capacity would do the allocation -- I don't see how it could behave otherwise. Where in the Vec structure would the "amount to allocate on next allocation" be stored?

@elichai
Copy link
Member Author

elichai commented Feb 27, 2019

@apoelstra
Copy link
Member

@elichai The cap field stores the amount of currently-allocated memory. The with_capacity constructor definitely allocates memory and the docs don't suggest otherwise.

@apoelstra
Copy link
Member

If you're using "allocate" to refer to setting the vector's length, we do this manually after the FFI call by calling the unsafe set_len method on the vector.

@elichai
Copy link
Member Author

elichai commented Feb 27, 2019

@apoelstra Ok, I were wrong and you were right.
the docs were a bit confusing for me because it's written a bit through negation If capacity is 0, the vector will not allocate meaning that if it's not 0 it will allocate

I'm really Sorry for the confusion

@elichai
Copy link
Member Author

elichai commented Mar 3, 2019

So my problems appears neither in compilation nor in runtime.
To run a code in SGX I need to later link it with some other C files and it fails in linkage because it tries to link with stdio:

$ g++ enclave/Enclave_t.o -o enclave/enclave.so -m64 -O2 -Wl,--no-undefined -nostdlib -nodefaultlibs -nostartfiles -L/opt/intel/sgxsdk/lib64 -Wl,--whole-archive -lsgx_trts -lsgx_tservice -Wl,--no-whole-archive -Wl,--start-group -lsgx_tstdc -lsgx_tcxx -lsgx_tcrypto -L./lib -lcompiler-rt-patch -lenclave -Wl,--end-group -Wl,-Bstatic -Wl,-Bsymbolic -Wl,--no-undefined -Wl,-pie,-eenclave_entry -Wl,--export-dynamic -Wl,--defsym,__ImageBase=0 -Wl,--gc-sections -Wl,--version-script=enclave/Enclave.lds
./lib/libenclave.a(secp256k1.o): In function `fprintf':
/usr/include/x86_64-linux-gnu/bits/stdio2.h:97: undefined reference to `stderr'
/usr/include/x86_64-linux-gnu/bits/stdio2.h:97: undefined reference to `__fprintf_chk'
/usr/include/x86_64-linux-gnu/bits/stdio2.h:97: undefined reference to `stderr'
/usr/include/x86_64-linux-gnu/bits/stdio2.h:97: undefined reference to `__fprintf_chk'
/usr/include/x86_64-linux-gnu/bits/stdio2.h:97: undefined reference to `stderr'
/usr/include/x86_64-linux-gnu/bits/stdio2.h:97: undefined reference to `__fprintf_chk'
collect2: error: ld returned 1 exit status

@elichai
Copy link
Member Author

elichai commented Mar 3, 2019

@apoelstra how do you want to proceed with this?
Should I open an Issue on https://github.com/bitcoin-core/secp256k1 to see what sipa/gmaxwell think?

@real-or-random
Copy link
Collaborator

@elichai I'll create a simple PR to secp256k1 tomorrow that should make it possible to build it without stdio.h (I'm runing into similar problems with a different project).

@elichai
Copy link
Member Author

elichai commented Mar 3, 2019

@real-or-random Thanks! tag me in it so i'll follow when it gets merged :)

@elichai
Copy link
Member Author

elichai commented Mar 11, 2019

So I think it's best to put this on hold until these two are figured out:
bitcoin-core/secp256k1#566
bitcoin-core/secp256k1#595

@elichai
Copy link
Member Author

elichai commented Apr 11, 2019

I just thought of another interesting way to make the serialize_der() no-std,
We can make a struct that has a 72 byte array and a length,
then that struct can implement AsRef / Have a slice() function that returns &arr[..len],
then the user is invisible to what happens but he get's a struct that can be easily serializable into his signature.

@apoelstra
Copy link
Member

Oh! Yeah, I like this idea. We could call it a SerSignature and give it slicing ability and the ability to go to/from a regular Signature.

@elichai
Copy link
Member Author

elichai commented Apr 11, 2019

Should that be as part of serialize_der_no_alloc() or the regular serialize_der()?
(I think it's a good enough solution but it will break the api.)

@apoelstra
Copy link
Member

I think we should go ahead and break the API and change serialize_der. I hate that this function returns a Vec anyway.

@elichai
Copy link
Member Author

elichai commented Apr 11, 2019

BTW, is it using this: https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/ffi.rs#L419 or the one from the bindings? (https://github.com/rust-bitcoin/rust-secp256k1/blob/master/src/ffi.rs#L198)

If the rust one, what do you think about a re-implementation in safe rust?

@apoelstra
Copy link
Member

We are using the FFI one. The other is something Matt wrote for fuzzing; I doubt that it actually serializes a signature (though it might); these implementations are there to provide low-complexity mock implementations of the crypto, to prevent the fuzzer getting lost trying to break cryptography.

I like the idea of a safe reimplementation, though it does create a layer violation: the types exposed by the underlying secp256k1 are considered opaque and subject to change without warning. On the other hand, since rust-secp bundles a fixed version of secp256k1, it won't change without us taking active steps..

@apoelstra
Copy link
Member

Cool. I'm happy with the result. Can you rebase/squash so that we don't go through the ArrayVec attempt in the git history?

@elichai
Copy link
Member Author

elichai commented Apr 14, 2019

Done. (the changes from before the rebasing to after are here if you're interested: https://github.com/rust-bitcoin/rust-secp256k1/compare/0e0d8df84b001da2ba42c07205ad39e739c532ad..41af612ae277962b4b1747c5b58704bd40e1819d )

Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

looks good to me overall :)

src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/types.rs Outdated
pub type c_int = i32;
pub type c_uchar = u8;
pub type c_uint = u32;
pub type c_void = c_uchar;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it really correct to just use u8 for c_void? That comment makes me think otherwise https://doc.rust-lang.org/src/core/ffi.rs.html#21 (though I don't understand it).

Copy link
Member Author

Choose a reason for hiding this comment

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

In one of the comments here I think @apoelstra pointed out that *void is almost equivalent to *char, it's hard to look for resolved comments on the phone.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But wouldn't it be cleaner to copy the enum implementation with the two variants? It's just three lines of code, so it wouldn't hurt I guess.

Copy link
Member

Choose a reason for hiding this comment

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

Agree with real-or-random. I don't mind type-punning *c_void for *c_uchar but the base types c_void and c_uchar are very different.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I see two options.
one is to remove the c_void type and replace the pointers to *c_uchar, another is what @real-or-random suggested and copy the enum implementation.

I'm really no expert in C and don't know the implications if someone later on misuse c_void with c_uchar so I don't have any opinion on this, I think it should be as foolproof as possible.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should copy the enum implementation and link to the libc code, and mention that in Rust 1.30 we will be able to just use core::ffi.

Copy link
Member Author

@elichai elichai Apr 24, 2019

Choose a reason for hiding this comment

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

Ok, FYI c_void != void.
but *const c_void == const void* and *mut c_void == void*

@elichai elichai force-pushed the master branch 2 times, most recently from 5f04fcb to f42cfcb Compare April 24, 2019 12:35
src/types.rs Outdated Show resolved Hide resolved
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK. Thank you for your patience!!

cc @real-or-random @jonasnick can you also approve since you've been guiding this PR?

@real-or-random real-or-random self-requested a review May 1, 2019 14:07
Copy link
Collaborator

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

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

ACK

@real-or-random real-or-random merged commit ab4320f into rust-bitcoin:master May 10, 2019
mattaudesse added a commit to AlacrisIO/mkb that referenced this pull request May 14, 2019
gmaxwell added a commit to bitcoin-core/secp256k1 that referenced this pull request May 27, 2019
e49f799 Add missing #(un)defines to base-config.h (Tim Ruffing)
77defd2 Add secp256k1_ prefix to default callback functions (Tim Ruffing)
908bdce Include stdio.h and stdlib.h explicitly in secp256k1.c (Tim Ruffing)
5db782e Allow usage of external default callbacks (Tim Ruffing)
6095a86 Replace CHECKs for no_precomp ctx by ARG_CHECKs without a return (Tim Ruffing)

Pull request description:

  This is intended for environments without implementations for `abort()`, `fprintf()`, and `stderr`. e.g., embedded systems. Those can provide their own implementations of `default_illegal_callback_fn` and `default_error_callback_fn` at compile time.

  If you want to use your own default callback, things will be somewhat inconsistent unfortunately: We cannot make the callback data `extern` too, because then the initialization lists for `default_illegal_callback` won't contain only constants. (`const` variables are not compile-time constants). So you cannot take callback data in your own default callback function.

  As a more drastic/breaking alternative I suggest to remove the callback data entirely. I don't think it's a big loss and I would be surprised if anyone uses it. Additionally, we could even remove the possibility to set the callback function at runtime after this PR. This will simplify things a lot, and again I don't think it's a big loss.

  Note that `abort()`, `fprintf()`, and `stderr` are also used in `CHECK`, which is still used in production code if we rely on gmp for scalar and field inversions (e.g.,  https://github.com/bitcoin-core/secp256k1/blob/master/src/scalar_impl.h#L240). This is not an issue for embedded system which probably don't want to use gmp anyway, but it is probably an issue for the reasons explained in #566 (comment).

  (related downstream: rust-bitcoin/rust-secp256k1#100 @elichai)

ACKs for commit e49f79:

Tree-SHA512: 4dec0821eef4156cbe162bd8cdf0531c1fae8c98cd9db8438170ff1aa0e59b199739eeab293695bb582246812bea5309959f02f1fb74bb57872da54ebc52313f
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

4 participants