Bug 61547 - Invalid sanitization of trailing byte in __builtin_strlen
Summary: Invalid sanitization of trailing byte in __builtin_strlen
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: sanitizer (show other bugs)
Version: 5.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-18 09:52 UTC by Yury Gribov
Modified: 2015-04-17 07:45 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments
Reprocase (466 bytes, text/plain)
2014-06-18 09:52 UTC, Yury Gribov
Details
Proposed patch (1.13 KB, patch)
2014-06-18 09:54 UTC, Yury Gribov
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Gribov 2014-06-18 09:52:14 UTC
Created attachment 32963 [details]
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
Comment 1 Yury Gribov 2014-06-18 09:54:44 UTC
Created attachment 32964 [details]
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.
Comment 2 Kostya Serebryany 2014-06-18 10:37:51 UTC
Note that in clang we chose not to instrument any builtins in compiler, 
but instead fully rely on interceptors.
Comment 3 Richard Biener 2014-06-18 11:18:57 UTC
(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?
Comment 4 Kostya Serebryany 2014-06-18 11:33:02 UTC
(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)
Comment 5 Yuri Gribov 2014-06-19 20:09:29 UTC
Yeah, GCC is different in this regard.
Comment 6 Yury Gribov 2014-10-16 13:47:10 UTC
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
Comment 7 Yury Gribov 2015-04-17 07:45:42 UTC
.