[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

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

40

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

[ygribov]

test/asan/TestCases/Linux/recvfrom.cc

37

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

Nice, thanks!

40

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

[ygribov]

test/asan/TestCases/Linux/recvfrom.cc

50

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

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

55

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

60

You don't seem to use this.

63

Shouldn't you abort here?

71

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?

25

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

30

Perhaps avoid htons?

34

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).

52

Should probably initialialize to avoid UB.

68

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

82

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;
+int kPortNum = 1234;
 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
   int sockfd = socket(AF_INET, SOCK_DGRAM, 0);
   if (sockfd < 0)
     fprintf(stderr, "ERROR opening socket\n");
 
   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");
-
+  // In parallel test run we may already have acquired port 1234. Increase
+  // kPortNum until we found free port.
+  while (bind(sockfd, (struct sockaddr *)&serveraddr, sizeof(serveraddr)) < 0) {
+    ++kPortNum;
+    serveraddr.sin_port = htons(kPortNum);
+  }
+  // Release the mutex since the server is ready now and kPortNum has proper
+  // value.
+  pthread_mutex_unlock(&server_ready_mu);
   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
   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);
   }
 
   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);
   }
 
   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);
+
+  // Wait until the server found free port and initialized kPortNum with proper
+  // value.
+  pthread_mutex_lock(&server_ready_mu);
   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);
   }
 
   return 0;
 }