Re: kern/58425: negation of pid or pgid is UB for INT_MIN

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Date: Sun, 14 Jul 2024 10:52:19 +0700
> From: Robert Elz <kre%munnari.OZ.AU@localhost>
> 
>      Date:        Sat, 13 Jul 2024 19:55:00 +0000 (UTC)
>      From:        campbell+netbsd%mumble.net@localhost
>      Message-ID:  <20240713195500.A81291A923C%mollari.NetBSD.org@localhost>
>  
>    | This could be a theoretical issue or it could be a practical issue,
>  
> I believe that unless we have some architecture which traps when
> executing -INT_MIN rather than just generating INT_MIN as the result
> (which is conceivable, but I doubt anything supported will do that)
> then I suspect this is really a non-issue for practical purposes.
> Even if the architecture were to generate a random value (which UB
> allows) it would not be truly harmful ... an application is allowed
> to send any random value to these sys calls, and things must work,
> that the random value was generated by this operation, rather than
> some other way really doesn't matter.

The issue isn't so much that the architecture's SUB or NEG instruction
might trap or produce wild behaviour -- although in principle it
could, and not even in a pathological architecture design, just like
signed division INT_MIN/-1 traps on x86.

The more likely risk is that a _compiler_ may draw an inference from
an unprotected -pgid expression -- it may infer that along any control
flow paths that unconditionally pass through that expression, pgid
must not be INT_MIN, because if it were then the behaviour would be
undefined and we all know C programmers never write programs that
could lead to UB on the possible inputs.

So, for example, a compiler might optimize (a)

	*p = -pgid;
	return (pgid == INT_MIN);

into

	*p = -pgid;
	return false;

or it might optimize (b)

	return (-pgid == pgid);

into

	return (pgid == 0);

where you might expect it (or require it, with -fwrapv) to be
equivalent instead to

	return (pgid == 0 || pgid == INT_MIN);

I haven't seen (a) in practice but (b) is easy to exhibit:

https://godbolt.org/z/rzxjf38aY

> I am however in the process of validating the value, and not performing
> the undefined operation.   Note, that as (ktrace excepted, where the
> pid arg is actually defined to be an int) generally dealing with pid_t
> values, which we cannot assume are int ... it could be short (and once
> was, even earlier I think it might have been char, not that the name
> "pid_t" existed then, but that was a long long time ago) which would
> never give a problem, it gets promoted to int for both the -pid
> operation (and so could never be INT_MIN) and to pass as a parameter.
> Alternatively, pid_t might be wider than an int (could be long)
> in which case the value might be < INT_MIN, so I am making (most of)
> the tests check that the value is > INT_MIN not just != INT_MIN.

Thanks!  Maybe we should use -PID_MAX as the limit instead of INT_MIN,
to make it more resilient to (unlikely but possible) changes of the
type and easier to audit?

...Except PID_MAX apparently isn't actually the max, once we cross
~16k processes.  So maybe we should have a different name for the
_real_ maximum pid.

> ps: Taylor, if you feel we need ATF test cases for this, by all means,
> go ahead.

I added one for kill(INT_MIN, 0).  We should add more tests for ktrace
and for FIOSETOWN on a tty (uses tty.c path) and a pipe or socket
(uses kern_proc.c fsetown path).