Never return a section size smaller than virtual size

[dureuill]

See the associated issue (#291) for details.

This PR does the following:

1. Introduce a subtraction that was present in the stackexchange post linked in the source
2. Replace the min by a max so that the section size is at least as big as the virtual size. Note that this deviates from the linked stackexchange post. However, it does fix the associated issue for the tested binaries.

Tests

Manual tests with the following test driver (using symbolic, that uses goblin):

# Cargo.toml
[package]
name = "symbolic_symbols"
version = "0.1.0"
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
symbolic = "*"
anyhow = "1.0"
env_logger = "*"


[patch.crates-io]
goblin = { path = "goblin" }

// src/main.rs
use std::ops::Deref;

use anyhow::Context;
use symbolic::debuginfo::Object;

fn main() -> anyhow::Result<()> {
    env_logger::init();
    let data = std::fs::read(std::env::args_os().nth(1).context("Expected path to binary")?)?;
    let object = Object::parse(data.deref())?;
    for symbol in object.symbols() {
        println!("{}", symbol.name().unwrap());
    }
    Ok(())
}

On the two tests binaries: libxml2.dll and
VBoxMRXNP.dll + quine.exe from #101.

Compared with rabin2 -E $PE.

1. Before this PR, not the same number of symbols, quine.exe loads successfully (with 0 symbols)
2. After this PR, same number of symbols as rabin2, quine.exe still loads successfully (still with 0 symbols)

Thank you for your time and consideration!

[philipc]

The first commit looks good.

The second commit is wrong. This function is only intended to return the number of bytes of data that are actually in the file data, even if the virtual size is larger than that.

The problem you are seeing with the exports is in the caller. This code in Export::try_from_ctx expects that every export address will have a corresponding file offset, but this is not a valid assumption. I think fixing that will require changing Export::offset to Option<usize>, which is a breaking change.

[dureuill]

I see. Thank you for the answer. I'm not sure how to move forward. Would such a breaking change be possible for goblin?

Meanwhile, I will close this PR, as it is wrong. At least, it prompted your answer 😅. The associated issue remains open #291.

[philipc]

I think a breaking change is okay if there is no alternative, and I don't think there is, so feel free to open a PR with that change (or reopen this PR after a force push).

[dureuill]

Thank you. As I had already force pushed my branch, GitHub does not let me continue in this PR, so I opened #293.