[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 ↗

(On Diff #49163)

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

40 ↗

(On Diff #49163)

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

[ygribov]

test/asan/TestCases/Linux/recvfrom.cc

37 ↗

(On Diff #49163)

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

37 ↗

(On Diff #49163)

Nice, thanks!

40 ↗

(On Diff #49163)

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

[ygribov]

test/asan/TestCases/Linux/recvfrom.cc

50 ↗

(On Diff #49163)

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.

51 ↗

(On Diff #49163)

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

55 ↗

(On Diff #49163)

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

60 ↗

(On Diff #49163)

You don't seem to use this.

63 ↗

(On Diff #49163)

Shouldn't you abort here?

71 ↗

(On Diff #49163)

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 ↗

(On Diff #49178)

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

30 ↗

(On Diff #49178)

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

35 ↗

(On Diff #49178)

Perhaps avoid htons?

45 ↗

(On Diff #49178)

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 ↗

(On Diff #49178)

Should probably initialialize to avoid UB.

64 ↗

(On Diff #49178)

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

72 ↗

(On Diff #49178)

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

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

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.

compiler-rt/trunk/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;
-const int kBufSize = 10;
-
-static void *server_thread_udp(void *data) {
-  char buf[kBufSize / 2];
-  struct sockaddr_in serveraddr; // server's addr
-  int sockfd = socket(AF_INET, SOCK_DGRAM, 0);
-  if (sockfd < 0)
-    fprintf(stderr, "ERROR opening socket\n");
+#define CHECK_ERROR(p, m)                                                      \
+  do {                                                                         \
+    if (p) {                                                                   \
+      fprintf(stderr, "ERROR " m "\n");                                        \
+      exit(1);                                                                 \
+    }                                                                          \
+  } while (0)
 
-  memset((char *) &serveraddr, 0, sizeof(serveraddr));
-  serveraddr.sin_family = AF_INET;
-  serveraddr.sin_addr.s_addr = htonl(INADDR_ANY);
-  serveraddr.sin_port = htons(kPortNum);
-
-  if (bind(sockfd, (struct sockaddr *) &serveraddr, sizeof(serveraddr)) < 0)
-    fprintf(stderr, "ERROR on binding\n");
+const int kBufSize = 10;
+int sockfd;
 
-  recvfrom(sockfd, buf, kBufSize, 0, NULL, NULL); // BOOM
-  // CHECK: {{WRITE of size 9 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
+static void *client_thread_udp(void *data) {
+  const char buf[kBufSize] = {0, };
+  socklen_t addrlen;
+  struct sockaddr_in serveraddr;
+
+  int succeeded = getsockname(sockfd, (struct sockaddr *)&serveraddr, &addrlen);
+  CHECK_ERROR(succeeded < 0, "in getsockname");
+
+  succeeded = sendto(sockfd, buf, kBufSize, 0,
+                     (struct sockaddr *)&serveraddr, sizeof(serveraddr));
+  CHECK_ERROR(succeeded < 0, "in sending message");
   return NULL;
 }
 
 int main() {
-  char buf[kBufSize] = "123456789";
-  struct sockaddr_in serveraddr; // server's addr
-
-  pthread_t server_thread;
-  if (pthread_create(&server_thread, NULL, server_thread_udp, NULL)) {
-    fprintf(stderr, "Error creating thread\n");
-    exit(1);
-  }
+  char buf[kBufSize / 2];
+  pthread_t client_thread;
+  struct sockaddr_in serveraddr;
 
-  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);
-  }
+  sockfd = socket(AF_INET, SOCK_DGRAM, 0);
+  CHECK_ERROR(sockfd < 0, "opening socket");
 
-  memset((char *) &serveraddr, 0, sizeof(serveraddr));
+  memset(&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,
-         sizeof(serveraddr));
-
-  if (pthread_join(server_thread, NULL)) {
-    fprintf(stderr, "Error joining thread\n");
-    exit(1);
-  }
+  serveraddr.sin_addr.s_addr = htonl(INADDR_ANY);
+  serveraddr.sin_port = 0;
+
+  int bound = bind(sockfd, (struct sockaddr *)&serveraddr, sizeof(serveraddr));
+  CHECK_ERROR(bound < 0, "on binding");
 
+  int succeeded =
+      pthread_create(&client_thread, NULL, client_thread_udp, &serveraddr);
+  CHECK_ERROR(succeeded, "creating thread");
+
+  recvfrom(sockfd, buf, kBufSize, 0, NULL, NULL); // BOOM
+  // CHECK: {{WRITE of size 10 at 0x.* thread T0}}
+  // CHECK: {{    #1 0x.* in main.*recvfrom.cc:}}[[@LINE-2]]
+  // CHECK: {{Address 0x.* is located in stack of thread T0 at offset}}
+  // CHECK-NEXT: in{{.*}}main{{.*}}recvfrom.cc
+  succeeded = pthread_join(client_thread, NULL);
+  CHECK_ERROR(succeeded, "joining thread");
   return 0;
 }