False-positive: code analyzer does not know what "accept()/recvfrom()" does [initializing arguments by pointer]

[alavrentiev]

Consider the following code snippet:

struct sockaddr_in addr;
socklen_t addrlen = sizeof(addr);
sock = accept (fd, (struct sockaddr*) &addr, &addrlen);
if (sock < 0) {
// 14←Assuming 'sock' is >= 0
  return False;
}
... /* nothing touches "addr" in the interim */
host = addr.sin_addr.s_addr;
// 18←Assigned value is garbage or undefined

At "18" a bogus report is flagged. Even if code analyzer did not know what the accept() syscall does, giving the function (which can't be previewed at the source code level) a benefit of the doubt, it should be considered that the call did initialize its argument addr passed by the pointer, thus validating the entire contents of it. No garbage.

The same happens for the recvfrom() call:

if (recvfrom(s_Sock[0], buf, sizeof(buf), 0, (struct sockaddr*) &sin, &sinlen) > 0) {
 ...
 from = sin.sin_addr.s_addr;
 // 13←Assigned value is garbage or undefined
}

BTW, using a different technique of dealing with accept() peculiarity, causes the same bogus report:

union {
  struct sockaddr     sa;
  struct sockaddr_in  in;
  struct sockaddr_in6 in6;
  struct sockaddr_un  un;
} u;
...
fd = accept (sock, &u.sa, &addrlen);
...
assert(u.un.sun_family == AF_UNIX);
// 28←The left operand of '==' is a garbage value

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

Author: None (alavrentiev)

Consider the following code snippet: ``` struct sockaddr_in addr; socklen_t addrlen = sizeof(addr); sock = accept (fd, (struct sockaddr*) &addr, &addrlen); if (sock < 0) { // 14←Assuming 'sock' is >= 0 return False; } ... /* nothing touches "addr" in the interim */ host = addr.sin_addr.s_addr; // 18←Assigned value is garbage or undefined ``` At "18" a bogus report is flagged. Even if code analyzer did not know what the `accept()` syscall does, giving the function (which can't be previewed at the source code level) a benefit of the doubt, it should be considered that the call did initialize its argument `addr` passed by the pointer, thus validating the entire contents of it. No garbage.

The same happens for the recvfrom() call:

if (recvfrom(s_Sock[0], buf, sizeof(buf), 0, (struct sockaddr*) &sin, &sinlen) > 0) {
 ...
 from = sin.sin_addr.s_addr;
 // 13←Assigned value is garbage or undefined
}

BTW, using a different technique of dealing with accept() peculiarity, causes the same bogus report:

union {
  struct sockaddr     sa;
  struct sockaddr_in  in;
  struct sockaddr_in6 in6;
  struct sockaddr_un  un;
} u;
...
fd = accept (sock, &u.sa, &addrlen);
...
assert(u.un.sun_family == AF_UNIX);
// 28←The left operand of '==' is a garbage value

[NagyDonat]

I searched for these function names within the analyzer source code and I found that they are recognized by both GenericTaintChecker.cpp and StdLibraryFunctionsChecker.cpp, but those checkers don't override the conservative evaluation (which models the fact that the value stored in the memory referenced by the pointer argument may be updated and initialized by the function call). (GenericTaintChecker doesn't have an eval::Call callback, while StdLibraryFunctionsChecker does have eval::Call but marks these functions with NoEvalCall.)

[steakhal]

My gut feeling suggests it may be related to the fact that this is a union, and we can't track stores in unions.

[alavrentiev]

@steakhal :

that this is a union

As shown above, the bogus report is produced when it is not handled as a union, as well as when it is handled as such (snippet 3).

we can't track stores in unions

If something cannot be tracked, then it should not be assumed "garbage". This creates unnecessary noise (which there is plenty already and yet which can be easily avoided). The benefit of the doubt is the best policy in this case.

[NagyDonat]

If something cannot be tracked, then it should not be assumed "garbage". This creates unnecessary noise (which there is plenty already and yet which can be easily avoided).

You're right, I don't think that this would be the intentional policy for unions – however, unions are a rarely used language feature which doesn't "fit" well into our data model, so it's natural to guess that they cause bugs in some corner cases.

However, as your examples show that the issue can appear without unions, this bug is probably not caused by them.