Safer FFI string slice handling

[scovich]

What changes are proposed in this pull request?

It is easy to create a use-after-free condition when creating kernel string slices from rust strings -- the raw pointers have no lifetime tracking that would allow the compiler to help. Thus, code like this would compile and produce a use-after-free at runtime:

expects_string_slice(KernelStringSlice::from(&String::from("free me!")))

The temporary String is freed as soon as KernelStringSlice::from returns, and the resulting KernelStringSlice is already invalid before expects_string_slice receives it. More pernicious cases may not involve any syntactically obvious borrowing:

expects_string_slice(String::from("free me!").as_str().into())

At that point, it's 100% up to the human to recognize the danger and avoid it by materializing the temporary string.

We can't "just" add an explicit lifetime parameter to KernelStringSlice, because lifetimes are meaningless when crossing the FFI boundary. Instead, we define a new kernel_string_slice macro, which only accepts identifiers as input:

// Fails to compile - macro rejects input
expects_string_slice(kernel_string_slice!(String::from("free me!").as_str()))

// First line fails to compile - the new string immediately goes out of scope
let s = String::from("free me!").as_str();
expects_string_slice(kernel_string_slice!(s))

// Compiles -- the new string stays in scope
let s = String::from("free me!");
expects_string_slice(kernel_string_slice!(s))

How does it work? Named inputs are guaranteed to at least live long enough to create the slice; any dangerous situations must arise from the subsequent use of that slice, such as returning it from the function or code block that owns the source memory (which results in a move that could over-extend the slice lifetime).

Note: There is nothing magical about the macro. It merely enforces the discipline that the input be named, which allows the compiler to verify all lifetimes leading up to the macro invocation.

While we're at it -- add an impl TryFromStringSlice for &str, which supports the common case where the caller doesn't need an owned string.

This PR affects the following public APIs

Removed the (very dangerous) impl From for KernelStringSlice, and added an unsafe constructor instead. Also restrict both the constructor and the macro to crate visibility for now. We can consider a public export if/when the need arises.

How was this change tested?

Refactor. Compilation + existing tests cover it.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 35 lines in your changes missing coverage. Please review.

Project coverage is 78.08%. Comparing base (40dc2b5) to head (12a8f04).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
ffi/src/lib.rs	0.00%	19 Missing ⚠️
ffi/src/expressions/kernel.rs	0.00%	7 Missing ⚠️
ffi/src/scan.rs	0.00%	4 Missing ⚠️
ffi/src/engine_funcs.rs	0.00%	3 Missing ⚠️
ffi/src/expressions/engine.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #441      +/-   ##
==========================================
- Coverage   78.16%   78.08%   -0.08%     
==========================================
  Files          55       55              
  Lines       11594    11605      +11     
  Branches    11594    11605      +11     
==========================================
  Hits         9062     9062              
- Misses       2032     2043      +11     
  Partials      500      500              

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

[nicklan]

the string slice stuff seems great.

I wonder about the warn!s rather than panics. We don't (currently) have a good way over FFI to enable logging, so it's likely that those logs will go nowhere and we'll just get unexpected behavior.

Crash an burn at least indicates you did something wrong, although breaking the engine isn't great.

[scovich]

I wonder about the warn!s rather than panics. We don't (currently) have a good way over FFI to enable logging, so it's likely that those logs will go nowhere and we'll just get unexpected behavior.

Crash an burn at least indicates you did something wrong, although breaking the engine isn't great.

It seems like we should just change the functions to return ExternResult then? Engine can swallow it if they really want, or handle it however (un)gracefully they want?

Update: Tracked as #446. I'll revert those changes from this PR.

[nicklan]

lgtm assuming we revert the warns for now.

[scovich]

lgtm assuming we revert the warns for now.

Good catch, I thought I had already done that!

[zachschuermann]

lgtm!