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

fix: improve ffi safety #247

Merged
merged 19 commits into from
Apr 28, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cgo/proofs.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,13 @@ func GenerateWinningPoSt(randomness *ByteArray32, replicas SliceRefPrivateReplic
func GenerateWindowPoSt(randomness *ByteArray32, replicas SliceRefPrivateReplicaInfo, proverId *ByteArray32) ([]PoStProofGo, []uint64, error) {
resp := C.generate_window_post(randomness, replicas, proverId)
defer resp.destroy()
faults := resp.value.faulty_sectors.copy()

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return nil, faults, err
}

proofs := resp.value.proofs.copy()
return proofs, faults, nil
return proofs, []uint64{}, nil
}

func GetGpuDevices() ([]string, error) {
Expand Down
8 changes: 4 additions & 4 deletions proofs.go
Original file line number Diff line number Diff line change
Expand Up @@ -586,17 +586,17 @@ func GenerateWindowPoSt(

randomnessBytes := cgo.AsByteArray32(randomness)
proofsRaw, faultsRaw, err := cgo.GenerateWindowPoSt(&randomnessBytes, cgo.AsSliceRefPrivateReplicaInfo(filReplicas), &proverID)
faultySectors := fromFilPoStFaultySectors(faultsRaw)
if err != nil {
faultySectors := fromFilPoStFaultySectors(faultsRaw)
return nil, faultySectors, err
}

proofs, err := fromFilPoStProofs(proofsRaw)
if err != nil {
return nil, faultySectors, err
return nil, nil, err
}

return proofs, faultySectors, nil
return proofs, nil, nil
}

// GetGPUDevices produces a slice of strings, each representing the name of a
Expand Down Expand Up @@ -796,7 +796,7 @@ func toFilPrivateReplicaInfos(src []PrivateSectorInfo, typ string) ([]cgo.Privat
func fromFilPoStFaultySectors(ptr []uint64) []abi.SectorNumber {
snums := make([]abi.SectorNumber, len(ptr))
for i := range ptr {
snums = append(snums, abi.SectorNumber(ptr[i]))
snums[i] = abi.SectorNumber(ptr[i])
}

return snums
Expand Down
4 changes: 2 additions & 2 deletions rust/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 4 additions & 2 deletions rust/src/util/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ use std::sync::Once;
use anyhow::anyhow;
use safer_ffi::prelude::*;

use super::types::{catch_panic_response, GpuDeviceResponse, InitLogFdResponse};
use super::types::{
catch_panic_response, catch_panic_response_no_log, GpuDeviceResponse, InitLogFdResponse,
};

/// Protects the init off the logger.
static LOG_INIT: Once = Once::new();
Expand Down Expand Up @@ -53,7 +55,7 @@ pub fn get_gpu_devices() -> repr_c::Box<GpuDeviceResponse> {
/// be initializes implicitely and log to stderr.
#[ffi_export]
pub fn init_log_fd(log_fd: libc::c_int) -> repr_c::Box<InitLogFdResponse> {
catch_panic_response("init_log_fd", || {
catch_panic_response_no_log(|| {
let file = unsafe { File::from_raw_fd(log_fd) };

if init_log_with_file(file).is_none() {
Expand Down
32 changes: 24 additions & 8 deletions rust/src/util/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,18 +160,20 @@ where
})
}

pub fn catch_panic_response_raw<F, T>(name: &str, callback: F) -> repr_c::Box<Result<T>>
pub fn catch_panic_response_no_log<F, T>(callback: F) -> repr_c::Box<Result<T>>
where
T: Sized + Default,
F: FnOnce() -> anyhow::Result<T> + std::panic::UnwindSafe,
{
catch_panic_response_raw_no_log(|| Result::from(callback().map_err(|err| err.to_string())))
}

pub fn catch_panic_response_raw_no_log<F, T>(callback: F) -> repr_c::Box<Result<T>>
where
T: Sized + Default,
F: FnOnce() -> Result<T> + std::panic::UnwindSafe,
{
let result = match panic::catch_unwind(|| {
init_log();
log::info!("{}: start", name);
let res = callback();
log::info!("{}: end", name);
res
}) {
let result = match panic::catch_unwind(callback) {
Ok(t) => t,
Err(panic) => {
let error_msg = match panic.downcast_ref::<&'static str>() {
Expand All @@ -186,6 +188,20 @@ where
repr_c::Box::new(result)
}

pub fn catch_panic_response_raw<F, T>(name: &str, callback: F) -> repr_c::Box<Result<T>>
where
T: Sized + Default,
F: FnOnce() -> Result<T> + std::panic::UnwindSafe,
{
catch_panic_response_raw_no_log(|| {
init_log();
log::info!("{}: start", name);
let res = callback();
log::info!("{}: end", name);
res
})
}

pub unsafe fn catch_panic_response_no_default<F, T>(
name: &str,
callback: F,
Expand Down