PR109439 - Terminate analysis path when OOB detected

PR109439 - Terminate analysis path when OOB detected

[Benjamin Priour]

[-- Attachment #1: Type: text/plain, Size: 2000 bytes --]

Hi David,

Sorry for not being active these past two weeks I've been overwhelmed at my
university with creating a new club and other uni stuff.

I just went back to solving these 3 bugs we discussed last time.

<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109439>
PR109439 <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109439> is the one
about the double warnings emitted (both OOB and use of uninitialized value).
Your second suggestion of terminating the diagnostic path on an OOB proved
to interfere with PR109437.
I might actually close PR109437 as solving PR109439 will probably solve it
too.
I'm going with your first suggestion of bubbling a boolean from
check_region_bounds up to get_rvalue.
I'm considering two approaches
 1. preventing the check_for_poison call if there is an OOB Read.
 2. or marking the OOB values as Unknown rather than Poisoned, but then we
are semantically incorrect.

Another unrelated question, I felt like the use of an uninitialized value
terminating the path was a bit strong. No other warnings will be considered
for the remaining of the function if there is such use, even for unrelated
stuff. Like a double free on a completely different variable.
Couldn't we tune that so we only ignore everything related to any variable
tainted by this uninitialized value ?

Sorry again for the past weeks, the club is finally running (somewhat).
Benjamin.

PS: I submitted a patch, bootstrapped and regtested, for the bug I was
solving on gcc-request. I guess I'm not too clear on the process of
submitting a patch, as I probably had to commit and push it afterwards,
sadly there was no feedback on the previous RFC as well as on the patch
submission - no blaming at all, people are busy and the flow of mails is
massive.
I believe I still don't have the right to commit it directly to the repo,
and honestly even if I would using my fresh gcc account, I would prefer not
to commit it myself for the first patch, I don't wanna mess with anything
because of an oversight.

Re: PR109439 - Terminate analysis path when OOB detected

[Mark Wielaard]

Hi Benjamin,

On Mon, May 01, 2023 at 02:43:13PM +0200, Benjamin Priour via Gcc wrote:
> I believe I still don't have the right to commit it directly to the repo,
> and honestly even if I would using my fresh gcc account, I would prefer not
> to commit it myself for the first patch, I don't wanna mess with anything
> because of an oversight.

An account has been setup for you, you can try it out by doing:

  ssh vultkayn@gcc.gnu.org alive

Some other helpful info can be found at:

  https://sourceware.org/sourceware/accountinfo.html

If anything seems wrong, please contact admin-requests@sourceware.org

And please coordinate actually pushing patches with David and the
other gcc developers on gcc-patches@gcc.gnu.org

Cheers,

Mark

Re: PR109439 - Terminate analysis path when OOB detected

[David Malcolm]

On Mon, 2023-05-01 at 14:43 +0200, Benjamin Priour wrote:
> Hi David,

Hi Benjamin

> 
> Sorry for not being active these past two weeks I've been overwhelmed
> at my
> university with creating a new club and other uni stuff.

Fair enough.

> 
> I just went back to solving these 3 bugs we discussed last time.
> 
> <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109439>
> PR109439 <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109439> is the
> one
> about the double warnings emitted (both OOB and use of uninitialized
> value).
> Your second suggestion of terminating the diagnostic path on an OOB
> proved
> to interfere with PR109437.
> I might actually close PR109437 as solving PR109439 will probably
> solve it
> too. 

Aha - so is the "uninit" warning terminating the path?  That could
explain it.

> I'm going with your first suggestion of bubbling a boolean from
> check_region_bounds up to get_rvalue.
> I'm considering two approaches
>  1. preventing the check_for_poison call if there is an OOB Read.
>  2. or marking the OOB values as Unknown rather than Poisoned, but
> then we
> are semantically incorrect.

What do you mean by "semantically incorrect" here?  If we have e.g.:

int would_like_only_oob ()
{
    int arr[] = {1,2,3,4,5,6,7};
    int y1 = arr[9]; // 2 warnings instead of only OOB warning are emitted here
    return y1;
} 

then we emit the OOB warning at the get_rvalue that accesses arr[9],
but we do need some svalue instance to return back from get_value, so
that we have something to put into "y1" in the store (or else y1 will
be falsely considered as uninit at the "return y1;".  The
unknown_svalue for 'int' seems to me to be the most appropriate svalue
to use here, but maybe there's some issue I haven't thought of with
that.

> 
> Another unrelated question, I felt like the use of an uninitialized
> value
> terminating the path was a bit strong. No other warnings will be
> considered
> for the remaining of the function if there is such use, even for
> unrelated
> stuff. Like a double free on a completely different variable.
> Couldn't we tune that so we only ignore everything related to any
> variable
> tainted by this uninitialized value ?

Maybe; FWIW, I implemented this termination logic in
8f6369157917a4371b5d66dfe82b84aded3b8268, which has some notes on my
reasoning.

> 
> Sorry again for the past weeks, the club is finally running
> (somewhat).
> Benjamin.
> 
> PS: I submitted a patch, bootstrapped and regtested, 

Great!

When you say "submitted", did you post it to a mailing list?

> for the bug I was
> solving on gcc-request.

Sorry, I'm not sure  what you mean by "gcc-request" here.

>  I guess I'm not too clear on the process of
> submitting a patch, as I probably had to commit and push it
> afterwards,
> sadly there was no feedback on the previous RFC as well as on the
> patch
> submission - no blaming at all, people are busy and the flow of mails
> is
> massive.

I'm swamped in email, and often miss things; sorry.  Please make sure
that stuff for the analyzer has my email address (as well as the
mailing list), in the recipient list, as it's more likely to be
detected by my filters.

If you haven't had a reply on a patch after a week, send a followup
with "PING" in the title.

Are you waiting on patch reviews from me?  If so, please resend
(sorry).

> I believe I still don't have the right to commit it directly to the
> repo,
> and honestly even if I would using my fresh gcc account, I would
> prefer not
> to commit it myself for the first patch, I don't wanna mess with
> anything
> because of an oversight.

I can talk you through the various steps as you familiarize yourself
with the process; thanks for your caution; sorry again if you're
waiting on me.

Dave