solana-labs / rbpf

Rust virtual machine and JIT compiler for eBPF programs
Apache License 2.0
277 stars 169 forks source link

Add error message when elf_parser fails on long section names #537

Closed ksolana closed 1 year ago

ksolana commented 1 year ago

Addresses: #532

With the patch error message now looks like:

failures:

---- test_sample stdout ----
thread 'test_sample' panicked at 'called `Result::unwrap()` on an `Err` value: ElfError(FailedToParse("Section/symbol name size `.bss.__rust_no_a` longer than `16`"))', tests/execution.rs:2211:5
stack backtrace:
   0: rust_begin_unwind
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/std/src/panicking.rs:593:5
   1: core::panicking::panic_fmt
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/panicking.rs:67:14
   2: core::result::unwrap_failed
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/result.rs:1651:5
   3: core::result::Result<T,E>::unwrap
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/result.rs:1076:23
   4: execution::test_sample
             at ./tests/execution.rs:2211:5
   5: execution::test_sample::{{closure}}
             at ./tests/execution.rs:2210:18
   6: core::ops::function::FnOnce::call_once
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/ops/function.rs:250:5
   7: core::ops::function::FnOnce::call_once
             at /rustc/d5c2e9c342b358556da91d61ed4133f6f50fc0c3/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
codecov-commenter commented 1 year ago

Codecov Report

Merging #537 (f1f0042) into main (32b1a6f) will decrease coverage by 0.01%. The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main     #537      +/-   ##
==========================================
- Coverage   88.41%   88.41%   -0.01%     
==========================================
  Files          24       24              
  Lines       10258    10264       +6     
==========================================
+ Hits         9070     9075       +5     
- Misses       1188     1189       +1     
Files Coverage Δ
src/elf_parser_glue.rs 68.53% <ø> (ø)
src/elf_parser/mod.rs 81.84% <85.71%> (+0.02%) :arrow_up:
ksolana commented 1 year ago

let me know if it is okay to merge the patch.

dmakarov commented 1 year ago

lgtm, but maybe confirm @Lichtso approves before merging.

Lichtso commented 1 year ago

Took the liberty to push the following changes:

dmakarov commented 1 year ago

Resolve #532

ksolana commented 1 year ago

Took the liberty to push the following changes:

* Used `String::from_utf8_lossy()` which uses replacement characters and can not fail, so we don't have to deal with UTF-8 errors.

* Moved `test_long_section_name` from execution.rs to elf.rs as it is about ELF parsing.

* Replaced `#[should_panic()]` with `assert_error!()` so we can programmatically cut the name to the desired length.

* Renamed `InvalidStringTooLong` to just `StringTooLong`. `InvalidString` can be removed because it is only used inside a debug formater.

Thanks! looks better now.