[asan] Fix recvfrom.cc testcase failure in large parallel tests run.

[m.ostapenko]

Summary

This testcase currently fails on sanitizer-x86_64-linux buildbot with following error:

FAIL: AddressSanitizer-i386-linux :: TestCases/Linux/recvfrom.cc (524 of 1166)
******************** TEST 'AddressSanitizer-i386-linux :: TestCases/Linux/recvfrom.cc' FAILED ********************
Script:
--
/mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/clang_build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m32 /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/asan/TestCases/Linux/recvfrom.cc -o /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/asan/I386LinuxConfig/TestCases/Linux/Output/recvfrom.cc.tmp && not  /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/clang_build/projects/compiler-rt/test/asan/I386LinuxConfig/TestCases/Linux/Output/recvfrom.cc.tmp 2>&1 | FileCheck /mnt/b/sanitizer-buildbot1/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/asan/TestCases/Linux/recvfrom.cc
--
Exit Code: 2

Command Output (stderr):
--
FileCheck error: '-' is empty.

--

I haven't managed reproduce this on my local box, but here my thoughts about the issue:
I believe this failure happens because of race on port 1234 between AddressSanitizer-i386-linux and AddressSanitizer-x86_64-linux instances of recvfrom.cc testcase. This patch tries to resolve the issue by trying to use another port if 1234 has already acquired.

[dvyukov]

test/asan/TestCases/Linux/recvfrom.cc

16–21

s/kPortNum/portNum/
this is not a constant anymore

51

I don't see where the mutex is locked... where is it?

[ygribov]

test/asan/TestCases/Linux/recvfrom.cc

48

SO says (http://stackoverflow.com/questions/1075399/how-to-bind-to-any-available-port) that you can specify port 0 so that kernel selects it automatically for you.

[m.ostapenko]

test/asan/TestCases/Linux/recvfrom.cc

48

Nice, thanks!

51

Eh, It seems I attached truncated patch. Sorry about that.

[ygribov]

test/asan/TestCases/Linux/recvfrom.cc

54–55

Why not just init this with all zeros? Currently you implicitly rely on the fact that strlen("123456789") > kBufSize/2 which seems to be unnecessary.

54–55

I'm not sure you need trivial comments (especially given the self-descriptive name of the variable).

57–58

Format of messages varies across fprintfs. Probably makes sense to unify them (e.g. by wrapping fprintf + exit into a macro).

58–59

You don't seem to use this.

59–65

Shouldn't you abort here?

69–70

Spurious white before &? Same issue in other places.

[m.ostapenko]

Dmitry, Yura, thanks for your nits. I'm updating the diff.

[ygribov]

test/asan/TestCases/Linux/recvfrom.cc

20

Maybe wrap in do-while(0) just in case?

30

Still no agreement on ERROR vs. Error. I suggest to move prefix and newline to CHECK_ERROR macro.

35

Perhaps avoid htons?

45

Shouldn't mutexes be checked as well? BTW you could probably avoid mutexes alltogether if you moved server to main thread (and start client thread from it after socket is initialized).

55

Should probably initialialize to avoid UB.

64

I always thought that there's no need for explicit casts to starred voids (ditto for memset, memcpy, etc.).

72

Should this be checked?

[m.ostapenko]

Addressing Yura's nits.

[dvyukov]

LGTM

[m.ostapenko]

Thanks! I'm going to land this on Monday to be able make quick response on possible failures.

[filcab]

Shouldn't all this be in TestCases/Posix?

compiler-rt/trunk/test/asan/TestCases/Linux/recvfrom.cc

16 ↗

(On Diff #49340)

Maybe use perror?

We can even do something like:

#define CHECK_ERROR(p, name)                                                   \
  do {                                                                         \
    if (p) {                                                                   \
      perror(#name);                                                           \
      exit(1);                                                                 \
    } else {                                                                   \
      printf("%s succeeded.\n", #name);                                        \
    }                                                                          \
  }

...
CHECK_ERROR(succeeded < 0, getsockname);
// CHECK: getsockname succeeded.

That way, even though we're slightly more noisy when there's no error (and with the CHECK lines), you should immediately see the proper error string when the test fails, since FileCheck will fail on that line.
(Then again, FileCheck might show the appropriate line already, so no need to switch that part (except for perror).

[ygribov]

Shouldn't all this be in TestCases/Posix?

We don't have (easy) access to Mac so we are really scared to break it...

compiler-rt/trunk/test/asan/TestCases/Linux/recvfrom.cc

16 ↗

(On Diff #49340)

We thought about doing a more proper macro but later decided to not invest too much time in it. That's throwaway test code used in just one file anyway.

test/asan/TestCases/Linux/recvfrom.cc

 // Test that ASan detects buffer overflow on read from socket via recvfrom.
 //
 // RUN: %clangxx_asan %s -o %t && not %run %t 2>&1 | FileCheck %s
 //
-// REQUIRES: broken
 // UNSUPPORTED: android
 
 #include <stdio.h>
 #include <unistd.h>
 #include <stdlib.h>
 #include <string.h>
 #include <netdb.h>
 #include <sys/types.h>
 #include <sys/socket.h>
 #include <pthread.h>
 
-const int kPortNum = 1234;
+#define CHECK_ERROR(p, m)   \
+  if (p) {                  \
+    fprintf(stderr, m);     \
+    exit(1);                \
+  }
+
 const int kBufSize = 10;
+pthread_mutex_t server_ready_mu = PTHREAD_MUTEX_INITIALIZER;
 
 static void *server_thread_udp(void *data) {
   char buf[kBufSize / 2];
-  struct sockaddr_in serveraddr; // server's addr
+  struct sockaddr_in serveraddr;
+  socklen_t addrlen;
   int sockfd = socket(AF_INET, SOCK_DGRAM, 0);
-  if (sockfd < 0)
-    fprintf(stderr, "ERROR opening socket\n");
+  CHECK_ERROR(sockfd < 0, "ERROR opening socket.\n");
 
-  memset((char *) &serveraddr, 0, sizeof(serveraddr));
+  memset((char *)&serveraddr, 0, sizeof(serveraddr));
   serveraddr.sin_family = AF_INET;
   serveraddr.sin_addr.s_addr = htonl(INADDR_ANY);
-  serveraddr.sin_port = htons(kPortNum);
+  serveraddr.sin_port = htons(0);
+
+  int bound = bind(sockfd, (struct sockaddr *)&serveraddr, sizeof(serveraddr));
+  CHECK_ERROR(bound < 0, "ERROR on binding.\n");
 
-  if (bind(sockfd, (struct sockaddr *) &serveraddr, sizeof(serveraddr)) < 0)
-    fprintf(stderr, "ERROR on binding\n");
+  // Provide necessary information (e.g. port number) to client thread.
+  int succeeded = getsockname(sockfd, (struct sockaddr *)data, &addrlen);
+  CHECK_ERROR(succeeded < 0, "ERROR in getsockname.\n");
 
+  // Now it's safe to unblock client thread.
+  pthread_mutex_unlock(&server_ready_mu);
   recvfrom(sockfd, buf, kBufSize, 0, NULL, NULL); // BOOM
-  // CHECK: {{WRITE of size 9 at 0x.* thread T1}}
+  // CHECK: {{WRITE of size 10 at 0x.* thread T1}}
   // CHECK: {{    #1 0x.* in server_thread_udp.*recvfrom.cc:}}[[@LINE-2]]
   // CHECK: {{Address 0x.* is located in stack of thread T1 at offset}}
   // CHECK-NEXT: in{{.*}}server_thread_udp{{.*}}recvfrom.cc
   return NULL;
 }
 
 int main() {
-  char buf[kBufSize] = "123456789";
-  struct sockaddr_in serveraddr; // server's addr
-
+  char buf[kBufSize];
+  struct sockaddr_in serveraddr;
   pthread_t server_thread;
-  if (pthread_create(&server_thread, NULL, server_thread_udp, NULL)) {
-    fprintf(stderr, "Error creating thread\n");
-    exit(1);
-  }
+
+  memset((char *)&serveraddr, 0, sizeof(serveraddr));
+  // Default state for server_ready_mu is 'unlocked'. Lock it to make sure
+  // following pthread_mutex_lock won't pass before server becomes ready.
+  pthread_mutex_lock(&server_ready_mu);
+  int succeeded = pthread_create(&server_thread, NULL, server_thread_udp,
+                                 (void *)&serveraddr);
+  CHECK_ERROR(succeeded, "Error creating thread.\n");
 
   int sockfd = socket(AF_INET, SOCK_DGRAM, 0);
-  struct hostent *server;
-  char hostname[] = "localhost";
-  if (sockfd < 0)
-    fprintf(stderr, "ERROR opening socket\n");
-
-  server = gethostbyname(hostname);
-  if (!server) {
-    fprintf(stderr,"ERROR, no such host as %s\n", hostname);
-    exit(1);
-  }
+  CHECK_ERROR(sockfd < 0, "ERROR opening socket.\n");
 
-  memset((char *) &serveraddr, 0, sizeof(serveraddr));
-  serveraddr.sin_family = AF_INET;
-  memcpy((char *)&serveraddr.sin_addr.s_addr, (char *)server->h_addr,
-          server->h_length);
-  serveraddr.sin_port = htons(kPortNum);
-  sendto(sockfd, buf, strlen(buf), 0, (struct sockaddr *) &serveraddr,
+  // Wait for server that should properly initialize serveraddr.
+  pthread_mutex_lock(&server_ready_mu);
+  sendto(sockfd, buf, sizeof(buf), 0, (struct sockaddr *)&serveraddr,
          sizeof(serveraddr));
 
-  if (pthread_join(server_thread, NULL)) {
-    fprintf(stderr, "Error joining thread\n");
-    exit(1);
-  }
-
+  succeeded = pthread_join(server_thread, NULL);
+  CHECK_ERROR(succeeded, "Error joining thread.\n");
+  pthread_mutex_unlock(&server_ready_mu);
+  pthread_mutex_destroy(&server_ready_mu);
   return 0;
 }