Skip to content
This repository has been archived by the owner on Mar 22, 2024. It is now read-only.

run the afl-clang-fast instrumentation much earlier #19

Merged
merged 1 commit into from
Aug 30, 2019

Conversation

ddcc
Copy link
Contributor

@ddcc ddcc commented Aug 30, 2019

As of current trunk LLVM, the instrumentation no longer functions correctly because the IR may have already been simplified to remove explicit branches. For example, on this snippet of the sample test-instr.c file:

if (buf[0] == '0')

Clang 10.0.0 (trunk 370425) (llvm/trunk 370432) produces the following IR:

%cmp2 = icmp eq i8 %19, 48, !dbg !36
%.sink = select i1 %cmp2, i8* getelementptr inbounds ([26 x i8], [26 x i8]* @.str.1, i64 0, i64 0), i8* getelementptr inbounds ([31 x i8], [31 x i8]* @.str.2, i64 0, i64 0), !dbg !37
%call6 = tail call i32 (i32, i8*, ...) @__printf_chk(i32 1, i8* %.sink) #6, !dbg !38

Previously, Clang 8.0.0-3~ubuntu18.04.1 (tags/RELEASE_800/final) produced the following IR:

%29 = icmp eq i8 %28, 48, !dbg !37
br i1 %29, label %30, label %38, !dbg !38

As a workaround, run the instrumentation earlier, likely before e.g. the 'simplifycfg' pass removes explicit branches.

As of current trunk LLVM, the instrumentation no longer functions correctly
because the IR may have already been simplified to remove explicit branches.
For example, on this snippet of the sample test-instr.c file:

> if (buf[0] == '0')

Clang 10.0.0 (trunk 370425) (llvm/trunk 370432) produces the following IR:

%cmp2 = icmp eq i8 %19, 48, !dbg !36
%.sink = select i1 %cmp2, i8* getelementptr inbounds ([26 x i8], [26 x i8]* @.str.1, i64 0, i64 0), i8* getelementptr inbounds ([31 x i8], [31 x i8]* @.str.2, i64 0, i64 0), !dbg !37
%call6 = tail call i32 (i32, i8*, ...) @__printf_chk(i32 1, i8* %.sink) google#6, !dbg !38

Previously, Clang 8.0.0-3~ubuntu18.04.1 (tags/RELEASE_800/final) produced the following IR:

%29 = icmp eq i8 %28, 48, !dbg !37
br i1 %29, label %30, label %38, !dbg !38

This change causes the input-dependent branch of test-instr.c to be invisible
to the instrumentation, resulting in an error when testing the instrumentation
after compilation. As a workaround, run the instrumentation earlier, likely
before the 'simplifycfg' pass simplifies out the branch.
Copy link
Contributor

@jonathanmetzman jonathanmetzman left a comment

Choose a reason for hiding this comment

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

Awesome, big thanks for this patch. I confirmed it unbreaks clang-fast with trunk LLVM and that it doesn't break clang-6.

I wonder if there are negative consequences to running earlier in older versions of LLVM, but I don't think this possibility outweighs the benefits this brings.

@jonathanmetzman jonathanmetzman merged commit 2786bbb into google:master Aug 30, 2019
@ddcc
Copy link
Contributor Author

ddcc commented Aug 30, 2019

Yeah, this may also change previous behavior of the instrumentation with older versions of clang, but I don't see any drawback, since moving the instrumentation earlier should fix any similar bugs and improve net coverage.

FYI LLVM does seem to be moving to a new pass manager, and when enabled with -fexperimental-new-pass-manager, the instrumentation doesn't run at all. I haven't looked into it, but it might be problem in the future.

@jonathanmetzman
Copy link
Contributor

Yeah, this may also change previous behavior of the instrumentation with older versions of clang, but I don't see any drawback, since moving the instrumentation earlier should fix any similar bugs and improve net coverage.

ACK

FYI LLVM does seem to be moving to a new pass manager, and when enabled with -fexperimental-new-pass-manager, the instrumentation doesn't run at all. I haven't looked into it, but it might be problem in the future.

Thanks for the heads up, it's been causing headaches in libFuzzer land. I may add a travis test with a version of clang that it always near ToT so that we know when/if this breaks again.

Thanks again for these contributions!

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.

2 participants