elf_parser fails loading a valid ELF file

[dmakarov]

The new elf parser breaks on a valid ELF input file. The file is attached in this issue.
sample.ZIP

[Lichtso]

It is invalid because it contains a section name longer than 16 bytes: .bss.__rust_no_alloc_shim_is_unstable
This should be easy to fix by merging all .bss* sections into one called just .bss in the linker script.

[dmakarov]

The error message reports invalid string. This has to be fixed. Also a test case must be added, that checks the correct error reporting.

[dmakarov]

Please don’t close issues that I open until I confirm that it is resolved.

[Lichtso]

Roger,

What other error message would you expect? It is an invalid string after all. Or do you want a separate StringTooLong error code?

[dmakarov]

It’s not an invalid string. in this case the error message must inform that a section name is over the length limit and indicate which section it is. The string that it breaks on is not even the one of the section name. Fix this.

[Lichtso]

Not saying it can't be done but it is tricky, hence we haven't implemented it yet.
You would have to retry the parsing with a much longer zero-termination-search but only on the error path, and also parse the UTF-8, both of which can fail leading to an exception during exception handling. Also, for the string in the error message you would need a heap allocation, which I just go rid of inside RBPF for the runtime (not parsing though).

[ksolana]

backtrace after applyhing the patch

diff --git a/tests/execution.rs b/tests/execution.rs
index bb2f9a9..5102e06 100644
--- a/tests/execution.rs
+++ b/tests/execution.rs
@@ -2206,6 +2206,17 @@ fn test_relative_call() {
     );
 }
 
+#[test]
+fn test_sample() {
+    test_interpreter_and_jit_elf!(
+        "tests/elfs/sample.so",
+        [1],
+        (),
+        TestContextObject::new(18),
+        ProgramResult::Ok(3),
+    );
+}
+

and cp sample.so tests/elfs/

---- test_sample stdout ----
thread 'test_sample' panicked at 'called `Result::unwrap()` on an `Err` value: ElfError(FailedToParse("invalid string"))', 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

[dmakarov]

Reporting a more meaningful error (not the obscured "invalid string") definitely doesn't require any of "parsing with a much longer zero-termination-search", "parse the UTF-8", or "heap allocation". Even if the error includes the first 16 characters of the section name, it would be sufficient (no heap allocation needed, even though using heap allocation as an excuse for improper error reporting is non-withstanding). Finally, the current tests do not test the elf_parser on any ELF files that would be rejected. This is simply unacceptable.