Fix clang uninitialized variable for struct curltime

[bkuschel]

When compiling with clang-15 memory sanitizer:

Uninitialized value was created by an allocation of 'start' in the stack frame of function 'Curl_socketpair'

Maybe the special handling of returning a struct from a function uses the uninitialized reference to curltime as initializing it to zeros (by {}) prior to Curl_now() avoids the uninitialized value error.

[bagder]

I'm counting 71 places in the code that use = Curl_now(). It seems odd to change one of those because of a clang warning. Either they all are wrong or none of them are I think.

And I can't see how it is wrong. Can you? If this fixes the warning, then it smells like a bad warning...

[bagder]

Isn't it the check variable it refers to? So a more appropriate fix to silence this might be:

diff --git a/lib/socketpair.c b/lib/socketpair.c
index b94c9843e..67ce8da8d 100644
--- a/lib/socketpair.c
+++ b/lib/socketpair.c
@@ -123,11 +123,11 @@ int Curl_socketpair(int domain, int type, int protocol,
   (void)Curl_poll(pfd, 1, 1000); /* one second */
   socks[1] = accept(listener, NULL, NULL);
   if(socks[1] == CURL_SOCKET_BAD)
     goto error;
   else {
-    struct curltime check;
+    struct curltime check = {0, 0};
     struct curltime start = Curl_now();
     char *p = (char *)✓
     size_t s = sizeof(check);
 
     /* write data to the socket */

[bkuschel]

I think it legitimate, the question is whether it's worth fixing. The issue is that the curltime struct uses unaligned types (time_t/long and an int) so buffer is added to align the struct. By default, this buffer is unintialized so when reading the struct as memory, msan flags that uninitialized memory is being read.

Here is a small example:

#include <time.h>
#include <string.h>

struct curltime {
  time_t tv_sec; /* seconds */
  int tv_usec;   /* microseconds */
};

struct curltime Curl_now()
{
  struct curltime now;
  now.tv_sec = time(NULL);
  now.tv_usec = 0;
  return now;
}

int main(int argc, char** argv)
{
    struct curltime check = {};
    struct curltime start;
    start = Curl_now();
    if(memcmp(&start, &check, sizeof(check)))
        return 1;
    return 0;
}

Msan will report unintialized variable in this case.
To fix it you either need to align the struct types (both int, for example), or manually set the struct memory to 0, thereby initializing the buffer.

The fix I propose above is actually not great.. it relies on an optimization trick that converts {} into an memset(&start,0, sizeof(start)). Msan will still complain w/o optimization. The fix would be:

int main(int argc, char** argv)
{
    struct curltime check;
    memset(&check,0,sizeof(check));
    struct curltime start;
    memset(&start,0,sizeof(start));
    start = Curl_now();
    if(memcmp(&start, &check, sizeof(check)))
        return 1;
    return 0;
}

[bkuschel]

• struct curltime check = {0, 0};
  struct curltime start = Curl_now();

{} or {0,0} only work with optimization on, where this seems to set the entire struct to 0, including the buffer. This doesn't happen when optimization is off.

Also, MSAN is complaining about start in this scenario.

[bkuschel]

Another option that might work:

struct curltime {
  time_t tv_sec; /* seconds */
  int tv_usec;   /* microseconds */
  int _notused = 0;
};

[bagder]

socketpair.c: In function 'Curl_socketpair':
socketpair.c:133:5: error: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
     struct curltime start;
     ^~~~~~
socketpair.c:136:5: error: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
     char *p = (char *)✓
     ^~~~

[bagder]

I figure maybe a nicer approach would be to switch to a plain random number to completely avoid the struct align thing.