[ASAN][DARWIN] Remove getpwnam(NULL) test for undefined behavior

[thetruestblue]

Summary

Reverting a patch that was added to test for getpwnam(NULL) -- it was noted at the time the behavior might have been a bug, however the patch was added for binary compatibility. Because of the change in the expected behavior, we are reverting this commit, as the test added is no longer passing.

Update: Rather than reverting the original commit, updating this to only remove the unnecessary test.

Original Patch: https://reviews.llvm.org/D40052

rdar://98592334

[yln]

Because of the change in the expected behavior

What is that change? I am a bit confused about this revert.

If I compile the test file without ASan, it still doesn't crash so ASan should allow execution without a crash as well.

[thetruestblue]

In D133445#3775419, @yln wrote:

Because of the change in the expected behavior

What is that change? I am a bit confused about this revert.

If I compile the test file without ASan, it still doesn't crash so ASan should allow execution without a crash as well.

I need to look into this, I was going off of info from others and I should have confirmed myself. But currently in our CI we get a segfault for passing NULL.

The test deleted tests getpwnam(NULL) which as far as I know is undefined behavior. From what I saw it looked like this behavior on Darwin now causes a Seg Fault.
Regardless, we can change the single test without changing sanitizer_common_interceptors.inc, which would be more appropriate.

Also @yln it fails on devices, doesn't fail on sims. But the behavior is undefined, as was noted in the original patch, and I'm not sure it's necessary to test for a behavior that changes in different scenarios if nothing relies on that.

[rsundahl]

The guard on line 1999 (above) can go as well since it was only to allow getpwnam(NULL) to succeed at line 2001 and not fail at line 2000. If someone is calling getpwnam(NULL) then they may as well crash at line 2000. (You could put an assert in there as an affirmation that it's not called with NULL. Might as well fail fast.)

[thetruestblue]

Keeping the null guard from the original commit.

[rsundahl]

Thank you @thetruestblue. I came around as well that we leave the guard to protect the strlen() call.

[yln]

compiler-rt/test/asan/TestCases/Darwin/getpwnam.c

3

How about this?

[rsundahl]

compiler-rt/test/asan/TestCases/Darwin/getpwnam.c

3

So far, but this is a test that can't be expected to pass (Posix specification), the justification for which can't be recalled and the maintenance of which isn't being enforced internally. @kubamracek is the original author and agreed with culling the test itself but leaving the guard for the call to strlen(). Worst case we discover that we do depend on this behavior (it will segfault) and we can elevate the expectation to a first class requirement.

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

 INTERCEPTOR(__sanitizer_passwd *, getpwnam, const char *name) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, getpwnam, name);
-  if (name)
-    COMMON_INTERCEPTOR_READ_RANGE(ctx, name, internal_strlen(name) + 1);
+  COMMON_INTERCEPTOR_READ_RANGE(ctx, name, internal_strlen(name) + 1);
   __sanitizer_passwd *res = REAL(getpwnam)(name);
   unpoison_passwd(ctx, res);
   return res;

compiler-rt/test/asan/TestCases/Darwin/getpwnam.c

-// RUN: %clang_asan %s -o %t
-// RUN: %run %t 2>&1 | FileCheck %s
-
-#include <pwd.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <sys/types.h>
-
-int main(int argc, const char * argv[]) {
-  getpwnam(NULL);
-  fprintf(stderr, "Finished.\n");
-  return 0;
-}
-
-// CHECK: Finished.