Re: Problems with dhcpcd

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Mon, 09 Oct 2023 12:41:30 +0100
    From:        Roy Marples <roy%marples.name@localhost>
    Message-ID:  <18b143e0358.f3fff8a9957587.8373038006501947224%marples.name@localhost>

I haven't read the patched (or unpatched) code, but this change makes no
sense to me:

  | diff --git a/src/script.c b/src/script.c
  | index 2ef99e38..69297a46 100644
  | --- a/src/script.c
  | +++ b/src/script.c
  | @@ -681,6 +681,21 @@ send_interface(struct fd_list *fd, const struct interface *ifp, int af)
  |  	return retval;
  |  }
  |  
  | +static int
  | +script_status(const char *script, int status)
  | +{
  | +
  | +	if (WIFEXITED(status)) {
  | +		if (WEXITSTATUS(status))
  | +			logerrx("%s: %s: WEXITSTATUS %d",
  | +			    __func__, script, WEXITSTATUS(status));
  | +	} else if (WIFSIGNALED(status))
  | +		logerrx("%s: %s: %s",
  | +		    __func__, script, strsignal(WTERMSIG(status)));
  | +
  | +	return WEXITSTATUS(status);
  | +}
  | +

That final return looks to meas if that final line is just "return 0"
written in a long winded way.

That's assuming that the status can't indicate some other untested
possibilities, about which I can't tell if they might apply, eg: if the
wait() status is indicating "stopped" or "continued" the possibility of
which depends upon which wait*() function generated the status and what
options were used with it (if the call has them)).

It is also assuming that logerrx() is like a call to errx() with syslog()
logging, or something similar ... the logging part isn't important
here, but the "errx()" which generally should mean "no return".

In that case, if we're not dealing with stopped or continued notifications,
then either WIFEXITED() or WIFSIGNALED() is true, if it is the latter,
this func doesn't return, so the final line isn't relevant.  If it was
the former, then if WEXITSTATUS(status) is not 0, then it doesn't return
either, so the only way we get to that final return is if WEXITSTATUS(status)
is 0 (or if the status came from a wait() which indicated stopped or
continued, in which case using WEXITSTATUS() would simply be wrong - similarly
if logerrx() doesn't imply an exit(), then that final WEXITSTATUS()
is just wrong - that macro only ever applies if WIFEXITED()  is true.

That's kind of fortunate, as otherwise:
  [I removed the lines that are being deleted by the patch here,
   as those just confuse things]

  | @@ -699,13 +714,7 @@ script_run(struct dhcpcd_ctx *ctx, char **argv)
  |  				break;
  |  			}
  |  		}
  | +		status = script_status(argv[0], status);
  |  	}
  |  
  |  	return WEXITSTATUS(status);

that would be completely bogus, and the final return there in the case
that script_status() was called, would certainly be return 0 as the
status returned by script_status() is the 8 bit exit code, and
WEXITSTATUS() does: ((int)(((unsigned int)_W_INT(x)) >> 8) & 0xff)
(_W_INT() is just a horrid cast of 'x' to an int to handle the case
where x is a union wait - which is something we should simply retire,
but haven't yet) - in any case in this case, that would be an 8 bit value
which is shifted right 8 bits...  that's 0 (which is only working here
because script_status() is only ever returning 0 as its value).

And then here (again with parts of the patch removed):

  | @@ -763,9 +772,13 @@ script_runreason(const struct interface *ifp, const char *reason)
  |  
  |  #ifdef PRIVSEP
  |  	if (ctx->options & DHCPCD_PRIVSEP) {

  | +		err = ps_root_script(ctx, ctx->script_buf, (size_t)buflen);
  | +		if (err == -1)
  |  			logerr(__func__);
  | +		else
  | +			script_status(ctx->script, (int)err);

the code is calling a func (script_status) that returns an int and ignoring
the value returned, which most current compilers will issue a warning about,
which could easily be avoided.

I'd suggest making script_status() a void func, just delete that final
return line, and remove the "status =" from the first call of it, leaving
'status' there as the original wait status (rather than changing the meaning
of that variable, which is then acceptable to apply WEXITSTATUS() to in the
return that follows (provided that we know that if we get that far, the
wait() must have been indicating an exit() and not some other event).

kre