[ASan] Define an internal implementation of strchr to make stack OOB tests pass

Define an internal implementation of strchr to make stack OOB tests pass

Committed as http://llvm.org/viewvc/llvm-project?view=rev&revision=150499

[timurrrr_at_google_com, 2012-02-14 17:47:15]

Hi Kostya,

Can you please review this small patch?

Thanks!

[mclow.lists_gmail.com, 2012-02-14 18:12:41]

On Feb 14, 2012, at 9:47 AM, timurrrr@google.com wrote:

> Reviewers: kcc1,
> 
> Message:
> Hi Kostya,
> 
> Can you please review this small patch?
> 
> Thanks!
> 
> Description:
> Define an internal implementation of strchr to make stack OOB tests pass
> 
> Please review this at http://codereview.appspot.com/5668047/
> 
> Affected files:
>   M lib/asan/asan_interceptors.cc
>   M lib/asan/asan_rtl.cc
> 
> +char* internal_strchr(const char *s, int c) {
> +  for (size_t i = 0; ; ++i) {
> +    if (s[i] == (char)c)
> +      return (char*)&s[i];
> +    if (s[i] == 0)
> +      return NULL;
> +  }
> +}

Isn't strchr defined to return a const char *?
And why all the mucking about with indexes?

How about:

	const char* internal_strchr(const char *s, int c) {
	  for ( ; *s; ++s )
	  	if ( *s == c )
	  	    return s;
	  return NULL;
	}

instead?


-- Marshall

Marshall Clow     Idio Software   <mailto:mclow.lists@gmail.com>

A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly
moderated down to (-1, Flamebait).
        -- Yu Suzuki

[ramosian.glider, 2012-02-14 18:18:27]

+1 to what Marshall said

http://codereview.appspot.com/5668047/diff/4/lib/asan/asan_interceptors.cc
File lib/asan/asan_interceptors.cc (right):

http://codereview.appspot.com/5668047/diff/4/lib/asan/asan_interceptors.cc#ne...
lib/asan/asan_interceptors.cc:112: char* internal_strchr(const char *s, int c) {
Please consider checking for s to be non-NULL.

[timurrrr_at_google_com, 2012-02-14 18:26:39]

On Tue, Feb 14, 2012 at 10:12 PM, Marshall Clow <mclow.lists@gmail.com> wrote:
> Isn't strchr defined to return a const char *?
Not in asan_interceptors.cc :369,  :380
:)

> And why all the mucking about with indexes?
>
> How about:
>
>        const char* internal_strchr(const char *s, int c) {
>          for ( ; *s; ++s )
>                if ( *s == c )
>                    return s;
>          return NULL;
>        }
> instead?
I think this is a wrong implementation as strchr(s="\0", 0) should
return s, not NULL.
I agree with "no need for indices" though, will update soon.

[timurrrr_at_google_com, 2012-02-14 18:26:49]

http://codereview.appspot.com/5668047/diff/4/lib/asan/asan_interceptors.cc
File lib/asan/asan_interceptors.cc (right):

http://codereview.appspot.com/5668047/diff/4/lib/asan/asan_interceptors.cc#ne...
lib/asan/asan_interceptors.cc:112: char* internal_strchr(const char *s, int c) {
On 2012/02/14 18:18:27, ramosian.glider wrote:
> Please consider checking for s to be non-NULL.
Why?
You don't do that in the other internal_str* functions, right?

[timurrrr_at_google_com, 2012-02-14 18:30:57]

On Tue, Feb 14, 2012 at 10:26 PM, Timur Iskhodzhanov
<timurrrr@google.com> wrote:
> On Tue, Feb 14, 2012 at 10:12 PM, Marshall Clow <mclow.lists@gmail.com> wrote:
>> Isn't strchr defined to return a const char *?
> Not in asan_interceptors.cc :369,  :380
I'm surprised but "man strchr" actually shows "char *" return type on
both Linux and Mac!

[timurrrr_at_google_com, 2012-02-14 18:33:48]

Got rid of the indices, PTAL

[scanon_apple.com, 2012-02-14 18:46:18]

On Feb 14, 2012, at 1:30 PM, Timur Iskhodzhanov <timurrrr@google.com> wrote:

> On Tue, Feb 14, 2012 at 10:26 PM, Timur Iskhodzhanov
> <timurrrr@google.com> wrote:
>> On Tue, Feb 14, 2012 at 10:12 PM, Marshall Clow <mclow.lists@gmail.com>
wrote:
>>> Isn't strchr defined to return a const char *?
>> Not in asan_interceptors.cc :369,  :380
> I'm surprised but "man strchr" actually shows "char *" return type on
> both Linux and Mac!

strchr( ) returns a char *.

7.21.5.2 The strchr function

Synopsis

     #include <string.h>
     char *strchr(const char *s, int c);

- Steve

[mclow.lists_gmail.com, 2012-02-14 18:47:05]

On Feb 14, 2012, at 10:30 AM, Timur Iskhodzhanov wrote:

> On Tue, Feb 14, 2012 at 10:26 PM, Timur Iskhodzhanov
> <timurrrr@google.com> wrote:
>> On Tue, Feb 14, 2012 at 10:12 PM, Marshall Clow <mclow.lists@gmail.com>
wrote:
>>> Isn't strchr defined to return a const char *?
>> Not in asan_interceptors.cc :369,  :380
> I'm surprised but "man strchr" actually shows "char *" return type on
> both Linux and Mac!

yeah, that's a C bug - but it's in the standard, (AFAIK), so it's an official
bug.

In C++, there are two ;-)
> const char* strchr(const char* s, int c);
>       	char* strchr(      char* s, int c);


-- Marshall

Marshall Clow     Idio Software   <mailto:mclow.lists@gmail.com>

A.D. 1517: Martin Luther nails his 95 Theses to the church door and is promptly
moderated down to (-1, Flamebait).
        -- Yu Suzuki

[kcc, 2012-02-14 18:49:15]

There is only one place where REAL(strchr) is used and that place is not
performance critical, so we don't need to use REAL(strchr). 
Instead define internal_strchr (as you did) and use it directly. 

Also, please check what does usual strchr return with c=0.

[kcc, 2012-02-14 18:57:40]

> Also, please check what does usual strchr return with c=0.
Ignore this bit, looks like you are doing it right.

[ramosian.glider, 2012-02-14 19:04:08]

http://codereview.appspot.com/5668047/diff/4/lib/asan/asan_interceptors.cc
File lib/asan/asan_interceptors.cc (right):

http://codereview.appspot.com/5668047/diff/4/lib/asan/asan_interceptors.cc#ne...
lib/asan/asan_interceptors.cc:112: char* internal_strchr(const char *s, int c) {
In fact I'd better do that everywhere :)

[timurrrr_at_google_com, 2012-02-14 19:30:18]

Kostya, PTAL

Alexander,
I think the value of these CHECKs is debatable and if we decide they are needed
we can bulk-add them to all str* mem* functions later, WDYT?

[kcc, 2012-02-14 19:33:36]

LGTM! 

You may want to declare internal_strchr as returning "const char *" -- it does
not have to obey the C standard bug.

[timurrrr_at_google_com, 2012-02-14 19:38:16]

On Tue, Feb 14, 2012 at 11:33 PM,  <konstantin.s.serebryany@gmail.com> wrote:
> LGTM!
Thanks!
r150499

> You may want to declare internal_strchr as returning "const char *" --
> it does not have to obey the C standard bug.
I want to keep its prototype compatible with REAL(strchr) just in case.
Otherwise, one might have to crack his head a few more times :)