[Bug sanitizer/61547] New: Invalid sanitization of trailing byte in __builtin_strlen

[Bug sanitizer/61547] New: Invalid sanitization of trailing byte in __builtin_strlen

[y.gribov at samsung dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61547

            Bug ID: 61547
           Summary: Invalid sanitization of trailing byte in
                    __builtin_strlen
           Product: gcc
           Version: 4.10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: y.gribov at samsung dot com
                CC: dodji at gcc dot gnu.org, dvyukov at gcc dot gnu.org,
                    jakub at gcc dot gnu.org, kcc at gcc dot gnu.org

Created attachment 32963
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=32963&action=edit
Reprocase

The attached testcase performs an overflow in strlen call. Current GCC fails to
detect it because of invalid instrumentation of trailing byte:
$ gcc repro.c -fsanitize=address -O1 -static-libasan
$ ./a.out
$ echo $?
0

[Bug sanitizer/61547] Invalid sanitization of trailing byte in __builtin_strlen

[y.gribov at samsung dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61547

--- Comment #1 from Yury Gribov <y.gribov at samsung dot com> ---
Created attachment 32964
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=32964&action=edit
Proposed patch

Adding draft patch. I only ran Asan regression tests though (leaving for long
vacation today). Perhaps someone will have time to have a look at this.

[Bug sanitizer/61547] Invalid sanitization of trailing byte in __builtin_strlen

[kcc at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61547

--- Comment #2 from Kostya Serebryany <kcc at gcc dot gnu.org> ---
Note that in clang we chose not to instrument any builtins in compiler, 
but instead fully rely on interceptors.

[Bug sanitizer/61547] Invalid sanitization of trailing byte in __builtin_strlen

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61547

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Kostya Serebryany from comment #2)
> Note that in clang we chose not to instrument any builtins in compiler, 
> but instead fully rely on interceptors.

So you never expand such builtins inline?

[Bug sanitizer/61547] Invalid sanitization of trailing byte in __builtin_strlen

[kcc at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61547

--- Comment #4 from Kostya Serebryany <kcc at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #3)
> (In reply to Kostya Serebryany from comment #2)
> > Note that in clang we chose not to instrument any builtins in compiler, 
> > but instead fully rely on interceptors.
> 
> So you never expand such builtins inline?

Not that I know of. 
LLVM has only 3 builtins like this (memset/memcpy/memmove):
http://llvm.org/docs/LangRef.html#standard-c-library-intrinsics
And asan replaces these builtins with asan's own call (e.g. __asan_memset)

[Bug sanitizer/61547] Invalid sanitization of trailing byte in __builtin_strlen

[tetra2005 at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61547

Yuri Gribov <tetra2005 at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tetra2005 at gmail dot com

--- Comment #5 from Yuri Gribov <tetra2005 at gmail dot com> ---
Yeah, GCC is different in this regard.

[Bug sanitizer/61547] Invalid sanitization of trailing byte in __builtin_strlen

[ygribov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61547

--- Comment #6 from ygribov at gcc dot gnu.org ---
Author: ygribov
Date: Thu Oct 16 13:46:39 2014
New Revision: 216326

URL: https://gcc.gnu.org/viewcvs?rev=216326&root=gcc&view=rev
Log:
New asan-instrumentation-with-call-threshold
 parameter.

2014-10-16  Yury Gribov  <y.gribov@samsung.com>

    Backport from mainline
    2014-06-16  Yury Gribov  <y.gribov@samsung.com>

    * asan.c (check_func): New function.
    (maybe_create_ssa_name): Likewise.
    (build_check_stmt_with_calls): Likewise.
    (use_calls_p): Likewise.
    (report_error_func): Change interface.
    (build_check_stmt): Allow non-integer lengths; add support
    for new parameter.
    (asan_instrument): Likewise.
    (instrument_mem_region_access): Moved code to
    build_check_stmt.
    (instrument_derefs): Likewise.
    (instrument_strlen_call): Likewise.
    * cfgcleanup.c (old_insns_match_p): Add support for new
    functions.
    * doc/invoke.texi: Describe new parameter.
    * params.def: Define new parameter.
    * params.h: Likewise.
    * sanitizer.def: Describe new builtins.

    * c-c++-common/asan/instrument-with-calls-1.c: New test.
    * c-c++-common/asan/instrument-with-calls-2.c: Likewise.
    * c-c++-common/asan/no-redundant-instrumentation-1.c: Update
    test patterns.
    * c-c++-common/asan/no-redundant-instrumentation-2.c:
    Likewise.
    * c-c++-common/asan/no-redundant-instrumentation-4.c:
    Likewise.
    * c-c++-common/asan/no-redundant-instrumentation-5.c:
    Likewise.
    * c-c++-common/asan/no-redundant-instrumentation-6.c:
    Likewise.
    * c-c++-common/asan/no-redundant-instrumentation-7.c:
    Likewise.
    * c-c++-common/asan/no-redundant-instrumentation-8.c:
    Likewise.

    Backport from mainline
    2014-06-16  Yury Gribov  <y.gribov@samsung.com>

    * asan.c (build_check_stmt): Fix maybe-uninitialized warning.

    Backport from mainline
    2014-06-18  Yury Gribov  <y.gribov@samsung.com>

    PR sanitizer/61530

    * asan.c (build_check_stmt): Add condition.

    * c-c++-common/asan/pr61530.c: New test.

    Backport from mainline
    2014-06-18  Yury Gribov  <y.gribov@samsung.com>

    PR sanitizer/61547

    * asan.c (instrument_strlen_call): Fixed instrumentation of
    trailing byte.

    * c-c++-common/asan/strlen-overflow-1.c: New test.

Added:
   
branches/gcc-4_9-branch/gcc/testsuite/c-c++-common/asan/instrument-with-calls-1.c
   
branches/gcc-4_9-branch/gcc/testsuite/c-c++-common/asan/instrument-with-calls-2.c
    branches/gcc-4_9-branch/gcc/testsuite/c-c++-common/asan/pr61530.c
    branches/gcc-4_9-branch/gcc/testsuite/c-c++-common/asan/strlen-overflow-1.c
Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/asan.c
    branches/gcc-4_9-branch/gcc/cfgcleanup.c
    branches/gcc-4_9-branch/gcc/doc/invoke.texi
    branches/gcc-4_9-branch/gcc/params.def
    branches/gcc-4_9-branch/gcc/params.h
    branches/gcc-4_9-branch/gcc/sanitizer.def
    branches/gcc-4_9-branch/gcc/testsuite/ChangeLog
   
branches/gcc-4_9-branch/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c
   
branches/gcc-4_9-branch/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-2.c
   
branches/gcc-4_9-branch/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c
   
branches/gcc-4_9-branch/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c
   
branches/gcc-4_9-branch/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c
   
branches/gcc-4_9-branch/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c
   
branches/gcc-4_9-branch/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c

[Bug sanitizer/61547] Invalid sanitization of trailing byte in __builtin_strlen

[y.gribov at samsung dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61547

Yury Gribov <y.gribov at samsung dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #7 from Yury Gribov <y.gribov at samsung dot com> ---
.