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

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

It is possible for an application to do kill(INT_MIN, sig) and pass
through INT_MIN as the pid, which being negative, is treated as a
pgrp, negated.   But assuming -INT_MIN actually becomes -INT_MIN
or something else, not a trap, that would be harmless, it simply results
in a search for something different -- if it remains INT_MIN, the normal
case for most 2's complement architectures, or anything else negative,
it cannot be found, as all pids (and hence all pgrp ids) are positive
(or 0 for the kernel) a negative value (any negative value) can never
match (that's how this hack was invented).   So the result would have
just been ESRCH anyway I believe.

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.

My test build looks to have succeeded (on amd64) - currently building
the sets - I will do an i386 build, and then commit the changes, and
after a few says, see about pullups to -10 and -9.

The only system place outside the kernel I could see was in the killpg()
function (compat function, for old BSD code, posix defines the kill(-pid)
method) - that one will be fixed as well.

Random applications or other libraries which might accept a pid, and
negate it, will need to wait for some other time (if there are any).

kre

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