[cfe-commits] PATCH: Enhance array bounds checking

[Kaelyn Uhrain]

For clarification, which part of the restrictions I mentioned do you see as
an ommision in the logic that need to be mentioned in comments with the
patch to indicate they are intentional--given that Sema::CheckArrayAccess
explicitly only works with an Expr of ConstantArrayType and a second Expr
that must return true for isIntegerConstantExpr otherwise CheckArrayAccess
doesn't have the two values it needs to perform the check, and given there
is already a comment explaining the slight difference in acceptable values
depending on whether the index value is an array subscript or a
pointer-arithmetic offset?

On Thu, Jul 14, 2011 at 11:21 AM, Ted Kremenek <kremenek at apple.com> wrote:

> Okay, I think that is a solid argument.  I'd actually like those design
> restrictions to be directly reflected in comments with the patch.  That way
> it is clear that those restrictions are intentional, as oppose to an
> omission in the logic.
>
> On Jul 14, 2011, at 11:13 AM, Kaelyn Uhrain wrote:
>
> On Thu, Jul 14, 2011 at 10:28 AM, Ted Kremenek <kremenek at apple.com> wrote:
>
>> Hi Kaelyn,
>>
>> I was reviewing this patch (which I think is a great step), and I had a
>> high-level comment about the following test case:
>>
>> +void swallow (const char *x) { (void)x; }
>> +void test_pointer_arithmetic() {
>> +  const char hello[] = "Hello world!"; // expected-note 2 {{declared
>> here}}
>> +  const char *helloptr = hello;
>> +
>> +  swallow("Hello world!" + 6); // no-warning
>> +  swallow("Hello world!" - 6); // expected-warning {{refers before the
>> beginning of the array}}
>> +  swallow("Hello world!" + 14); // expected-warning {{refers past the end
>> of the array}}
>>
>> Do we really want this to be a warning?  There are plenty of examples
>> where an out-of-bounds pointer is computed for legit reasons.  As long as
>> that address is not dereferenced, there isn't necessarily a problem.  I'm
>> fearful this may generate a fair amount of noise on codebases that do
>> elaborate tricks with pointer offsets.  Indeed this very example doesn't
>> actually exhibit a "bug".
>>
>
> This patch came about from a Google-internal bug requesting a warning for
> pointer arithmetic with string constants that is guaranteed to be undefined.
> Given that the warning only triggers on pointer arithmetic involving a
> constant-sized array and a constant value (as that's the only time the
> compiler is guaranteed to know the result is invalid in regard to the array
> itself) if and only if the result goes more than one past the end of the
> array, I doubt there are many elaborate pointer offset tricks that would
> trigger this warning and are not inherently buggy. The resulting pointers
> would refer to arbitrary memory in or near the static program code/data or
> the stack since the arrays that could trigger this warning are pretty much
> by definition not allocated on the heap. Also, math on pointers (not arrays
> or constant strings) does not trigger the warning, as shown by the uses of
> helloptr in the test case--and at least in my limited experience, all of the
> fancy pointer arithmetic tricks I've seen have on pointers (possibly to
> constant arrays), not on literal arrays or array variables. Given the
> constraints on when the warning will be triggered for pointer arithmetic, I
> feel it does much more good than harm.
>
> Cheers,
> Kaelyn
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110714/aae2b506/attachment.html>