elf.parse: resolve overflow issue in hash sections

[quake]

I found an overflow error when running the fuzz test (test case), this PR try to fix the issue by using wrapping operator.

[m4b]

Hmmm looks like macos CI is failing (for likely unrelated reasons ??)

[quake]

weird, trying revert commit and trigger the CI again

[quake]

Hmmm looks like macos CI is failing (for likely unrelated reasons ??)

I'm sure the CI error has nothing to do with this commit, I reverted the commit and got same error.

[m4b]

it looks like /Library/Developer/CommandLineTools/usr/bin/dyldinfo doesn't exist anymore, so that's the failure

[willglynn]

Ah, I believe that's moved from …/dyldinfo args to /usr/bin/xcrun dyldinfo args.

[m4b]

Oh you sweet, sweet cherub @willglynn , swooping in and saving the day; @quake this is annoying, but would you mind changing

goblin/tests/compare_dyldinfos.rs

Line 4 in e94ca6b

let apple = process::Command::new("/Library/Developer/CommandLineTools/usr/bin/dyldinfo")

to xcrun dyldinfo ?

[m4b]

actually it may be more complicated, maybe a separate PR is better

[quake]

I will create a separate PR to try xcrun, but I suspect that's not the causing, since test output reports:

cargo dyldinfo ["-lazy_bind", "-bind", "/Library/Developer/CommandLineTools/usr/bin/dyldinfo"] output:
err: BadMagic(3405691582)

and /Library/Developer/CommandLineTools/usr/bin/dyldinfo runs well.

it looks like the magic header of dyldinfo is 0xcafebabe (3405691582), and it's ignored in parse_magic_and_ctx fn.

[m4b]

Oh wow you're right; fwiw, it should probably be updated to not hard code anyway, since it can fail on other machines, something perhaps like:

diff --git a/tests/compare_dyldinfos.rs b/tests/compare_dyldinfos.rs
index 0c9dcaf..806e004 100644
--- a/tests/compare_dyldinfos.rs
+++ b/tests/compare_dyldinfos.rs
@@ -1,7 +1,19 @@
 use std::process;
 
+fn get_realpath(cmd: &str) -> String {
+    let output = process::Command::new("/usr/bin/xcrun")
+        .arg("-f")
+        .arg(cmd)
+        .output()
+        .expect("can get realpath");
+    std::str::from_utf8(&output.stdout)
+        .expect("output is valid utf8")
+        .to_string()
+}
+
 pub fn compare(args: Vec<&str>) {
-    let apple = process::Command::new("/Library/Developer/CommandLineTools/usr/bin/dyldinfo")
+    let apple = process::Command::new("/usr/bin/xcrun")
+        .arg("dyldinfo")
         .args(&args)
         .output()
         .expect("run Apple dyldinfo");
@@ -39,39 +51,28 @@ pub fn compare(args: Vec<&str>) {
 #[cfg(target_os = "macos")]
 #[test]
 fn compare_binds() {
-    compare(vec![
-        "-bind",
-        "/Library/Developer/CommandLineTools/usr/bin/dyldinfo",
-    ]);
-    compare(vec![
-        "-bind",
-        "/Library/Developer/CommandLineTools/usr/bin/clang",
-    ]);
+    let dyldinfo = get_realpath("dyldinfo");
+    let clang = get_realpath("dyldinfo");
+    compare(vec!["-bind", &dyldinfo]);
+    compare(vec!["-bind", &clang]);
     compare(vec!["-bind", "/usr/bin/tmutil"]);
 }
 
 #[cfg(target_os = "macos")]
 #[test]
 fn compare_lazy_binds() {
-    compare(vec![
-        "-lazy_bind",
-        "/Library/Developer/CommandLineTools/usr/bin/dyldinfo",
-    ]);
-    compare(vec![
-        "-lazy_bind",
-        "/Library/Developer/CommandLineTools/usr/bin/clang",
-    ]);
+    let dyldinfo = get_realpath("dyldinfo");
+    let clang = get_realpath("dyldinfo");
+    compare(vec!["-lazy_bind", &dyldinfo]);
+    compare(vec!["-lazy_bind", &clang]);
     compare(vec!["-lazy_bind", "/usr/bin/tmutil"]);
 }
 
 #[cfg(target_os = "macos")]
 #[test]
 fn compare_combined_options() {
-    compare(vec![
-        "-lazy_bind",
-        "-bind",
-        "/Library/Developer/CommandLineTools/usr/bin/dyldinfo",
-    ]);
+    let dyldinfo = get_realpath("dyldinfo");
+    compare(vec!["-lazy_bind", "-bind", &dyldinfo]);
 }
 
 #[cfg(not(target_os = "macos"))]

[m4b]

ok back to this PR; so what are the semantics if this overflows? This seems like a subtle bug now (I guess it was always present) Also, won't this only crash in debug mode, but will wrap overflow in release? Ditto for the other explicit overflow wraps

[quake]

I'm using wrapping_xxx here because I've seen other code do it too, for example,

goblin/src/mach/imports.rs

Line 234 in 2188699

let seg_offset = bind_info.seg_offset.wrapping_add(addr);

, semantically, maybe overflowing_xxx is better?

[m4b]

yes, i think your intuition is right, was kind of just wondering out loud what pros/cons are of the explict wrap, and what it means for the gnu hash table calculations to overflow (i just assume it means the binary is bad, better not to crash then i suppose?)

[tesuji]

Does gnu_hash_len calculate total size of GNU_HASH table?
Then performing some checks like

goblin/src/elf/gnu_hash.rs

Lines 116 to 135 in ece3ce6

	let hashtab = &hashtab[16..];
	{
	// SAFETY: Condition to check for an overflow
	// size_of(chains) + size_of(buckets) + size_of(bloom_filter) == size_of(hashtab)
	if dynsyms.len() <= symindex as usize {
	return Err("symindex must be smaller than dynsyms.len()");
	}
	let chains_size = (dynsyms.len() - symindex as usize).checked_mul(U32_SIZE);
	let buckets_size = (nbuckets as usize).checked_mul(U32_SIZE);
	let bloom_size = (maskwords as usize).checked_mul(INT_SIZE);
	let total_size = match (chains_size, buckets_size, bloom_size) {
	(Some(a), Some(b), Some(c)) => {
	a.checked_add(b).and_then(|t| t.checked_add(c))
	}
	_ => None,
	};
	match total_size {
	Some(size) if size == hashtab.len() => {}

is Ok.

[m4b]

@lzutao perhaps you have some thoughts on behavior of overflow wrapping, you did the work on the gnu hash rewrite

[m4b]

@quake are you still interested in getting this merged? let me know whenever you have a chance, just triaging old PRs here, thanks!