Re: [PATCH BZ#20422] Do not allow asan/msan/tsan and fortify at the same time.

[Yuri Gribov <tetra2005@gmail.com>]

On Tue, Sep 6, 2016 at 9:39 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 09/05/2016 07:27 PM, Maxim Ostapenko wrote:
>>
>> Hi!
>>
>> When fortify is used with MSan it will cause MSan false positives.
>>
>> #include <stdio.h>
>> #include <string.h>
>> int main()
>> {
>>         char text[100];
>>         sprintf(text, "hello");
>>         printf("%lu\n", strlen(text));
>> }
>>
>> % clang test.c -fsanitize=memory   -O3 && ./a.out
>> 5
>> % clang test.c -fsanitize=memory -D_FORTIFY_SOURCE=2  -O3 && ./a.out
>> Uninitialized bytes in __interceptor_strlen at offset 0 inside
>> [0x7ffe259e4d20, 6)
>> ==26297==WARNING: MemorySanitizer: use-of-uninitialized-value
>>     #0 0x4869cc in main
>>
>> With ASan, this will not cause false positives, but may case false
>> negatives or just confuse people with "wrong" reports when fortify
>> catches the error.
>
>
> Why does the above cause a warning, but this does not, and happily prints
> the undefined array members?
>
> #include <stdio.h>
> #include <pwd.h>
> #include <unistd.h>
> #include <string.h>
> #include <err.h>
> #include <grp.h>
>
> int main()
> {
>   struct passwd *pw = getpwuid (getuid ());
>   if (pw == NULL)
>     err (1, "getpwuid");
>   gid_t groups[50];
>   int ngroups = 50;
>   if (getgrouplist (pw->pw_name, pw->pw_gid, groups, &ngroups) < 0)
>     errx (1, "getgrouplist");
>   for (int i = 0; i < 50; ++i)
>     printf ("group %d: %lld\n", i + 1, (long long) groups[i]);
> }
>
>
> I find this rather confusing.  -D_FORTIFY_SOURCE=2 does not make a
> difference here.
>
>> Although fortify is good thing as it (and it's enabled by default on
>> some major distros e.g. Ubuntu and Gentoo), people still complain about
>> {A, M}San vs fortify interaction, see e.g.
>> https://github.com/google/sanitizers/issues/689. One possible solution
>> would be to extend {A, M}San to support foo_chk() functions, but this
>> would increase the complexity of sanitizer tools with quite small
>> benefit. Another choice would be to warn users when they compile their
>> code with {A, M, T}San and fortify enabled.
>
>
> At least for Memory Sanitizer, the documentation clearly says that
> *everything* has to be compiled with it.  I read that as as meaning that the
> interceptors are just a kludge.
>
> If you do not intended to implement further interceptors, I expect you don't
> want to implement those for open/openat either, other compile-time
> fortification, or whatever other fortify functions we might add which are
> not directly related to memory safety.  This means that for full coverage, a
> developer would have to compile to test with the different sanitizers *and*
> _FORTIFY_SOURCE.  I'm not sure if that leads to a good developer experience.
>
> We could conceivably guard every _chk wrapper (say __sprintf_chk) with
>
> #ifndef __fortify_no___sprintf_chk
> …
> #endif
>
> .  In sanitizer mode, for those wrappers you have deemed to be unnecessary,
> the compiler would define these macros, so that glibc wouldn't install the
> wrapper.

Hi Florian,

Would the above approach be accepted for trunk? The reason for me
pushing this is because FORTIFY_SOURCE is now enabled by default in
major distros and this start to be a detrimental factor for ASan
efficiency (there are already 3 open bugs related to this in tracker
and they keep coming).

Sanitized Glibc may be a solution for some users (although I suspect
many users would keep using current interceptor-based approach as it
less invasive and much easier to integrate) but I'm not aware of
anyone working or planning to start work on it in near future.

> Another option would be to provide a glibc which has been compiled with the
> required sanitizers, so that most interceptors become unnecessary.
>
>> I've tried to add a testcase for new warning into Glibc testsuite, but
>> failed to see how exactly I can do it. Does Glibc have some framework
>> for compilation tests?
>
>
> It does not.  Carlos proposed something earlier this year, but it was not
> able to check for the presence expected warnings.  We need something like
> this for the compile-time checks.
>
> Florian