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

[Florian Weimer <fweimer at redhat dot com>]

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.

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