Skip to content
This repository has been archived by the owner on Jan 10, 2025. It is now read-only.

Remove unreachable BssNotSupported error #462

Merged

Conversation

ripatel-fd
Copy link

@ripatel-fd ripatel-fd commented May 2, 2023

Minor find while porting the ELF loader:

The second branch of the if expression in the writable section check is dead code.

We already check for name.starts_with(".bss") || ? so name == ".bss" is unreachable.

rbpf/src/elf.rs

Lines 690 to 697 in e9c2d20

if name.starts_with(".bss")
|| (section_header.is_writable()
&& (name.starts_with(".data") && !name.starts_with(".data.rel")))
{
return Err(ElfError::WritableSectionNotSupported(name.to_owned()));
} else if name == ".bss" {
return Err(ElfError::BssNotSupported);
}

The unit test for this piece of code confirms this:

rbpf/src/elf.rs

Lines 2114 to 2119 in e9c2d20

#[test]
#[should_panic(expected = r#"validation failed: WritableSectionNotSupported(".bss")"#)]
fn test_bss_section() {
let elf_bytes =
std::fs::read("tests/elfs/bss_section.so").expect("failed to read elf file");
ElfExecutable::load(&elf_bytes, loader()).expect("validation failed");

Related to that, we should probably add a unit test for an ELF that has a read-only .bss section.

@Lichtso
Copy link

Lichtso commented May 10, 2023

Can you rebase onto the main branch?

@ripatel-fd ripatel-fd force-pushed the ripatel/remove-bss-not-supported branch from b4cbf46 to f61daa6 Compare May 10, 2023 14:43
@codecov-commenter
Copy link

Codecov Report

Merging #462 (f61daa6) into main (7a87580) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #462   +/-   ##
=======================================
  Coverage   89.83%   89.84%           
=======================================
  Files          23       23           
  Lines        9948     9946    -2     
=======================================
- Hits         8937     8936    -1     
+ Misses       1011     1010    -1     
Impacted Files Coverage Δ
src/elf.rs 89.18% <ø> (+0.05%) ⬆️

@Lichtso Lichtso merged commit 527add7 into solana-labs:main May 10, 2023
@ripatel-fd ripatel-fd deleted the ripatel/remove-bss-not-supported branch May 12, 2023 16:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants