Re: Add fuzzing coverage support

classic Classic list List threaded Threaded
18 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Dmitry Vyukov
ping

Number of bugs found with this coverage in kernel already crossed 40:
https://github.com/google/syzkaller/wiki/Found-Bugs




On Fri, Nov 27, 2015 at 3:30 PM, Dmitry Vyukov <[hidden email]> wrote:

> +syzkaller group
>
> On Fri, Nov 27, 2015 at 3:28 PM, Dmitry Vyukov <[hidden email]> wrote:
>> Hello,
>>
>> This patch adds support for coverage-guided fuzzing:
>> https://codereview.appspot.com/280140043
>>
>> Coverage-guided fuzzing is a powerful randomized testing technique
>> that uses coverage feedback to determine new interesting inputs to a
>> program. Some examples of the systems that use it are AFL
>> (http://lcamtuf.coredump.cx/afl/) and LibFuzzer
>> (http://llvm.org/docs/LibFuzzer.html). Compiler coverage
>> instrumentation allows to make fuzzing more efficient as compared to
>> binary instrumentation.
>>
>> Flag that enables coverage is named -fsanitize-coverage=trace-pc to
>> make it consistent with similar functionality in clang, which already
>> supports a set of fuzzing coverage modes:
>> http://clang.llvm.org/docs/SanitizerCoverage.html
>> -fsanitize-coverage=trace-pc is not yet supported in clang, but we
>> plan to add it.
>>
>> This particular coverage mode simply inserts function calls into every
>> basic block.
>> I've built syzkaller, a Linux system call fuzzer
>> (https://github.com/google/syzkaller), using this functionality. The
>> fuzzer has found 30+ previously unknown bugs in kernel
>> (https://github.com/google/syzkaller/wiki/Found-Bugs) in slightly more
>> than a month (while kernel was extensively fuzzed with trinity --
>> non-guided fuzzer). Quentin also built some kernel fuzzer on top of
>> it.
>>
>> Why not gcov. Typical fuzzing loop looks as follows: (1) reset
>> coverage, (2) execute a bit of code, (3) collect coverage, repeat. A
>> typical coverage can be just a dozen of basic blocks (e.g. invalid
>> input). In such context gcov becomes prohibitively expensive as
>> reset/collect coverage steps depend on total number of basic
>> blocks/edges in program (in case of kernel it is about 2M). Cost of
>> this "tracing" mode depends on number of executed basic blocks/edges.
>> On top of that, kernel required per-thread coverage because there are
>> always background threads and unrelated processes that also produce
>> coverage. With inlined gcov instrumentation per-thread coverage is not
>> possible.
>> Inlined fast paths do not make lots of sense in fuzzing scenario,
>> because lots of code is executed just once in between resets. Also it
>> is not really possible to inline accesses to per-cpu and per-task data
>> structures for kernel.
>>
>> OK for trunk?
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Bernd Schmidt-2
On 12/02/2015 05:10 PM, Dmitry Vyukov wrote:
> ping

I do not see the original submission in my archives. This one comes too
late to make it into gcc-6. I can make some initial comments.

>>> This patch adds support for coverage-guided fuzzing:
>>> https://codereview.appspot.com/280140043

Please send patches with the mail (the best method is usually a
text/plain attachment) so that they can be quoted.

 From what I can tell this is relatively straight-forward. You'll want
to fix a number of issues with coding style (formatting, and lack of
function comments).

There's no documentation in invoke.texi.

We seem to have no established process for deciding whether we want a
new feature. I am not sure how to approach such a question, and it comes
up quite often. Somehow the default answer usually seems to be to accept
anything.


Bernd
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Dmitry Vyukov
On Wed, Dec 2, 2015 at 5:47 PM, Bernd Schmidt <[hidden email]> wrote:
> On 12/02/2015 05:10 PM, Dmitry Vyukov wrote:
>>
>> ping
>
>
> I do not see the original submission in my archives.

That's strange. I don't see it in gcc-patches archives as well.
The original email contained a plain-text patch attachment. Attaching it again.


> This one comes too late
> to make it into gcc-6. I can make some initial comments.
>
>>>> This patch adds support for coverage-guided fuzzing:
>>>> https://codereview.appspot.com/280140043
>
>
> Please send patches with the mail (the best method is usually a text/plain
> attachment) so that they can be quoted.
>
> From what I can tell this is relatively straight-forward. You'll want to fix
> a number of issues with coding style (formatting, and lack of function
> comments).
>
> There's no documentation in invoke.texi.
Will fix.
Can you point to some concrete coding style violations (besides
function comments)?


> We seem to have no established process for deciding whether we want a new
> feature. I am not sure how to approach such a question, and it comes up
> quite often. Somehow the default answer usually seems to be to accept
> anything.

sancov.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Bernd Schmidt-2
On 12/02/2015 05:55 PM, Dmitry Vyukov wrote:
> Can you point to some concrete coding style violations (besides
> function comments)?
>
>        (flag_sanitize & (SANITIZE_ADDRESS | SANITIZE_THREAD \
> - | SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT)))
> + | SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT) \
> + || flag_sanitize_coverage))

The || should line up with the other condition (i.e. the part starting
with flag_sanitize).

> +unsigned sancov_pass (function *fun)

Split the line after the return type.

> +
> +template<bool O0>
> +class pass_sancov : public gimple_opt_pass
> +{

This seems to be a new idiom but I find it OK. One thing to consider
would be whether you really need this split between O0/optimize
versions, or whether you can find a place in the queue where to insert
it unconditionally. Have you considered this at all or did you just
follow asan/tsan?

> +public:
> +  static pass_data pd ()
> +  {
> +    static const pass_data data =

I think a static data member would be better than the unnecessary pd ()
function. This is also unlike existing practice, and I wonder how others
think about it. IMO a fairly strong case could be made that if we're
using C++, then this sort of thing ought to be part of the class definition.


Bernd
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Dmitry Vyukov
On Wed, Dec 2, 2015 at 6:11 PM, Bernd Schmidt <[hidden email]> wrote:

> On 12/02/2015 05:55 PM, Dmitry Vyukov wrote:
>>
>> Can you point to some concrete coding style violations (besides
>> function comments)?
>>
>>               (flag_sanitize & (SANITIZE_ADDRESS | SANITIZE_THREAD \
>> -                               | SANITIZE_UNDEFINED |
>> SANITIZE_NONDEFAULT)))
>> +                               | SANITIZE_UNDEFINED |
>> SANITIZE_NONDEFAULT) \
>> +                       || flag_sanitize_coverage))
>
>
> The || should line up with the other condition (i.e. the part starting with
> flag_sanitize).
>
>> +unsigned sancov_pass (function *fun)
>
>
> Split the line after the return type.
>
>> +
>> +template<bool O0>
>> +class pass_sancov : public gimple_opt_pass
>> +{

Thanks for the review!


> This seems to be a new idiom but I find it OK.

Duplicating this code would double patch size :)
Asan pass has this duplication.
FWIW I found this template trick better than duplication of a large
chunk of code


> One thing to consider would
> be whether you really need this split between O0/optimize versions, or
> whether you can find a place in the queue where to insert it
> unconditionally. Have you considered this at all or did you just follow
> asan/tsan?

I inserted the pass just before asan/tsan because it looks like the
right place for it. If we do it after asan, it will insert coverage
for all asan-emited BBs which is highly undesirable. I also think it
is a good idea to run a bunch of optimizations before coverage pass to
not emit too many coverage callbacks (but I can't say that I am very
knowledgeable in this area). FWIW clang does the same: coverage passes
run just before asan/tsan.




>> +public:
>> +  static pass_data pd ()
>> +  {
>> +    static const pass_data data =
>
>
> I think a static data member would be better than the unnecessary pd ()
> function. This is also unlike existing practice, and I wonder how others
> think about it. IMO a fairly strong case could be made that if we're using
> C++, then this sort of thing ought to be part of the class definition.

I vary name of the pass depending on the O0 template argument (again
following asan):

    O0 ? "sancov_O0" : "sancov", /* name */

If we call it "sancov" always, then I can make it just a global var
(as all other passes in gcc).
Or I can make it a static variable of the template class and move
definition of the class (as you proposed).
What would you prefer?
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Jakub Jelinek
In reply to this post by Dmitry Vyukov
On Wed, Dec 02, 2015 at 05:55:29PM +0100, Dmitry Vyukov wrote:
> Can you point to some concrete coding style violations (besides
> function comments)?
>
>
> > We seem to have no established process for deciding whether we want a new
> > feature. I am not sure how to approach such a question, and it comes up
> > quite often. Somehow the default answer usually seems to be to accept
> > anything.

> Index: ChangeLog
> ===================================================================
> --- ChangeLog (revision 231000)
> +++ ChangeLog (working copy)
> @@ -1,3 +1,13 @@
> +2015-11-27  Dmitry Vyukov  <[hidden email]>
> +
> + * sancov.c: add file.
> + * Makefile.in: add sancov.c.
> + * passes.def: add sancov pass.
> + * tree-pass.h: add sancov pass.
> + * common.opt: add -fsanitize-coverage=trace-pc flag.
> + * sanitizer.def: add __sanitizer_cov_trace_pc builtin.
> + * builtins.def: enable sanitizer builtins when coverage is enabled.

All the ChangeLog entries should start with upper case letter after :,
instead of add file. say New file., in most other cases there is missing
() with what you've actually changed.
Say
        * Makefile.in (OBJS): Add sancov.o.
You should not add sancov.c to GTFILES if there are no GTY variables (and if
there were, you would need to include the generated gt-* header at the end).
        * builtins.def (DEF_SANITIZER_BUILTIN): Enable for
        flag_sanitize_coverage.
etc.

> --- builtins.def (revision 231000)
> +++ builtins.def (working copy)
> @@ -210,7 +210,8 @@
>    DEF_BUILTIN (ENUM, "__builtin_" NAME, BUILT_IN_NORMAL, TYPE, TYPE,    \
>         true, true, true, ATTRS, true, \
>        (flag_sanitize & (SANITIZE_ADDRESS | SANITIZE_THREAD \
> - | SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT)))
> + | SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT) \
> + || flag_sanitize_coverage))

The || should be indented below flag_.

> +  FOR_EACH_BB_FN (bb, fun)
> +    {
> +      gsi = gsi_start_bb (bb);
> +      stmt = gsi_stmt (gsi);
> +      while (stmt && dyn_cast <glabel *> (stmt))
> +        {
> +          gsi_next (&gsi);
> +          stmt = gsi_stmt (gsi);
> +        }
> +      if (!stmt)
> +        continue;

That would be
        gsi = gsi_after_labels (bb);
If you want to ignore empty basic blocks, then followed by
        if (gsi_end_p (gsi))
          continue;

> +      f = gimple_build_call (builtin_decl_implicit (BUILT_IN_SANCOV_TRACE), 0);

Generally, the text after BUILT_IN_ should be uppercase variant of the
function name, not a shorthand for it.
So I'd expect BUILT_IN_SANITIZER_COV_TRACE_PC instead.
If that makes the line too long, just use some temporary variable
say for builtin_decl_implicit result.

But I guess the most important question is, where is the
__sanitizer_cov_trace_pc function defined, I don't see it in libsanitizer
right now, is it meant for kernel only, something else?
If it is going to be in some libsanitizer library (which one), then one also
needs code to ensure that the library is linked in if -fsanitize-coverage
is used during linking.  Why is it not -fsanitize=coverage, btw?
Is it incompatible with some other sanitizers, or compatible with all of
them?

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Kostya Serebryany
On Wed, Dec 2, 2015 at 11:51 AM, Jakub Jelinek <[hidden email]> wrote:

> On Wed, Dec 02, 2015 at 05:55:29PM +0100, Dmitry Vyukov wrote:
>> Can you point to some concrete coding style violations (besides
>> function comments)?
>>
>>
>> > We seem to have no established process for deciding whether we want a new
>> > feature. I am not sure how to approach such a question, and it comes up
>> > quite often. Somehow the default answer usually seems to be to accept
>> > anything.
>
>> Index: ChangeLog
>> ===================================================================
>> --- ChangeLog (revision 231000)
>> +++ ChangeLog (working copy)
>> @@ -1,3 +1,13 @@
>> +2015-11-27  Dmitry Vyukov  <[hidden email]>
>> +
>> +     * sancov.c: add file.
>> +     * Makefile.in: add sancov.c.
>> +     * passes.def: add sancov pass.
>> +     * tree-pass.h: add sancov pass.
>> +     * common.opt: add -fsanitize-coverage=trace-pc flag.
>> +     * sanitizer.def: add __sanitizer_cov_trace_pc builtin.
>> +     * builtins.def: enable sanitizer builtins when coverage is enabled.
>
> All the ChangeLog entries should start with upper case letter after :,
> instead of add file. say New file., in most other cases there is missing
> () with what you've actually changed.
> Say
>         * Makefile.in (OBJS): Add sancov.o.
> You should not add sancov.c to GTFILES if there are no GTY variables (and if
> there were, you would need to include the generated gt-* header at the end).
>         * builtins.def (DEF_SANITIZER_BUILTIN): Enable for
>         flag_sanitize_coverage.
> etc.
>
>> --- builtins.def      (revision 231000)
>> +++ builtins.def      (working copy)
>> @@ -210,7 +210,8 @@
>>    DEF_BUILTIN (ENUM, "__builtin_" NAME, BUILT_IN_NORMAL, TYPE, TYPE,    \
>>              true, true, true, ATTRS, true, \
>>             (flag_sanitize & (SANITIZE_ADDRESS | SANITIZE_THREAD \
>> -                             | SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT)))
>> +                             | SANITIZE_UNDEFINED | SANITIZE_NONDEFAULT) \
>> +                     || flag_sanitize_coverage))
>
> The || should be indented below flag_.
>
>> +  FOR_EACH_BB_FN (bb, fun)
>> +    {
>> +      gsi = gsi_start_bb (bb);
>> +      stmt = gsi_stmt (gsi);
>> +      while (stmt && dyn_cast <glabel *> (stmt))
>> +        {
>> +          gsi_next (&gsi);
>> +          stmt = gsi_stmt (gsi);
>> +        }
>> +      if (!stmt)
>> +        continue;
>
> That would be
>         gsi = gsi_after_labels (bb);
> If you want to ignore empty basic blocks, then followed by
>         if (gsi_end_p (gsi))
>           continue;
>
>> +      f = gimple_build_call (builtin_decl_implicit (BUILT_IN_SANCOV_TRACE), 0);
>
> Generally, the text after BUILT_IN_ should be uppercase variant of the
> function name, not a shorthand for it.
> So I'd expect BUILT_IN_SANITIZER_COV_TRACE_PC instead.
> If that makes the line too long, just use some temporary variable
> say for builtin_decl_implicit result.
>
Probably I should answer these:
> But I guess the most important question is, where is the
> __sanitizer_cov_trace_pc function defined, I don't see it in libsanitizer
> right now, is it meant for kernel only, something else?

For the kernel, __sanitizer_cov_trace_pc will have to be in the kernel
itself -- we do not use any of our user-space run-times for the
kernel.
For the user space we may add it to
sanitizer_common/sanitizer_coverage_libcdep.cc,
however for userspace the benefit of having  __sanitizer_cov_trace_pc
is not so big because we already have __sanitizer_cov_trace_basic_block.
__sanitizer_cov_trace_basic_block has similar functionality but a bit
more hairy implementation in the compiler
and requires an initialization step done at module init, which in the
kernel we don't have.
(Dmitry, correct me if I am wrong).

> If it is going to be in some libsanitizer library (which one),
All of them, the coverage code is in sanitizer_common

> then one also
> needs code to ensure that the library is linked in if -fsanitize-coverage
> is used during linking.  Why is it not -fsanitize=coverage, btw?
> Is it incompatible with some other sanitizers, or compatible with all of
> them?

Partially for historical reasons, partially because Clang's
-fsanitize-coverage has many subflags
(today: func, bb, edge, indir-call, 8bit-counters, trace-cmp,
trace-bb) and having them as
-fsanitize=coverage-foo,coverage-bar,coverage-zoo would be more
verbose.

--kcc


>
>         Jakub
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Bernd Schmidt-2
In reply to this post by Dmitry Vyukov
On 12/02/2015 06:38 PM, Dmitry Vyukov wrote:

>> One thing to consider would
>> be whether you really need this split between O0/optimize versions, or
>> whether you can find a place in the queue where to insert it
>> unconditionally. Have you considered this at all or did you just follow
>> asan/tsan?
>
> I inserted the pass just before asan/tsan because it looks like the
> right place for it. If we do it after asan, it will insert coverage
> for all asan-emited BBs which is highly undesirable. I also think it
> is a good idea to run a bunch of optimizations before coverage pass to
> not emit too many coverage callbacks (but I can't say that I am very
> knowledgeable in this area). FWIW clang does the same: coverage passes
> run just before asan/tsan.

There's one other thing I want to put out there. Is this kind of thing
maybe what plugins were invented for? I don't really like the concept of
plugins, but it seems to me that this sort of thing might be an
application for them.

>>> +public:
>>> +  static pass_data pd ()
>>> +  {
>>> +    static const pass_data data =
>>
>>
>> I think a static data member would be better than the unnecessary pd ()
>> function. This is also unlike existing practice, and I wonder how others
>> think about it. IMO a fairly strong case could be made that if we're using
>> C++, then this sort of thing ought to be part of the class definition.
>
> I vary name of the pass depending on the O0 template argument (again
> following asan):
>
>      O0 ? "sancov_O0" : "sancov", /* name */
>
> If we call it "sancov" always, then I can make it just a global var
> (as all other passes in gcc).
> Or I can make it a static variable of the template class and move
> definition of the class (as you proposed).
> What would you prefer?

I think I prefer the static var of the template class. I just wonder why
we don't have the pass_data for all the existing passes as static data
members? I'm sure there's some reason.

asan also distinguishes the name between asan/asan0. I'd either follow
that naming convention, or remove the _O0 variant for all three of them.
I lean towards the latter.


Bernd
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Dmitry Vyukov
I've attached updated patch (also reuploaded
https://codereview.appspot.com/280140043).
Fixed ChangeLog.
Added invoke.texi.
Fixed style issues.

The function is defined only in kernel at the moment. Here is my patch:
https://github.com/dvyukov/linux/commit/f86eda0c895c47ea02ee37e981aeade7b03014d7
It is not mailed yet, for kernel asan people requested submit to gcc
first, then to kernel.

It will also be supported by libsanitizer later (Kostya?). But it is
not yet there.

Regarding plugins, we did tsan first as gcc plugin. It was difficult
to support, difficult to use, difficult to distribute. I maintain this
patch for a month, two people complained that it does not build
(because they synched to slightly different revisions).

sancov.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Dmitry Vyukov
On Thu, Dec 3, 2015 at 7:34 PM, Dmitry Vyukov <[hidden email]> wrote:

> I've attached updated patch (also reuploaded
> https://codereview.appspot.com/280140043).
> Fixed ChangeLog.
> Added invoke.texi.
> Fixed style issues.
>
> The function is defined only in kernel at the moment. Here is my patch:
> https://github.com/dvyukov/linux/commit/f86eda0c895c47ea02ee37e981aeade7b03014d7
> It is not mailed yet, for kernel asan people requested submit to gcc
> first, then to kernel.
>
> It will also be supported by libsanitizer later (Kostya?). But it is
> not yet there.
>
> Regarding plugins, we did tsan first as gcc plugin. It was difficult
> to support, difficult to use, difficult to distribute. I maintain this
> patch for a month, two people complained that it does not build
> (because they synched to slightly different revisions).

Added missing:
  stmt = gsi_stmt (gsi);
Now actually run tests and compiled kernel with it.

sancov.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Jakub Jelinek
Hi!

While this has been posted after stage1 closed and I'm not really happy
that it missed the deadline, I'm willing to grant an exception, the patch
is small enough that it is ok at this point of stage3.  That said, next time
please try to submit new features in time.

Are there any plans for GCC 7 for the other -fsanitize-coverage= options,
or are those just LLVM alternatives to GCC's gcov/-fprofile-generate etc.?

On Thu, Dec 03, 2015 at 08:17:06PM +0100, Dmitry Vyukov wrote:
> +unsigned sancov_pass (function *fun)

Formatting:
unsigned
sancov_pass (function *fun)

> +{
> +  basic_block bb;
> +  gimple_stmt_iterator gsi;
> +  gimple *stmt, *f;
> +  static bool inited;
> +
> +  if (!inited)
> +    {
> +      inited = true;
> +      initialize_sanitizer_builtins ();
> +    }

You can call this unconditionally, it will return as the first thing
if it is already initialized, no need for another guard.

> +
> +  /* Insert callback into beginning of every BB. */
> +  FOR_EACH_BB_FN (bb, fun)
> +    {
> +      gsi = gsi_after_labels (bb);
> +      if (gsi_end_p (gsi))
> +        continue;
> +      stmt = gsi_stmt (gsi);
> +      f = gimple_build_call (builtin_decl_implicit (
> +                             BUILT_IN_SANITIZER_COV_TRACE_PC), 0);

I (personally) prefer no ( at the end of line unless really needed.
In this case you can just do:
      tree fndecl = builtin_decl_implicit (BUILT_IN_SANITIZER_COV_TRACE_PC);
      gimple *g = gimple_build_call (fndecl, 0);
which is same number of lines, but looks nicer.
Also, please move also the gsi, stmt and f (better g or gcall)
declarations to the first assignment to them, they aren't used outside of
the loop.

> --- testsuite/gcc.dg/sancov/asan.c (revision 0)
> +++ testsuite/gcc.dg/sancov/asan.c (working copy)
> @@ -0,0 +1,21 @@
> +/* Test coverage/asan interaction:
> +     - coverage instruments __asan_init ctor (thus 4 covarage callbacks)
> +     - coverage does not instrument asan-emitted basic blocks
> +     - asan considers coverage callback as "nonfreeing" (thus 1 asan store
> +       callback.  */
> +/* { dg-do compile } */
> +/* { dg-options "-fsanitize-coverage=trace-pc -fsanitize=address" } */
> +
> +void notailcall ();
> +
> +void foo(volatile int *a, int *b)
> +{
> +  *a = 1;
> +  if (*b)
> +    *a = 2;
> +  notailcall ();
> +}
> +
> +/* { dg-final { scan-assembler-times "call __sanitizer_cov_trace_pc" 4 } } */
> +/* { dg-final { scan-assembler-times "call __asan_report_load4" 1 } } */
> +/* { dg-final { scan-assembler-times "call __asan_report_store4" 1 } } */

I don't like these, we have lots of targets, and different targets have
different instructions for making calls, different whitespace in between
the insn name and called function, sometimes some extra decoration on the fn
name, (say sometimes an extra _ prefix), etc.  IMHO much better to add
-fdump-tree-optimized and scan-tree-dump-times instead for the calls in the
optimized dump.  Affects all tests.

Please repost a patch with these changes fixed, it will be hopefully ackable
then.

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Yury Gribov
On 12/04/2015 04:41 PM, Jakub Jelinek wrote:

> Hi!
>
> While this has been posted after stage1 closed and I'm not really happy
> that it missed the deadline, I'm willing to grant an exception, the patch
> is small enough that it is ok at this point of stage3.  That said, next time
> please try to submit new features in time.
>
> Are there any plans for GCC 7 for the other -fsanitize-coverage= options,
> or are those just LLVM alternatives to GCC's gcov/-fprofile-generate etc.?
>
> On Thu, Dec 03, 2015 at 08:17:06PM +0100, Dmitry Vyukov wrote:
>> +unsigned sancov_pass (function *fun)
>
> Formatting:
> unsigned
> sancov_pass (function *fun)
>
>> +{
>> +  basic_block bb;
>> +  gimple_stmt_iterator gsi;
>> +  gimple *stmt, *f;
>> +  static bool inited;
>> +
>> +  if (!inited)
>> +    {
>> +      inited = true;
>> +      initialize_sanitizer_builtins ();
>> +    }
>
> You can call this unconditionally, it will return as the first thing
> if it is already initialized, no need for another guard.
>
>> +
>> +  /* Insert callback into beginning of every BB. */
>> +  FOR_EACH_BB_FN (bb, fun)
>> +    {
>> +      gsi = gsi_after_labels (bb);
>> +      if (gsi_end_p (gsi))
>> +        continue;
>> +      stmt = gsi_stmt (gsi);
>> +      f = gimple_build_call (builtin_decl_implicit (
>> +                             BUILT_IN_SANITIZER_COV_TRACE_PC), 0);
>
> I (personally) prefer no ( at the end of line unless really needed.
> In this case you can just do:
>        tree fndecl = builtin_decl_implicit (BUILT_IN_SANITIZER_COV_TRACE_PC);
>        gimple *g = gimple_build_call (fndecl, 0);
> which is same number of lines, but looks nicer.
> Also, please move also the gsi, stmt and f (better g or gcall)
> declarations to the first assignment to them, they aren't used outside of
> the loop.

Also FYI clang-format config has been recently added to contrib/
(https://gcc.gnu.org/ml/gcc-patches/2015-11/msg02214.html).

>
>> --- testsuite/gcc.dg/sancov/asan.c (revision 0)
>> +++ testsuite/gcc.dg/sancov/asan.c (working copy)
>> @@ -0,0 +1,21 @@
>> +/* Test coverage/asan interaction:
>> +     - coverage instruments __asan_init ctor (thus 4 covarage callbacks)
>> +     - coverage does not instrument asan-emitted basic blocks
>> +     - asan considers coverage callback as "nonfreeing" (thus 1 asan store
>> +       callback.  */
>> +/* { dg-do compile } */
>> +/* { dg-options "-fsanitize-coverage=trace-pc -fsanitize=address" } */
>> +
>> +void notailcall ();
>> +
>> +void foo(volatile int *a, int *b)
>> +{
>> +  *a = 1;
>> +  if (*b)
>> +    *a = 2;
>> +  notailcall ();
>> +}
>> +
>> +/* { dg-final { scan-assembler-times "call __sanitizer_cov_trace_pc" 4 } } */
>> +/* { dg-final { scan-assembler-times "call __asan_report_load4" 1 } } */
>> +/* { dg-final { scan-assembler-times "call __asan_report_store4" 1 } } */
>
> I don't like these, we have lots of targets, and different targets have
> different instructions for making calls, different whitespace in between
> the insn name and called function, sometimes some extra decoration on the fn
> name, (say sometimes an extra _ prefix), etc.  IMHO much better to add
> -fdump-tree-optimized and scan-tree-dump-times instead for the calls in the
> optimized dump.  Affects all tests.
>
> Please repost a patch with these changes fixed, it will be hopefully ackable
> then.
>
> Jakub
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Dmitry Vyukov
In reply to this post by Jakub Jelinek
On Fri, Dec 4, 2015 at 2:41 PM, Jakub Jelinek <[hidden email]> wrote:
> Hi!
>
> While this has been posted after stage1 closed and I'm not really happy
> that it missed the deadline, I'm willing to grant an exception, the patch
> is small enough that it is ok at this point of stage3.  That said, next time
> please try to submit new features in time.

Sorry.
Thanks!

> Are there any plans for GCC 7 for the other -fsanitize-coverage= options,
> or are those just LLVM alternatives to GCC's gcov/-fprofile-generate etc.?

No, they are not alternatives to gcov. Other coverage modes are backed
by sanitizer_common runtime and libFuzzer, which together allow to do
efficient in-process fuzzing.
We don't have plans to port other options at the moment per se.
Though, that's doable and we could sync sanitizer library for runtime
support.


> On Thu, Dec 03, 2015 at 08:17:06PM +0100, Dmitry Vyukov wrote:
>> +unsigned sancov_pass (function *fun)
>
> Formatting:
> unsigned
> sancov_pass (function *fun)

Missed this. Done.

>> +{
>> +  basic_block bb;
>> +  gimple_stmt_iterator gsi;
>> +  gimple *stmt, *f;
>> +  static bool inited;
>> +
>> +  if (!inited)
>> +    {
>> +      inited = true;
>> +      initialize_sanitizer_builtins ();
>> +    }
>
> You can call this unconditionally, it will return as the first thing
> if it is already initialized, no need for another guard.
Done

>> +
>> +  /* Insert callback into beginning of every BB. */
>> +  FOR_EACH_BB_FN (bb, fun)
>> +    {
>> +      gsi = gsi_after_labels (bb);
>> +      if (gsi_end_p (gsi))
>> +        continue;
>> +      stmt = gsi_stmt (gsi);
>> +      f = gimple_build_call (builtin_decl_implicit (
>> +                             BUILT_IN_SANITIZER_COV_TRACE_PC), 0);
>
> I (personally) prefer no ( at the end of line unless really needed.
> In this case you can just do:
>       tree fndecl = builtin_decl_implicit (BUILT_IN_SANITIZER_COV_TRACE_PC);
>       gimple *g = gimple_build_call (fndecl, 0);
> which is same number of lines, but looks nicer.
> Also, please move also the gsi, stmt and f (better g or gcall)
> declarations to the first assignment to them, they aren't used outside of
> the loop.
Done

>> --- testsuite/gcc.dg/sancov/asan.c    (revision 0)
>> +++ testsuite/gcc.dg/sancov/asan.c    (working copy)
>> @@ -0,0 +1,21 @@
>> +/* Test coverage/asan interaction:
>> +     - coverage instruments __asan_init ctor (thus 4 covarage callbacks)
>> +     - coverage does not instrument asan-emitted basic blocks
>> +     - asan considers coverage callback as "nonfreeing" (thus 1 asan store
>> +       callback.  */
>> +/* { dg-do compile } */
>> +/* { dg-options "-fsanitize-coverage=trace-pc -fsanitize=address" } */
>> +
>> +void notailcall ();
>> +
>> +void foo(volatile int *a, int *b)
>> +{
>> +  *a = 1;
>> +  if (*b)
>> +    *a = 2;
>> +  notailcall ();
>> +}
>> +
>> +/* { dg-final { scan-assembler-times "call   __sanitizer_cov_trace_pc" 4 } } */
>> +/* { dg-final { scan-assembler-times "call   __asan_report_load4" 1 } } */
>> +/* { dg-final { scan-assembler-times "call   __asan_report_store4" 1 } } */
>
> I don't like these, we have lots of targets, and different targets have
> different instructions for making calls, different whitespace in between
> the insn name and called function, sometimes some extra decoration on the fn
> name, (say sometimes an extra _ prefix), etc.  IMHO much better to add
> -fdump-tree-optimized and scan-tree-dump-times instead for the calls in the
> optimized dump.  Affects all tests.
Done
Much better now.

> Please repost a patch with these changes fixed, it will be hopefully ackable
> then.


New patch is attached.
Code review is updated:
https://codereview.appspot.com/280140043
Test are passing. Also compiled and booted kernel with this.
Also applied clang-format to sancov.c.

Please take another look.

sancov.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Dmitry Vyukov
In reply to this post by Yury Gribov
On Fri, Dec 4, 2015 at 2:45 PM, Yury Gribov <[hidden email]> wrote:

> On 12/04/2015 04:41 PM, Jakub Jelinek wrote:
>>
>> Hi!
>>
>> While this has been posted after stage1 closed and I'm not really happy
>> that it missed the deadline, I'm willing to grant an exception, the patch
>> is small enough that it is ok at this point of stage3.  That said, next
>> time
>> please try to submit new features in time.
>>
>> Are there any plans for GCC 7 for the other -fsanitize-coverage= options,
>> or are those just LLVM alternatives to GCC's gcov/-fprofile-generate etc.?
>>
>> On Thu, Dec 03, 2015 at 08:17:06PM +0100, Dmitry Vyukov wrote:
>>>
>>> +unsigned sancov_pass (function *fun)
>>
>>
>> Formatting:
>> unsigned
>> sancov_pass (function *fun)
>>
>>> +{
>>> +  basic_block bb;
>>> +  gimple_stmt_iterator gsi;
>>> +  gimple *stmt, *f;
>>> +  static bool inited;
>>> +
>>> +  if (!inited)
>>> +    {
>>> +      inited = true;
>>> +      initialize_sanitizer_builtins ();
>>> +    }
>>
>>
>> You can call this unconditionally, it will return as the first thing
>> if it is already initialized, no need for another guard.
>>
>>> +
>>> +  /* Insert callback into beginning of every BB. */
>>> +  FOR_EACH_BB_FN (bb, fun)
>>> +    {
>>> +      gsi = gsi_after_labels (bb);
>>> +      if (gsi_end_p (gsi))
>>> +        continue;
>>> +      stmt = gsi_stmt (gsi);
>>> +      f = gimple_build_call (builtin_decl_implicit (
>>> +                             BUILT_IN_SANITIZER_COV_TRACE_PC), 0);
>>
>>
>> I (personally) prefer no ( at the end of line unless really needed.
>> In this case you can just do:
>>        tree fndecl = builtin_decl_implicit
>> (BUILT_IN_SANITIZER_COV_TRACE_PC);
>>        gimple *g = gimple_build_call (fndecl, 0);
>> which is same number of lines, but looks nicer.
>> Also, please move also the gsi, stmt and f (better g or gcall)
>> declarations to the first assignment to them, they aren't used outside of
>> the loop.
>
>
> Also FYI clang-format config has been recently added to contrib/
> (https://gcc.gnu.org/ml/gcc-patches/2015-11/msg02214.html).

Rock-n-roll!

Applied to sancov.c
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Jakub Jelinek
In reply to this post by Dmitry Vyukov
On Fri, Dec 04, 2015 at 06:32:38PM +0100, Dmitry Vyukov wrote:

> +2015-12-04  Dmitry Vyukov  <[hidden email]>
> +
> + * sancov.c: New file.
> + * Makefile.in (OBJS): Add sancov.o.
> + * invoke.texi (-fsanitize-coverage=trace-pc): Describe.
> + * passes.def (sancov_pass): Add.
> + * tree-pass.h  (sancov_pass): Add.
> + * common.opt (-fsanitize-coverage=trace-pc): Add.
> + * sanitizer.def (BUILT_IN_SANITIZER_COV_TRACE_PC): Add.
> + * builtins.def (DEF_SANITIZER_BUILTIN): Enable for
> + flag_sanitize_coverage.

This is ok for trunk.

        Jakub
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Dmitry Vyukov
On Fri, Dec 4, 2015 at 6:39 PM, Jakub Jelinek <[hidden email]> wrote:

> On Fri, Dec 04, 2015 at 06:32:38PM +0100, Dmitry Vyukov wrote:
>> +2015-12-04  Dmitry Vyukov  <[hidden email]>
>> +
>> +     * sancov.c: New file.
>> +     * Makefile.in (OBJS): Add sancov.o.
>> +     * invoke.texi (-fsanitize-coverage=trace-pc): Describe.
>> +     * passes.def (sancov_pass): Add.
>> +     * tree-pass.h  (sancov_pass): Add.
>> +     * common.opt (-fsanitize-coverage=trace-pc): Add.
>> +     * sanitizer.def (BUILT_IN_SANITIZER_COV_TRACE_PC): Add.
>> +     * builtins.def (DEF_SANITIZER_BUILTIN): Enable for
>> +     flag_sanitize_coverage.
>
> This is ok for trunk.


Committed as 231296
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Nathan Sidwell-2
On 12/04/15 13:28, Dmitry Vyukov wrote:

> On Fri, Dec 4, 2015 at 6:39 PM, Jakub Jelinek <[hidden email]> wrote:
>> On Fri, Dec 04, 2015 at 06:32:38PM +0100, Dmitry Vyukov wrote:
>>> +2015-12-04  Dmitry Vyukov  <[hidden email]>
>>> +
>>> +     * sancov.c: New file.
>>> +     * Makefile.in (OBJS): Add sancov.o.
>>> +     * invoke.texi (-fsanitize-coverage=trace-pc): Describe.
>>> +     * passes.def (sancov_pass): Add.
>>> +     * tree-pass.h  (sancov_pass): Add.
>>> +     * common.opt (-fsanitize-coverage=trace-pc): Add.
>>> +     * sanitizer.def (BUILT_IN_SANITIZER_COV_TRACE_PC): Add.
>>> +     * builtins.def (DEF_SANITIZER_BUILTIN): Enable for
>>> +     flag_sanitize_coverage.
>>
>> This is ok for trunk.
>
>
> Committed as 231296

This seems to have changed the testsuite (gcc.dg/sancov) without record in
testsuite/ChangeLog.  Further, the tests presume sanitizer coverage is
implemented.  I'm seeing asan.c fail with:

cc1: warning: -fsanitize=address and -fsanitize=kernel-address are not supported
for this target

cc1: warning: -fsanitize=address not supported for this target

output is:
cc1: warning: -fsanitize=address and -fsanitize=kernel-address are not supported
for this target

cc1: warning: -fsanitize=address not supported for this target


FAIL: gcc.dg/sancov/asan.c   -O0  (test for excess errors)
Reply | Threaded
Open this post in threaded view
|

Re: Add fuzzing coverage support

Dmitry Vyukov
On Sat, Dec 5, 2015 at 1:54 AM, Nathan Sidwell <[hidden email]> wrote:

> On 12/04/15 13:28, Dmitry Vyukov wrote:
>>
>> On Fri, Dec 4, 2015 at 6:39 PM, Jakub Jelinek <[hidden email]> wrote:
>>>
>>> On Fri, Dec 04, 2015 at 06:32:38PM +0100, Dmitry Vyukov wrote:
>>>>
>>>> +2015-12-04  Dmitry Vyukov  <[hidden email]>
>>>> +
>>>> +     * sancov.c: New file.
>>>> +     * Makefile.in (OBJS): Add sancov.o.
>>>> +     * invoke.texi (-fsanitize-coverage=trace-pc): Describe.
>>>> +     * passes.def (sancov_pass): Add.
>>>> +     * tree-pass.h  (sancov_pass): Add.
>>>> +     * common.opt (-fsanitize-coverage=trace-pc): Add.
>>>> +     * sanitizer.def (BUILT_IN_SANITIZER_COV_TRACE_PC): Add.
>>>> +     * builtins.def (DEF_SANITIZER_BUILTIN): Enable for
>>>> +     flag_sanitize_coverage.
>>>
>>>
>>> This is ok for trunk.
>>
>>
>>
>> Committed as 231296
>
>
> This seems to have changed the testsuite (gcc.dg/sancov) without record in
> testsuite/ChangeLog.  Further, the tests presume sanitizer coverage is
> implemented.  I'm seeing asan.c fail with:
>
> cc1: warning: -fsanitize=address and -fsanitize=kernel-address are not
> supported for this target
>
> cc1: warning: -fsanitize=address not supported for this target
>
> output is:
> cc1: warning: -fsanitize=address and -fsanitize=kernel-address are not
> supported for this target
>
> cc1: warning: -fsanitize=address not supported for this target
>
>
> FAIL: gcc.dg/sancov/asan.c   -O0  (test for excess errors)


Mailed a fix titled "[PATCH] Fix new sancov tests".
Sorry for the breakage.