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