Re-work nxt_process_check_pid_status() slightly (resolves a clang-analyzer (non-)issue)

[ac000]

There has been a long standing clang-analyzer issue in nxt_process_check_pid_status(), well ever since I introduced this function in commit b0e2d9d0a ("Isolation: Switch to fork(2) & unshare(2) on Linux."),

It is complaining that there are cases where 'status' could be returned with an undefined or garbage value.

Now I'm convinced this can't happen

  If nxt_process_pipe_timer() returns NXT_OK
    read(2) the status value

  If the read(2) failed or if we got NXT_ERROR from
  nxt_process_pipe_timer(), NXT_OK (0) and NXT_ERROR (-1) are the only
  possible return values here, then we set status to -1

I don't see a scenario where status is either not set by the read(2) or not set to -1.

Now I'm not a fan of initialising variables willy-nilly, however, in this case if we initialise status to -1, then we can simply remove the

  if (ret <= 0) {

check. So it closes this (non-)issue but also provides a little code simplification.

NOTE: There is no need to check the return from the read(2) here. We are reading a single byte, we either get it or don't.

[javorszky]

Is there a guarantee that *gc_pipe is not going to be null? From a static analysis point of view.

Within the function there's no nullcheck, and there's also no check to see whether what's at that address is what we expect there to be 🤔 . Could it be that coverity says "well this could be null, so the return of nxt_process_pipe_timer could be not NXT_OK, which means status was whatever the uninitialised value was.

If I understand it correctly, local non-static uninitialised variable is indeterminate: https://stackoverflow.com/questions/1597405/what-happens-to-a-declared-uninitialized-variable-in-c-does-it-have-a-value

[ac000]

Is there a guarantee that *gc_pipe is not going to be null? From a static analysis point of view.

Well let's see...

gc_pipe is an array declared in nxt_process_create()

    nxt_fd_t       pid_pipe[2], gc_pipe[2];                                     

At this point the two members of gc_pipe have undefined values.

It is then passed into nxt_process_init_pidns()

    ret = nxt_process_init_pidns(task, process, pid_pipe, gc_pipe, &use_pidns); 

From there it is passed into nxt_pipe_create() to actually create the pipe(2)

    ret = nxt_pipe_create(task, gc_pipe, 0, 0);                                 
    if (nxt_slow_path(ret == NXT_ERROR)) {                                      
        return NXT_ERROR;                                                       
    }

After returning, either the two members of gc_pipe[] contain the file descriptors for each end of the pipe, or we return NXT_ERROR which is handled and we return -1 from nxt_process_create()

So long story short, no, *gc_pipe can't be NULL...

[javorszky]

Hm, does that not mean that in order for gc_pipe to not be null we rely on calling those specific functions previously in that specific order?

In isolation there's nothing preventing any code addition in the future to pass a null pointer to nxt_process_check_pid_status besides knowing that it's a bad idea to do that. 🤔

[ac000]

Could it be that coverity says "well this could be null, so the return of nxt_process_pipe_timer could be not NXT_OK, which means status was whatever the uninitialised value was.

I don't actually see this issue in coverity only clang-analyzer.

But anyway the only possible return values of nxt_process_pipe_timer() are NXT_OK or NXT_ERROR, both of which were handled.

[ac000]

Hm, does that not mean that in order for gc_pipe to not be null we rely on calling those specific functions previously in that specific order?

The only way *gc_pipe can be NULL is if you actually pass NULL into the function and that's clearly wrong.

In isolation there's nothing preventing any code addition in the future to pass a null pointer to nxt_process_check_pid_status besides knowing that it's a bad idea to do that. 🤔

Right, this is an internal unit API, use it correctly! (In this if you did pass in NULL you'd very quickly know about it...)

It won't take you more than two seconds looking around the Unit code base to find more like this...

We do not need to check every single pointer passed into functions...

[ac000]

Within the function there's no nullcheck, and there's also no check to see whether what's at that address is what we expect there to be 🤔 . Could it be that coverity says "well this could be null, so the return of nxt_process_pipe_timer could be not NXT_OK, which means status was whatever the uninitialised value was.

Passing NULL into nxt_process_pipe_timer() as the first argument? That simply isn't possible either., the first parameter is an int.

[ac000]

This is completely outwith the scope of this PR but something we could consider, though I would need convincing it's a worthwhile exercise, would be to use the nonnull function attribute to enable compile time checks e.g.

#include <stddef.h>

static void f(void *p)
{
}

int main(void)
{
    f(NULL);

    return 0;
}

$ CFLAGS=-Wall make nn
cc -Wall    nn.c   -o nn

#include <stddef.h>

__attribute__((nonnull))
static void f(void *p)
{
}

int main(void)
{
    f(NULL);

    return 0;
}

$ CFLAGS=-Wall make nn
cc -Wall    nn.c   -o nn
nn.c: In function ‘main’:
nn.c:10:9: warning: argument 1 null where non-null expected [-Wnonnull]
   10 |         f(NULL);
      |         ^
nn.c:4:13: note: in a call to function ‘f’ declared ‘nonnull’
    4 | static void f(void *p)
      |             ^

(the attribute can also take a list of arguments position for nonnull pointers)

[ac000]

Rebased with master

$ git range-diff 15e6bad9...0e4342fa
-:  -------- > 1:  1c75aab3 tools/unitctl: use hyper-rustls instead of hyper-tls
-:  -------- > 2:  ac902548 tools/unitctl: bump bollard and clarify docker client error
1:  15e6bad9 = 3:  0e4342fa Re-work nxt_process_check_pid_status() slightly