Skip to content

Commit

Permalink
fix: ensure that memory is free'd using the same allocator (#4101)
Browse files Browse the repository at this point in the history
We were encountering segfaults on certain combinations of go / rust
versions. This was determined to be due to differing allocators. Memory
reserved on one side of the sandwich and free'd on the other would
sometimes fail depending on toolchains / platform. This resolves that
problem.

To complicate things, this issue would only arise during
cross-compilation, and not when building on the target OS / arch, as the
allocator would presumably be the same.

### Testing Instructions

I am going to run a dry run canary that we can re-test, though I have
manualy verified the cross-compilation for win amd64.
  • Loading branch information
arlyon authored Mar 7, 2023
1 parent 4a57005 commit a6d1d47
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 3 deletions.
2 changes: 2 additions & 0 deletions cli/internal/ffi/bindings.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ typedef struct Buffer {
uint8_t *data;
} Buffer;

void free_buffer(struct Buffer buffer);

struct Buffer get_turbo_data_dir(void);

struct Buffer changed_files(struct Buffer buffer);
Expand Down
32 changes: 29 additions & 3 deletions cli/internal/ffi/ffi.go
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
package ffi

// ffi
//
// Please read the notes about safety (marked with `SAFETY`) in both this file,
// and in turborepo-ffi/lib.rs before modifying this file.

// #include "bindings.h"
//
// #cgo darwin,arm64 LDFLAGS: -L${SRCDIR} -lturborepo_ffi_darwin_arm64 -lz -liconv
Expand All @@ -25,7 +30,12 @@ func Unmarshal[M proto.Message](b C.Buffer, c M) error {
return err
}

b.Free()
// free the buffer on the rust side
//
// SAFETY: do not use `C.free_buffer` to free a buffer that has been allocated
// on the go side. If you happen to accidentally use the wrong one, you can
// expect a segfault on some platforms. This is the only valid callsite.
C.free_buffer(b)

return nil
}
Expand All @@ -42,12 +52,27 @@ func Marshal[M proto.Message](c M) C.Buffer {
return toBuffer(bytes)
}

// Free frees a buffer that has been allocated *on the go side*.
//
// SAFETY: this is not the same as `C.free_buffer`, which frees a buffer that
// has been allocated *on the rust side*. If you happen to accidentally use
// the wrong one, you can expect a segfault on some platforms.
//
// EXAMPLE: it is recommended use this function via a `defer` statement, like so:
//
// reqBuf := Marshal(&req)
// defer reqBuf.Free()
func (c C.Buffer) Free() {
C.free(unsafe.Pointer(c.data))
}

// rather than use C.GoBytes, we use this function to avoid copying the bytes,
// since it is going to be immediately Unmarshalled into a proto.Message
//
// SAFETY: go slices contain a pointer to an underlying buffer with a length.
// if the buffer is known to the garbage collector, dropping the last slice will
// cause the memory to be freed. this memory is owned by the rust side (and is
// not known the garbage collector), so dropping the slice will do nothing
func toBytes(b C.Buffer) []byte {
var out []byte

Expand Down Expand Up @@ -102,9 +127,9 @@ func ChangedFiles(repoRoot string, fromCommit string, toCommit string, includeUn
RelativeTo: relativeToRef,
}
reqBuf := Marshal(&req)
defer reqBuf.Free()

respBuf := C.changed_files(reqBuf)
reqBuf.Free()

resp := ffi_proto.ChangedFilesResp{}
if err := Unmarshal(respBuf, resp.ProtoReflect().Interface()); err != nil {
Expand All @@ -126,8 +151,9 @@ func PreviousContent(repoRoot, fromCommit, filePath string) ([]byte, error) {
}

reqBuf := Marshal(&req)
defer reqBuf.Free()

respBuf := C.previous_content(reqBuf)
reqBuf.Free()

resp := ffi_proto.PreviousContentResp{}
if err := Unmarshal(respBuf, resp.ProtoReflect().Interface()); err != nil {
Expand Down
15 changes: 15 additions & 0 deletions crates/turborepo-ffi/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
//! turborepo-ffi
//!
//! Please read the notes about safety (marked with `SAFETY`) in both this file,
//! and in ffi.go before modifying this file.
use std::{mem::ManuallyDrop, path::PathBuf};

mod proto {
Expand All @@ -11,6 +16,16 @@ pub struct Buffer {
data: *mut u8,
}

#[no_mangle]
pub extern "C" fn free_buffer(buffer: Buffer) {
// SAFETY
// it is important that any memory allocated in rust, is freed in rust
// we do this by converting the raw pointer to a Vec and letting it drop
// this is safe because we know that the memory was allocated by rust
// and that the length is correct
let _ = unsafe { Vec::from_raw_parts(buffer.data, buffer.len as usize, buffer.len as usize) };
}

impl<T: prost::Message> From<T> for Buffer {
fn from(value: T) -> Self {
let mut bytes = ManuallyDrop::new(value.encode_to_vec());
Expand Down

0 comments on commit a6d1d47

Please sign in to comment.