Skip to content

Commit

Permalink
Re-work nxt_process_check_pid_status() slightly
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
ac000 committed Sep 25, 2024
1 parent ac90254 commit 0e4342f
Showing 1 changed file with 2 additions and 6 deletions.
8 changes: 2 additions & 6 deletions src/nxt_process.c
Original file line number Diff line number Diff line change
Expand Up @@ -370,18 +370,14 @@ nxt_process_pipe_timer(nxt_fd_t fd, short event)
static nxt_int_t
nxt_process_check_pid_status(const nxt_fd_t *gc_pipe)
{
int8_t status;
int8_t status = -1;
ssize_t ret;

close(gc_pipe[1]);

ret = nxt_process_pipe_timer(gc_pipe[0], POLLIN);
if (ret == NXT_OK) {
ret = read(gc_pipe[0], &status, sizeof(int8_t));
}

if (ret <= 0) {
status = -1;
read(gc_pipe[0], &status, sizeof(int8_t));
}

close(gc_pipe[0]);
Expand Down

0 comments on commit 0e4342f

Please sign in to comment.