On 5/14/24 8:57 AM, Qing Zhao wrote:
On May 13, 2024, at 20:14, Kees Cook <keesc...@chromium.org> wrote:
On Tue, May 14, 2024 at 01:38:49AM +0200, Andrew Pinski wrote:
On Mon, May 13, 2024, 11:41 PM Kees Cook <keesc...@chromium.org> wrote:
But it makes no sense to warn about:
void sparx5_set (int * ptr, struct nums * sg, int index)
{
if (index >= 4)
warn ();
*ptr = 0;
*val = sg->vals[index];
if (index >= 4)
warn ();
*ptr = *val;
}
Because at "*val = sg->vals[index];" the actual value range tracking for
index is _still_ [INT_MIN,INT_MAX]. (Only within the "then" side of the
"if" statements is the range tracking [4,INT_MAX].)
However, in the case where jump threading has split the execution flow
and produced a copy of "*val = sg->vals[index];" where the value range
tracking for "index" is now [4,INT_MAX], is the warning valid. But it
is only for that instance. Reporting it for effectively both (there is
only 1 source line for the array indexing) is misleading because there
is nothing the user can do about it -- the compiler created the copy and
then noticed it had a range it could apply to that array index.
"there is nothing the user can do about it" is very much false. They could
change warn call into a noreturn function call instead. (In the case of
the Linux kernel panic). There are things the user can do to fix the
warning and even get better code generation out of the compilers.
This isn't about warn() not being noreturn. The warn() could be any
function call; the jump threading still happens.
When the program is executed on the “if (index > = 4)” path, the value of
“index” is definitely
= 4, when sg->vals[index] is referenced on this path (the case when the routine
“warn” is NOT noreturn), it’s
definitely an out-of-bounds array access. So, the compiler’s warning is
correct. And this warning does catch
a potential issue in the source code that need to be fixed by either of the
following two solutions:
1. Make the routine “warn” as noreturn and mark it noreturn;
This would be my recommendation. We're about to execute undefined
behavior. I don't see a way to necessarily recover safely here, so I'd
suggest having warn() not return and mark it appropriately.
That'll have numerous secondary benefits as well.
jeff
