add a native C demangler #75
Conversation
|
How does this compare with https://github.com/LykenSol/rust-demangle.c? |
I think that being fuzzed to be equivalent to the Rust implementation rather than having specific test vectors makes it more maintainable. |
|
Personally, I think that having the demangler fuzzable against the Rust one makes me much more comfortable putting it in the rust-lang org. |
|
Fyi 
|
| /// Write the string in a `struct demangle` into a buffer. | ||
| /// | ||
| /// Return `OverflowOk` if the output buffer was sufficiently big, `OverflowOverflow` if it wasn't. | ||
| /// This function is `O(n)` in the length of the input + *output* [$], but the demangled output of demangling a symbol can |
There was a problem hiding this comment.
IIUC, output here refers specifically to the produced demangling, not the output buffer's size, right? We don't do anything with unused bytes in the output buffer.
(Just to check my understanding, no changes needed here I think).
There was a problem hiding this comment.
Yea, "output-sensitive algorithm"
crates/native-c/README
Outdated
| @@ -0,0 +1,5 @@ | |||
| A portable native C demangler, which should mostly have byte-for-byte identical outputs to the Rust one. | |||
There was a problem hiding this comment.
It might be worth noting what our expectations on safety are (e.g., do we fuzz etc sufficiently that we feel confident there's no UB on untrusted inputs)? And maybe whether we also fail in the same cases as the Rust one.
crates/native-c/src/build.rs
Outdated
There was a problem hiding this comment.
Accidentally added file?
fuzz/fuzz_targets/native_c.rs
Outdated
| } | ||
|
|
||
| fuzz_target!(|data: &[u8]| { | ||
| if data.len() == 0 { |
There was a problem hiding this comment.
Can we add a test to make sure an empty input is OK too?
fuzz/fuzz_targets/native_c.rs
Outdated
| match rustc_demangle_native_c::rust_demangle_display_demangle( | ||
| &demangle, | ||
| buf.as_mut_ptr().cast(), | ||
| buf.len() / 4, |
| if rust_overflowed.is_err() { | ||
| return; // rust overflowed as well, OK | ||
| } | ||
| // call C again with larger buffer. If it fits in an 1020-byte Rust buffer, it will fit in a 4096-byte C buffer |
There was a problem hiding this comment.
I'm not sure I understand why we have this special cased -- it feels like we ought to catch discrepancies of output with an assert on equivalent outputs; we don't really need differently sized buffers to catch that?
If we want to assert the overflow handling is correctly implemented maybe having a unit test that confirms output sizes less than the required output size all fail (e.g., 0..4kb for a 4kb output or so). Probably worth a test with zero-sized output buffer as well.
There was a problem hiding this comment.
If we want to assert the overflow handling is correctly implemented maybe having a unit test that confirms output sizes less than the required output size all fail (e.g., 0..4kb for a 4kb output or so). Probably worth a test with zero-sized output buffer as well.
I'll add a unit test for that as well, but the fuzzing helps too
There was a problem hiding this comment.
It would probably also make sense to update the top-level README noting this exists and indicating that expected usage is probably copy/paste or git submodule?
4 participants
Adds a native C demangler to allow users with a build system that does not support Rust to demangle Rust symbols.
There are fuzz tests that check output compatibility between the native C demangler and the Rust demangler.