fix: improve ffi safety #247

dignifiedquire · 2022-03-18T19:10:47Z

*mut ptr to indicate ownership
Boxed slices instead of vecs for safe transfers
expanded checks for null ptrs
refactor to use no more CStrings
improve api and safety
C -> go bindings: remove c-for-go in favor of manual bindings
rust -> C bindings: use safer_ffi instead of bindgen
check for enum values passed in the ffi interface

Closes filecoin-project/rust-fil-ffi-toolkit#9, #231

TODOs

~~Depends on filecoin-project/rust-fil-ffi-toolkit#11~~

rust/src/bls/api.rs

rust/src/util/types.rs

Stebalien · 2022-03-21T14:15:07Z

I think we're also missing some null checks inside fil_verify_aggregate_seal_proof.

rust/src/proofs/api.rs

Stebalien

A few more comments, but otherwise this looks good to go.

Stebalien · 2022-03-22T14:31:44Z

rust/src/util/api.rs

+        let devices: Vec<fil_Bytes> = devices.iter().map(|d| d.name().into()).collect();
+        let devices: fil_Array<fil_Bytes> = devices.into();


We could implement FromIterator for fil_Array.

rust/src/proofs/api.rs

Stebalien · 2022-03-22T14:59:57Z

rust/src/proofs/api.rs

 #[no_mangle]
 pub unsafe extern "C" fn fil_generate_fallback_sector_challenges(
    registered_proof: fil_RegisteredPoStProof,
    randomness: fil_32ByteArray,
-    sector_ids_ptr: *const u64,
-    sector_ids_len: libc::size_t,
+    sector_ids: &fil_Array<u64>,


future note: with a repr(transparent), we may be able to use a fil_Array<SectorId> here. But that's a future change.

rust/src/proofs/api.rs

rust/src/util/types.rs

rust/src/bls/api.rs

magik6k

Not seeing anything very wrong here, but I feel like this probably deserves at least one more look from someone who in more into rust.

magik6k · 2022-03-25T15:37:33Z

rust/src/bls/api.rs

 /// * `messages_sizes_ptr`        - pointer to an array containing the lengths of the messages
 /// * `messages_len`              - length of the two messages arrays
 /// * `flattened_public_keys_ptr` - pointer to a byte array containing public keys
 /// * `flattened_public_keys_len` - length of the array


Suggested change

/// * `messages_sizes_ptr` - pointer to an array containing the lengths of the messages

/// * `messages_len` - length of the two messages arrays

/// * `flattened_public_keys_ptr` - pointer to a byte array containing public keys

/// * `flattened_public_keys_len` - length of the array

/// * `messages_sizes` - pointer to an array containing the lengths of the messages

/// * `flattened_public_keys` - pointer to a byte array containing public keys

Suggested change

/// * `messages_sizes_ptr` - pointer to an array containing the lengths of the messages

/// * `messages_len` - length of the two messages arrays

/// * `flattened_public_keys_ptr` - pointer to a byte array containing public keys

/// * `flattened_public_keys_len` - length of the array

/// * `signature` - byte array containing the signature

/// * `messages` - array containing the pointers to the messages

/// * `messages_sizes` - array containing the lengths of the messages

/// * `flattened_public_keys` - byte array containing public keys

Stebalien

Some ideas/comments. Honestly, the unsafety of things like ptr.Slice() is probably fine given the context.

cgo/types.go

Stebalien · 2022-03-29T21:57:29Z

cgo/types.go

+func (ptr SliceBoxedUint8) Slice() []byte {
+	return unsafe.Slice((*byte)(ptr.ptr), int(ptr.len))
+}


Maybe call this something like UnsafeSlice (and do we actually need it?)

I just want to make it super clear that the underlying slice may be freed.

cgo/types.go

cgo/proofs.go

cgo/types.go

vmx

Only small things and some of them that could/should be done on subsequent PRs.

vmx · 2022-03-30T07:49:50Z

rust/src/bls/api.rs

@@ -12,32 +10,36 @@ use rand::rngs::OsRng;
 use rand::SeedableRng;
 use rand_chacha::ChaChaRng;
 use rayon::prelude::*;
+use safer_ffi::prelude::*;


I'm not a fan of star imports. When you're not familiar with safer-ffi, I'd wonder "where does that derive_ReprC come from? Is that a new builtin I haven't heard of?" when reading the source. As this PR is already huge, I'd be happy to change it myself in a later PR.

This is especially confusing because we have an str module in here.

rust/src/bls/api.rs

vmx · 2022-03-30T08:05:00Z

rust/src/bls/api.rs

 /// * `messages_sizes_ptr`        - pointer to an array containing the lengths of the messages
 /// * `messages_len`              - length of the two messages arrays
 /// * `flattened_public_keys_ptr` - pointer to a byte array containing public keys
 /// * `flattened_public_keys_len` - length of the array


Suggested change

/// * `messages_sizes_ptr` - pointer to an array containing the lengths of the messages

/// * `messages_len` - length of the two messages arrays

/// * `flattened_public_keys_ptr` - pointer to a byte array containing public keys

/// * `flattened_public_keys_len` - length of the array

/// * `signature` - byte array containing the signature

/// * `messages` - array containing the pointers to the messages

/// * `messages_sizes` - array containing the lengths of the messages

/// * `flattened_public_keys` - byte array containing public keys

rust/src/bls/api.rs

vmx

I'm still not a fan of glob imports, but other than that it looks good. I've only reviewed the Rust part of it btw.

vmx

Only two minor, non blocking things.

I've only reviewed the Rust side of things. Great work! This is really a massive improvement and also a huge amount of tiresome work. Wow.

rust/src/fvm/blockstore/cgo.rs

rust/src/fvm/machine.rs

Kubuxu · 2022-04-13T14:57:47Z

I will restate the question regarding changes to function signatures. filecoin-ffi is used by multiple downstream projects, and changes to function signatures will have painful side-effects.

Kubuxu · 2022-04-13T15:06:36Z

cgo/helpers.go

+	if len == 0 {
+		// can't take element 0 of an empty slice
+		return SliceRefUint8{
+			ptr: (*C.uint8_t)(unsafe.Pointer(&goBytes)),


I would feel better if we had a global uint8 variable and were taking pointer to it instead of the slice.
Reinterpreting the slice pointer to C.uint8_t feels wrong.

But it would make the code more verbose (all different types).
Alternative would be the pointer to be nil which AFAIK is acceptable from Go's perspective.

I don't understand the concern/solution I am afraid.

@Kubuxu can you re-explain this if it's still a concern?

explained in person, fix just pushed

Kubuxu · 2022-04-13T15:12:49Z

Same comment as previously on Slack: as the bindings are written for specific headers, I would would prefer if header file itself was checked in. I can be convinced otherwise but from my perspective, it makes changes more auditable as most Go types not are aliases to C types.

dignifiedquire · 2022-04-13T15:54:23Z

I will restate the question regarding changes to function signatures. filecoin-ffi is used by multiple downstream projects, and changes to function signatures will have painful side-effects.

Can you clarify which part of the api. The C-API or the go-api?

dignifiedquire · 2022-04-13T15:54:56Z

Same comment as previously on Slack: as the bindings are written for specific headers, I would would prefer if header file itself was checked in. I can be convinced otherwise but from my perspective, it makes changes more auditable as most Go types not are aliases to C types.

I don't mind either way, happy to do as others think makes the most sense

Kubuxu · 2022-04-13T16:46:14Z

Can you clarify which part of the api. The C-API or the go-api?

I know of few projects using the Go side of filecoin-ffi. I'm not aware of any using the C/rust side.

raulk · 2022-04-25T09:06:17Z

Unfortunately this requires a merge from master again following the merge of execution traces. @dignifiedquire do you have a few minutes to look at this?

cgo/helpers.go

cgo/types.go

distributed.go

proofs.go

Stebalien · 2022-04-25T12:04:41Z

rust/src/bls/api.rs

@@ -12,32 +10,36 @@ use rand::rngs::OsRng;
 use rand::SeedableRng;
 use rand_chacha::ChaChaRng;
 use rayon::prelude::*;
+use safer_ffi::prelude::*;


This is especially confusing because we have an str module in here.

cgo/types.go

Stebalien · 2022-04-25T13:33:56Z

rust/src/fvm/types.rs

    pub(crate) machine: Option<Mutex<CgoExecutor>>,
 }

+pub type FvmMachine = Option<repr_c::Box<InnerFvmMachine>>;


This is a very confusing type.

not a great type, but necessary, I'll add a comment

Stebalien · 2022-04-25T18:22:20Z

cgo/proofs.go

 	if err := CheckErr(resp); err != nil {
+		faults := resp.value.faulty_sectors.copy()


This is invalid memory otherwise? Maybe we should fix it in rust?

it wasn’t an issue, just wanted to make the code more defensive on both sides

Stebalien · 2022-04-28T14:17:13Z

rust/scripts/build-release.sh

@@ -64,6 +64,10 @@ main() {
        find . -type f -name "lib$1.a"
    fi

+    # generate filcrypto.h
+    RUSTFLAGS="${__rust_flags}" HEADER_DIR="." \
+        cargo test build_headers --features c-headers 


@dignifiedquire why build with a test?

Basically, it means we need to rebuild everything (and we should just be using cargo run with a bin target).

Mostly because that is how the safer-ffi docs do it, and it didn't quite work when doing it differently due to how that project currently works.

Stebalien · 2022-04-28T14:46:55Z

Ok. I've now synced the chain with the address sanitizer enabled (both in rust and go) with both the FVM and the lotus VM.

Now I'm running the lotus tests with the address sanitizer enabled.

Stebalien · 2022-04-28T15:40:57Z

I'm getting:

    github.com/filecoin-project/lotus/storage.(*WindowPoStScheduler).runPoStCycle
        /var/lib/lotus/src/storage/wdpost_run.go:725
  - merge windowPoSt partition proofs:
    github.com/filecoin-project/lotus/extern/sector-storage.(*Manager).generateWindowPoSt
        /var/lib/lotus/src/extern/sector-storage/manager_post.go:207
  - SlicedBoxedUint8 must not be empty

Why can this not be empty?

They're fine. We can't represent them as null, but rust will internally represent them as 0x1. We could just _assume_ that, but it's safer to call into rust and let it handle this case.

laser · 2022-05-19T23:45:57Z

C -> go bindings: remove c-for-go in favor of manual bindings

Wow!

dignifiedquire requested review from vmx, Stebalien and cryptonemo March 18, 2022 19:10

vmx approved these changes Mar 21, 2022

View reviewed changes

Stebalien reviewed Mar 21, 2022

View reviewed changes

rust/src/proofs/api.rs Outdated Show resolved Hide resolved

Stebalien reviewed Mar 21, 2022

View reviewed changes

rust/src/proofs/api.rs Show resolved Hide resolved

rust/src/proofs/api.rs Outdated Show resolved Hide resolved

Stebalien reviewed Mar 22, 2022

View reviewed changes

dignifiedquire marked this pull request as draft March 23, 2022 09:14

Stebalien reviewed Mar 23, 2022

View reviewed changes

rust/src/bls/api.rs Outdated Show resolved Hide resolved

magik6k reviewed Mar 25, 2022

View reviewed changes

Stebalien reviewed Mar 29, 2022

View reviewed changes

vmx reviewed Mar 30, 2022

View reviewed changes

raulk mentioned this pull request Mar 30, 2022

Open areas of hardening/investigation filecoin-project/ref-fvm#428

Closed

12 tasks

dignifiedquire marked this pull request as ready for review March 30, 2022 12:47

raulk requested review from vmx, Stebalien, Kubuxu and magik6k March 31, 2022 16:08

vmx approved these changes Mar 31, 2022

View reviewed changes

dignifiedquire force-pushed the safer-ffi branch 2 times, most recently from 89f57b6 to 55b02e4 Compare April 12, 2022 18:37

vmx approved these changes Apr 13, 2022

View reviewed changes

rust/src/fvm/blockstore/cgo.rs Show resolved Hide resolved

rust/src/fvm/machine.rs Show resolved Hide resolved

Kubuxu reviewed Apr 13, 2022

View reviewed changes

update install script for header location

edba97a

dignifiedquire mentioned this pull request Apr 22, 2022

use filecoin-ffi@safer-ffi filecoin-project/lotus#8538

Closed

dignifiedquire added 2 commits April 22, 2022 14:38

add initlogfd

966d3f1

test fixes

b31c9e7

Merge branch 'master' into safer-ffi

abf7231

Stebalien force-pushed the safer-ffi branch from dbf350c to abf7231 Compare April 25, 2022 11:11

Stebalien added 3 commits April 25, 2022 13:29

chore: remove cgo-gen

5677db6

chore: avoid copying failure_info twice

c04b10b

chore: remove paramfetch

28966ce

Stebalien approved these changes Apr 25, 2022

View reviewed changes

Stebalien and others added 2 commits April 25, 2022 14:22

fix: destroy SliceBoxedUint8 by ref

bc77831

apply cr

786f855

dignifiedquire commented Apr 25, 2022

View reviewed changes

cgo/types.go Show resolved Hide resolved

avoid double free in fvm machine execution

9ef8bcf

Stebalien reviewed Apr 25, 2022

View reviewed changes

fixes for some issues found through lotus tests

e0e4ac4

Stebalien reviewed Apr 25, 2022

View reviewed changes

dignifiedquire and others added 2 commits April 25, 2022 21:38

fix: do not destroy info that gets passed

efe0d8c

fix: copy nil

e556fc8

Stebalien reviewed Apr 28, 2022

View reviewed changes

fix: allow zero-length slice allocs

8b963d4

They're fine. We can't represent them as null, but rust will internally represent them as 0x1. We could just _assume_ that, but it's safer to call into rust and let it handle this case.

Stebalien merged commit bb281b5 into master Apr 28, 2022

Stebalien deleted the safer-ffi branch April 28, 2022 18:54

This was referenced May 3, 2022

WIP demo nicer FFI types #229

Closed

nv16 development checklist filecoin-project/ref-fvm#531

Closed

raulk mentioned this pull request May 10, 2022

[nv16] propagate gas outputs from fvm + integrate safer ffi filecoin-project/lotus#8593

Merged

		let devices: Vec<fil_Bytes> = devices.iter().map(\|d\| d.name().into()).collect();
		let devices: fil_Array<fil_Bytes> = devices.into();

		if err := CheckErr(resp); err != nil {
		faults := resp.value.faulty_sectors.copy()

fix: improve ffi safety #247

fix: improve ffi safety #247

Conversation

dignifiedquire commented Mar 18, 2022 • edited Loading

Stebalien commented Mar 21, 2022

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magik6k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vmx left a comment

Choose a reason for hiding this comment

vmx left a comment

Choose a reason for hiding this comment

Kubuxu commented Apr 13, 2022

Choose a reason for hiding this comment

Kubuxu Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kubuxu commented Apr 13, 2022

dignifiedquire commented Apr 13, 2022

dignifiedquire commented Apr 13, 2022

Kubuxu commented Apr 13, 2022

raulk commented Apr 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Apr 28, 2022

Stebalien commented Apr 28, 2022

laser commented May 19, 2022

dignifiedquire commented Mar 18, 2022 •

edited

Loading

Kubuxu Apr 13, 2022 •

edited

Loading