Adding some more miri tests #503

nicklan · 2024-11-16T00:09:20Z

What changes are proposed in this pull request?

Add more tests to ffi so we're using miri to test more things.

Also, add a testing only shallow_copy method to handles. This creates a new Handle just by copying the interior pointer value .

Functions like snapshot want to take an owned Handle<SharedExternEngine>, which means we can't "reuse" a handle from rust code. But the semantics are actually that dropping the handle (which will happen after calling snapshot) does not drop the underlying data, so if we call snapshot from "regular" rust code, we actually leak it. So to "simulate" what happens in c/c++ we can shallow_copy which just makes another pointer, so you don't have to lose access to the original. This is really not safe, so it's only available in #[cfg(test)]

codecov · 2024-11-16T00:14:41Z

Codecov Report

Attention: Patch coverage is 97.10145% with 2 lines in your changes missing coverage. Please review.

Project coverage is 80.73%. Comparing base (67cc099) to head (245404c).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
ffi/src/lib.rs	96.77%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #503      +/-   ##
==========================================
+ Coverage   79.82%   80.73%   +0.90%     
==========================================
  Files          57       61       +4     
  Lines       12591    13375     +784     
  Branches    12591    13375     +784     
==========================================
+ Hits        10051    10798     +747     
- Misses       2006     2027      +21     
- Partials      534      550      +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

scovich · 2024-11-16T12:12:13Z

ffi/src/lib.rs

+        let s: &str = unsafe { TryFromStringSlice::try_from_slice(&kernel_str).unwrap() };
+        let b = Box::new(s.to_string());


There's an impl for String as well:

Suggested change

let s: &str = unsafe { TryFromStringSlice::try_from_slice(&kernel_str).unwrap() };

let b = Box::new(s.to_string());

let s = unsafe { String::try_from_slice(&kernel_str).unwrap() };

let b = s.into_boxed_str();

Also, I'm pretty sure that String::into_boxed_str and From<Box<str>> for String are non-allocating and thus preferable to Box<String> that will be a pointer-to-pointer.

Re all of this *mut str is a fat pointer, because slices need to hold their length. To keep the length bundled up I used a String. There's probably a more elegant way to do it, but since it's testing code I wasn't going to spend too long optimizing it.

To expand a bit, you can totally get a c_void pointer from a String

Code

let boxed_str = s.into_boxed_str(); let raw_str_ptr: *mut str = Box::into_raw(boxed_str); let non_null = NonNull::new(raw_str_ptr).unwrap(); let ptr = non_null.cast::<c_void>();

but once you have a NonNull<c_void> I don't think you can recover it, because:

// ptr from above which is a NonNull<c_void> ptr.as_ptr() as *mut str

So you get into needing to basically recreate KernelStringSlice, since you need to store the pointer and the length separately. We could probably create a slice and leak a pointer to it, and then re-assemble, but just boxing and unboxing the string seems easier.

isn't legal: cannot cast thin pointer `*mut c_void` to wide pointer `*mut str`

Wait... str is actually a slice, not just deref to slice? That indeed poses a problem.

ffi/src/lib.rs

scovich · 2024-11-16T12:34:33Z

ffi/src/lib.rs

+        let p = Box::leak(b) as *mut String as *mut c_void;
+        let ptr = unsafe { NonNull::new_unchecked(p) };


nit:

Suggested change

let p = Box::leak(b) as *mut String as *mut c_void;

let ptr = unsafe { NonNull::new_unchecked(p) };

let ptr = Box::into_raw(b).cast();

let ptr = unsafe { NonNull::new_unchecked(ptr) };

scovich · 2024-11-16T12:49:09Z

ffi/src/lib.rs

+        let snapshot =
+            unsafe { ok_or_panic(snapshot(kernel_string_slice!(path), engine.clone_ptr())) };
+
+        let version = unsafe { version(snapshot.clone_ptr()) };
+        assert_eq!(version, 0);
+
+        let table_root = unsafe { snapshot_table_root(snapshot.clone_ptr(), allocate_str) };
+        assert!(table_root.is_some());
+        let s = recover_string(table_root.unwrap());
+        assert_eq!(&s, path);
+
+        unsafe { free_snapshot(snapshot) }


Interesting...

Functions like snapshot want to take an owned Handle<SharedExternEngine>, which means we can't "reuse" a handle from rust code. But the semantics are actually that dropping the handle (which will happen after calling snapshot) does not drop the underlying data, so if we call snapshot from "regular" rust code, we actually leak it.

Given the documented semantics of Handle (that it is valid until Handle::drop_handle is called), should we consider making Handle be Copy? Foreign code can anyway copy it freely, and is already responsible to properly enforce borrowed vs. mutable semantics (and Send+Sync) by not calling back into kernel from multiple threads simultaneously. Meanwhile, not being Copy gives no very little (**) protection to rust code because -- as both you and the docs note -- rust dropping the handle does not free the underlying memory.

(**) Not being Copy does grant some protection from double-free, because you can't call Handle::drop_handle twice... in the same function call. But that's only true for rust since handles are anyway effectively Copy from native code perspective. In particular, rust can't actually be sure a Handle has not been copied, if it was received from native code (tho that's an expected hazard of FFI being unsafe, I guess).

A safe alternative for shared handles is to call clone_handle and let the clone be consumed. But no such facility exists for exclusive handles.

yea and considering NonNull implements Copy this seems reasonable? we are just a pointer at that point?

Thinking more about this -- the actual target audience of this FFI API (e.g. C++ code) doesn't have the problem because handles are trivially copyable. So it's only rust code pretending to be FFI that needs the workaround. And for rust code, I suspect it's better to have an obvious function call to alert readers about what's going on, rather than an implicit copy?

yeah. i was working through this a bit, and I think, since we really just want it for testing atm, it makes sense to be a shallow_copy function call to make it really explicit

ffi/src/lib.rs

zachschuermann

I think this makes sense - definitely useful to mock the C(++) behavior in these tests, just left a couple comments on safety and implementing Copy

zachschuermann · 2024-11-21T19:44:45Z

ffi/src/handle.rs

+
+        /// In testing code we want to simulate what c code can do where a pointer can be used
+        /// without consuming it. This creates a "new" handle just by cloning the underlying
+        /// pointer. This is unsafe! Do not use outside testing code!


does this mean we should make it literally unsafe?

no, unsafe as a keyword means it calls unsafe code, this is "safe" in that sense. I will find a better word to use in the comment

"dangerous" or "highly error-prone"?

zachschuermann · 2024-11-21T19:47:33Z

ffi/src/lib.rs

+        let snapshot =
+            unsafe { ok_or_panic(snapshot(kernel_string_slice!(path), engine.clone_ptr())) };
+
+        let version = unsafe { version(snapshot.clone_ptr()) };
+        assert_eq!(version, 0);
+
+        let table_root = unsafe { snapshot_table_root(snapshot.clone_ptr(), allocate_str) };
+        assert!(table_root.is_some());
+        let s = recover_string(table_root.unwrap());
+        assert_eq!(&s, path);
+
+        unsafe { free_snapshot(snapshot) }


yea and considering NonNull implements Copy this seems reasonable? we are just a pointer at that point?

codecov · 2024-12-05T02:04:16Z

Codecov Report

Attention: Patch coverage is 97.10145% with 2 lines in your changes missing coverage. Please review.

Project coverage is 81.54%. Comparing base (cd31a07) to head (95e6153).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
ffi/src/lib.rs	96.77%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #503      +/-   ##
==========================================
+ Coverage   81.07%   81.54%   +0.46%     
==========================================
  Files          67       67              
  Lines       14496    14544      +48     
  Branches    14496    14544      +48     
==========================================
+ Hits        11753    11860     +107     
+ Misses       2170     2107      -63     
- Partials      573      577       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

scovich · 2024-12-05T19:46:37Z

@nicklan the last semver check came up clean -- you might try removing the label before your next push, to see if it gets re-added?

scovich · 2024-12-05T19:17:51Z

ffi/src/handle.rs

+
+        /// In testing code we want to simulate what c code can do where a pointer can be used
+        /// without consuming it. This creates a "new" handle just by cloning the underlying
+        /// pointer. This is unsafe! Do not use outside testing code!


"dangerous" or "highly error-prone"?

ffi/src/lib.rs

ffi/src/handle.rs

ffi/src/lib.rs

scovich · 2024-12-05T20:19:10Z

ffi/src/lib.rs

+        let snapshot =
+            unsafe { ok_or_panic(snapshot(kernel_string_slice!(path), engine.clone_ptr())) };
+
+        let version = unsafe { version(snapshot.clone_ptr()) };
+        assert_eq!(version, 0);
+
+        let table_root = unsafe { snapshot_table_root(snapshot.clone_ptr(), allocate_str) };
+        assert!(table_root.is_some());
+        let s = recover_string(table_root.unwrap());
+        assert_eq!(&s, path);
+
+        unsafe { free_snapshot(snapshot) }


Thinking more about this -- the actual target audience of this FFI API (e.g. C++ code) doesn't have the problem because handles are trivially copyable. So it's only rust code pretending to be FFI that needs the workaround. And for rust code, I suspect it's better to have an obvious function call to alert readers about what's going on, rather than an implicit copy?

zachschuermann

LGTM

scovich

LGTM!

ffi/src/handle.rs

ffi/src/lib.rs

nicklan added 2 commits November 15, 2024 13:43

checkpoint

ecd8e48

make it work

fde2251

github-actions bot assigned nicklan Nov 16, 2024

fmt + clippy

1e0d70d

github-actions bot added the breaking-change Change that require a major version bump label Nov 16, 2024

checking table_root

245404c

scovich reviewed Nov 16, 2024

View reviewed changes

zachschuermann reviewed Nov 21, 2024

View reviewed changes

nicklan added 3 commits December 4, 2024 17:55

cleaner allocate string

f0f7f6b

Merge branch 'main' into miri2

20c31c7

cleaner alloc/recover

5383538

nicklan force-pushed the miri2 branch from c9444be to 5383538 Compare December 5, 2024 02:00

scovich reviewed Dec 5, 2024

View reviewed changes

zachschuermann approved these changes Dec 5, 2024

View reviewed changes

nicklan removed the breaking-change Change that require a major version bump label Dec 5, 2024

comments + enable default-engine flag in miri ci test

051d972

nicklan marked this pull request as ready for review December 5, 2024 23:54

nicklan requested a review from scovich December 5, 2024 23:56

scovich approved these changes Dec 6, 2024

View reviewed changes

ffi/src/handle.rs Outdated Show resolved Hide resolved

ffi/src/lib.rs Outdated Show resolved Hide resolved

address nits

95e6153

nicklan merged commit 136a59e into delta-io:main Dec 6, 2024
20 checks passed

		let s: &str = unsafe { TryFromStringSlice::try_from_slice(&kernel_str).unwrap() };
		let b = Box::new(s.to_string());

		let p = Box::leak(b) as mut String as mut c_void;
		let ptr = unsafe { NonNull::new_unchecked(p) };

Uh oh!

Adding some more miri tests #503

Adding some more miri tests #503

Uh oh!

Conversation

nicklan commented Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes are proposed in this pull request?

Uh oh!

codecov bot commented Nov 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scovich Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

scovich commented Dec 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

scovich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nicklan commented Nov 16, 2024 •

edited

Loading

codecov bot commented Nov 16, 2024 •

edited

Loading

scovich Dec 5, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading