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

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

The following reply was made to PR kern/58425; it has been noted by GNATS.

From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: kre%NetBSD.org@localhost, gnats-admin%NetBSD.org@localhost, netbsd-bugs%NetBSD.org@localhost
Subject: Re: kern/58425: negation of pid or pgid is UB for INT_MIN
Date: Sun, 14 Jul 2024 13:55:11 +0000

 > 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).