Re: [PATCH] Make strlen range computations more conservative

[Richard Biener <rguenther at suse dot de>]

On Tue, 24 Jul 2018, Jeff Law wrote:

> On 07/24/2018 01:59 AM, Bernd Edlinger wrote:
> > Hi!
> > 
> > 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?
> 
> 
> 
> > 
> > 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".
> 
> 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.
> 
> 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.

Note you cannot solely look at what the C standard says.  Instead
you have to see where the middle-end lessens that constraints since
these functions are not only called from C FE context.

So arguing from a C language lawyer point here is pointless.  You
have to argue from a GENERIC language lawyer point which is going
to be impossible since apart from the implementation (which
includes how all existing GCC FEs _and_ the middle-end uses it)
there is no formal specification available.  Yes, in most cases
we say we match C language behavior and constraints but in some
cases we are clearly and definitely less strict and that has
followup consequences in other areas.

While exploiting all fine details of the C language might get
you more constraints on things like string lengths you have to
apply some common sense and middle-end knowledge - which means
from my side trusting hunch feelings whether something is
possibly not safe.

As for patches in this area I would really love to see _smaller_
changes.  And I'd like to see changes that make it clear
what cases where supposed to be handled and _not_ including
other cases "by accident".  See especially my comments on this patch
from Bernd.

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

I think with respect to patches to fix issues in previous patches
at this point a better option might be to revert the patches causing
the issues and start from scratch in a more defined manner.

Giving recent (temporary) regressions in the testsuite it feels like
Martin is going too fast.

Richard.

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)