Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

ac000
Copy link
Member

@ac000 ac000 commented Sep 24, 2024

There has been a long standing clang-analyzer issue in nxt_process_check_pid_status(), well ever since I introduced this function in commit b0e2d9d ("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.

@ac000 ac000 marked this pull request as ready for review September 24, 2024 15:56
@ac000 ac000 requested review from hongzhidao and avahahn September 24, 2024 15:57
@javorszky
Copy link
Contributor

javorszky commented Sep 24, 2024

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
Copy link
Member Author

ac000 commented Sep 24, 2024

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
Copy link
Contributor

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
Copy link
Member Author

ac000 commented Sep 24, 2024

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
Copy link
Member Author

ac000 commented Sep 24, 2024

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
Copy link
Member Author

ac000 commented Sep 24, 2024

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
Copy link
Member Author

ac000 commented Sep 24, 2024

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)

There has been a long standing clang-analyzer issue in
nxt_process_check_pid_status(), well ever since I introduced this
function in commit b0e2d9d ("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.

Signed-off-by: Andrew Clayton <[email protected]>
@ac000
Copy link
Member Author

ac000 commented Sep 25, 2024

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

@ac000 ac000 merged commit 0e4342f into nginx:master Sep 25, 2024
24 checks passed
@ac000 ac000 deleted the proc branch September 25, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants