GNU bug report logs - #19563
grep -F: fix a heap buffer (read) overrun

Previous Next

Full log

Message #11 received at 19563 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 19563 <at> debbugs.gnu.org, Yuliy Pisetsky <ypisetsky <at> fb.com>,
 Nima Aghdaii <naghdaii <at> fb.com>
Subject: Re: bug#19563: grep -F: fix a heap buffer (read) overrun
Date: Sat, 10 Jan 2015 16:42:28 -0800

[Message part 1 (text/plain, inline)]

On Sat, Jan 10, 2015 at 4:02 PM, Paul Eggert <eggert <at> cs.ucla.edu> wrote:
> Jim Meyering wrote:
>>
>> +#if defined __clang__
>> +# if __has_feature(address_sanitizer)
>> +#  define HAVE_ASAN 1
>> +# endif
>> +#elif defined __GNUC__ \
>> +  && (((__GNUC__ == 4) && (__GNUC_MINOR__ >= 8)) || (__GNUC__ >= 5)) \
>> +  && __SANITIZE_ADDRESS__
>> +# define HAVE_ASAN 1
>> +#endif
>
>
> How about the following instead?
>
> #ifndef __has_feature
> # define __has_feature(a) false
> #endif
>
> #if defined __SANITIZE_ADDRESS__ || __has_feature (address_sanitizer)
> # define HAVE_ASAN 1
> #else
> # define HAVE_ASAN 0
> #endif
>
> This is what Emacs uses (its symbol is ADDRESS_SANITIZER instead of
> HAVE_ASAN, for what that's worth).
>
>> +  ASAN_POISON_MEMORY_REGION (buflim + sizeof(uword),
>> +                             bufalloc - (buflim - buffer) -
>> sizeof(uword));
>>
>
> The two 'sizeof's need spaces afterwards.

Fixed.

> I don't see the value of having macros here.  How about the following
> instead?
>
> #ifndef HAVE_ASAN
> static void
> __asan_unpoison_memory_region (void const volatile *addr, size_t size)
> {
> }
>
> static void
> __asan_unpoison_memory_region (void const volatile *addr, size_t size)
> {
> }
> #endif

I agree.  Adjusted via s/unpoison/poison/ in the first.
In addition, I have added _GL_UNUSED so as not to run afoul of grep's
use of -Werror=unused-function. Also, I have adapted not to
declare when using the static wrappers.

and to use #if, not #ifndef, since now HAVE_ASAN is always defined.

> And then have the callers invoke '__asan_poison_memory_region' instead of
> 'ASAN_POISON_MEMORY_REGION'.  This way, there should be no need to pull in
> the ignore-value machinery, it's two less macros to worry about, and there's
> better type checking when address sanitization is not in use.

Thanks for all the good suggestions.  Here's an updated version:

[0001-tests-add-support-for-ASAN-memory-poisoning.patch (application/octet-stream, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.