This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] Make strlen range computations more conservative
On 07/24/18 23:46, Jeff Law wrote:
> On 07/24/2018 01:59 AM, Bernd Edlinger wrote:
>> This patch makes strlen range computations more conservative.
>> Firstly if there is a visible type cast from type A to B before passing
>> then value to strlen, don't expect the type layout of B to restrict the
>> possible return value range of strlen.
> Why do you think this is the right thing to do? ie, is there language
> in the standards that makes you think the code as it stands today is
> incorrect from a conformance standpoint? Is there a significant body of
> code that is affected in an adverse way by the current code? If so,
> what code?
I think if you have an object, of an effective type A say char, then
you can cast the address of A to B, say typedef char (*B) for instance
and then to const char *, say for use in strlen. I may be wrong, but I think
that we should at least try not to pick up char from B, but instead
use A for strlen ranges, or leave this range open. Currently the range
info for strlen is [0..1] in this case, even if we see the type cast
in the generic tree.
One other example I have found in one of the test cases:
if (strlen(&c) != 0) abort();
this is now completely elided, but why? Is there a code base where
that is used? I doubt, but why do we care to eliminate something
stupid like that? If we would emit a warning for that I'm fine with it,
But if we silently remove code like that I don't think that it
will improve anything. So I ask, where is the code base which
gets an improvement from that optimization?
>> Furthermore use the outermost enclosing array instead of the
>> innermost one, because too aggressive optimization will likely
>> convert harmless errors into security-relevant errors, because
>> as the existing test cases demonstrate, this optimization is actively
>> attacking string length checks in user code, while and not giving
>> any warnings.
> Same questions here.
> I'll also note that Martin is *very* aware of the desire to avoid
> introducing security relevent errors. In fact his main focus is to help
> identify coding errors that have a security impact. So please don't
> characterize his work as "actively attacking string length checks in
> user code".
I do fully respect Martin's valuable contributions over the years,
and I did not intend to say anything about the quality of his work,
for GCC, it is just breathtaking!
What I meant is just, what this particular optimization can do.
> Ultimately we want highly accurate string lengths to help improve the
> quality of the warnings we generate for potentially dangerous code.
> These changes seem to take us in the opposite direction.
No, I don't think so, we have full control on the direction, when
I do what Richi requested on his response, we will have one function
where the string length estimation is based upon, instead of several
open coded tree walks.
> So ISTM that you really need a stronger justification using the
> standards compliance and/or real world code that is made less safe by
> keeping string lengths as accurate as possible.
This work concentrates mostly on avoiding to interfere with code that
actually deserves warnings, but which is not being warned about.
>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
> I'd like to ask we hold on this until I return from PTO (Aug 1) so that
> we can discuss the best thing to do here for each class of change.
> I think you, Martin, Richi and myself should hash through the technical
> issues raised by the patch. Obviously others can chime in, but I think
> the 4 of us probably need to drive the discussion.
Yes, sure. I will try to help when I can.
Currently I thought Martin is working on the string constant folding,
(therefore I thought this range patch would not collide with his patch)
and there are plenty of change requests, plus I think he has some more
patches on hold. I would like to see the review comments resolved,
and maybe also get to see the follow up patches, maybe as a patch
series, so we can get a clearer picture?