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 <rguent...@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
