Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes I needed to get Siderophile working #17

Merged
merged 3 commits into from
Jan 3, 2020

Conversation

defuse
Copy link
Contributor

@defuse defuse commented Jul 31, 2019

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jul 31, 2019

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@disconnect3d disconnect3d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my question. Other than that, the changes looks good.

@@ -62,7 +62,7 @@ echo "trawling source code of dependencies for unsafety"

echo "generating LLVM bitcode for the callgraph"
cargo clean
RUSTFLAGS="-C lto=no -C opt-level=0 -C debuginfo=2 -C inline-threshold=9999 --emit=llvm-bc" \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason why we remove -C inline-threshold=9999?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rozbb btw why do we use inline threshold of 9999?

Copy link
Contributor Author

@defuse defuse Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it because it was causing functions to be inlined, so the generated call graph was missing all of the functions. (Setting it to 0 didn't work because that still allows one level of inlining). Here's the program I was trying to analyze:

use std::io::Cursor;
use byteorder::{BigEndian, ReadBytesExt};

pub fn main(v: Vec<u8>) {
    println!("Hello, world!");
    foobar(v);
}

pub fn foobar(v: Vec<u8>) {
    let mut rdr = Cursor::new(v);
    assert_eq!(0.01, rdr.read_f64::<BigEndian>().unwrap());
}

Without this change it got as far as realizing the read_f64 uses unsafe code and it added that to nodes_to_taint.txt, but the callgraph in unmangled_callgraph didn't show that main() calls foobar, so the final trace_unsafety.py step produced no useful result.

Copy link
Member

@disconnect3d disconnect3d Dec 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to reproduce this (by changing the example a bit e.g. assuming a proper fn main declaration) and confirmed (by reversing the produced binaries) that with the -C inline-threshold=9999 flag the foobar call got inlined into main and when the flag is not passed, the inlining did not happen.

Though, my questions now are:

  • Is it enough to remove the flag or maybe it is better to pass a value of 0? This grcov PR comment suggests that passing 0 should disable inlining while this Rust issue suggests that it might not be enough (I am not sure here)?
  • Why was this flag introduced in the first place? Maybe @rozbb can jump here :P.

Also for reference:

Copy link
Member

@disconnect3d disconnect3d Dec 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the rustc codegen-options specification, the opt-level=0 should also enable inline-threshold=0.

So when I don't pass the inline-threshold flag, the inlining doesn't happen as expected. If I pass it explicitly with 0 value, the inlining occurs. This is weird.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't think that setting the variable to 0 would work. I forgot why, but this seems like a good change.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I had no idea that -Cinline-threshold=0 allows one level of inlining. Does LLVM/rustc document that behavior anywhere?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allows one level of inlining

Does it? If you looked at the Rust lang docs I linked above, the N/A, only inlines always-inline functions is for the opt level of 0 and not necessarily the -Clinline-threshold=0 (which btw I think is a bit misleading).

Does LLVM/rustc document that behavior anywhere?

I couldn't find any other place except of the docs or rustc -C help.

Also there is -C llvm-args=-inline-threshold=0 too as in the Rust issue linked above. I don't even...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging this for now. We need proper CI/tests for such things, which I am going to add later on.

@disconnect3d disconnect3d merged commit 81e3461 into trailofbits:master Jan 3, 2020
disconnect3d added a commit that referenced this pull request Jan 3, 2020
This is related to the discussion of #17

We had to remove `-C inline-threshold=9999` from `RUSTFLAGS` we
pass to `cargo rustc ...` when emitting llvm. Otherwise, the compiler
inlined a function call and we didn't get proper results from
Siderophile. The test added in here checks if we find proper badness of
this particular example.
reaperhulk pushed a commit that referenced this pull request Jan 4, 2020
* Test 'inlining' case

This is related to the discussion of #17

We had to remove `-C inline-threshold=9999` from `RUSTFLAGS` we
pass to `cargo rustc ...` when emitting llvm. Otherwise, the compiler
inlined a function call and we didn't get proper results from
Siderophile. The test added in here checks if we find proper badness of
this particular example.

* Add librarycrate test

* Add test descriptions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants