bpf-sys, handle when `/proc/version_signature` is empty

[QuiiBz]

Hello, I tried to compile my BPF program on a system that's running proxmox, but strangely the /proc/version_signature is empty - it seems like it's due to /boot/config-5.13.19-4-pve having an empty CONFIG_VERSION_SIGNATURE="".

Here's the exact error when running my loader:

thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /root/.cargo/registry/src/github.com-1ecc6299db9ec823/redbpf-2.3.0/src/lib.rs:2765:54
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

which refers to here. As I can see, the parse_version_signature doesn't handle the fact that /proc/version_signature can be empty, and so return a None.

Would you like me to create a PR to fix this issue, or do you need any more information? Thanks.

[nbaksalyar]

That's a good catch. I think it would make sense to get rid of this unwrap in fn get_version in addition to handling the case of an empty string in parse_version_signature.

get_version is used only in ModuleBuilder::parse, so this line can be easily replaced with something like get_version(&content).ok_or(Error::InvalidVersion)?.

[QuiiBz]

Would I still be able to run the loader with your proposition? Looks like the version is used at other spots.

I'm thinking about checking in get_kernel_internal_version if the file is empty or not before calling parse_version_signature, to fallback to uname(). I think it's doesn't make a lot of sense to parse the version from a file that is empty 🤔 .

[nbaksalyar]

checking in get_kernel_internal_version if the file is empty or not before calling parse_version_signature

Sure, but fs::read_to_string should take care of that for you because there's no data associated with inode - so checking if a file is empty would be superfluous. So I would just do something like

let mut version = None;
if let Ok(version) = fs::read_to_string("/proc/version_signature") {
    version = parse_version_signature(&version.trim())?
}
if version.is_none() {
    version = to_str(&uname().ok()?.release).into()
}
parse_version(&version).map(|(major, minor, patch)| major << 16 | minor << 8 | patch)

Would I still be able to run the loader with your proposition?

I think so - in this case the get_kernel_internal_version call should fall back to uname() and if that fails then it's a legit error which is better conveyed through returning an Error variant instead of panicking through unwrap().

[QuiiBz]

Looks great, should I create a PR for this fix?

[nbaksalyar]

should I create a PR for this fix?

Sure, it would be very appreciated! :)

[QuiiBz]

Created the PR #322, with a slightly modified version of your answer.

[QuiiBz]

Closing since merged with #322, thanks!