This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Make strlen range computations more conservative


On Tue, 24 Jul 2018, 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.
> 
> 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.
> 
> 
> 
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?

I'd like us to be explicit in what we support, not what we do not
support, thus

+             /* Avoid arrays of pointers.  */
+             if (TREE_CODE (TREE_TYPE (arg)) == POINTER_TYPE)
+               return false;

should become

   /* We handle arrays of integer types.  */
  if (TREE_CODE (TRE_TYPE (arg)) != INTEGER_TYPE)
    return false;

+             tree base = arg;
+             while (TREE_CODE (base) == ARRAY_REF
+                    || TREE_CODE (base) == ARRAY_RANGE_REF
+                    || TREE_CODE (base) == COMPONENT_REF)
+               base = TREE_OPERAND (base, 0);
+
+             /* If this looks like a type cast don't assume anything.  */
+             if ((TREE_CODE (base) == MEM_REF
+                  && (! integer_zerop (TREE_OPERAND (base, 1))
+                      || TREE_TYPE (TREE_TYPE (TREE_OPERAND (base, 0)))
+                         != TREE_TYPE (base)))
+                 || TREE_CODE (base) == VIEW_CONVERT_EXPR)
                return false;

likewise - you miss to handle BIT_FIELD_REF.  So, instead

  if (!(DECL_P (base)
        || TREE_CODE (base) == STRING_CST
        || (TREE_CODE (base) == MEM_REF
            && ...

you should look at comparing TYPE_MAIN_VARIANT in your type
check, aligned/unaligned or const/non-const accesses shouldn't
be considered a "type cast".  Maybe even use
types_compatible_p.  Not sure why you enforce zero-offset MEMs?
Do we in the end only handle &decl bases of MEMs?  Given you
strip arbitrary COMPONENT_REFs the offset in a MEM isn't
so much different?

It looks like the component-ref stripping plus type-check part
could be factored out into sth like get_base_address?  I don't
have a good name or suggested semantics for it though.

Richard.


> 
> Thanks
> Bernd.

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]