[cfe-commits] StringRef: undefined behavior issues with null pointers

[Matthieu Monrocq]

2011/5/20 Argyrios Kyrtzidis <kyrtzidis at apple.com>

> (cc'ing cfe-commits)
>
> On May 20, 2011, at 11:04 AM, Matthieu Monrocq wrote:
>
> Hello,
>
> here is a second iteration of the patch
>
> 2011/5/19 Argyrios Kyrtzidis <kyrtzidis at apple.com>
>
>> (moved to cfe-commits)
>>
>>      /// Construct a string ref from a cstring.
>>      /*implicit*/ StringRef(const char *Str)
>> -      : Data(Str), Length(::strlen(Str)) {}
>> +      : Data(Str), Length() {
>> +        assert(Str && "StringRef cannot be built from a NULL argument");
>> +        Length = ::strlen(Str); // invoking strlen(NULL) is undefined
>> behavior
>> +      }
>>
>>
>>
>> "Length()" is not necessary.
>> Could you also add an assert in the  StringRef(const char *data, size_t
>> length) constructor asserting that data is not null or length is 0 ?
>>
>>
> Removed and Done.
>
>> +
>> +    // Workaround memcmp issue with null pointers (undefined behavior)
>> +    // by providing a specialized version
>> +    static int memcmp(const char *Lhs, const char *Rhs, size_t Length) {
>> +      if (Length == 0) { return 0; }
>> +      assert(Lhs && "memcmp - Lhs should be non-null when Length is not
>> 0");
>> +      assert(Rhs && "memcmp - Rhs should be non-null when Length is not
>> 0");
>> +      return ::memcmp(Lhs,Rhs,Length);
>> +    }
>> +
>>
>>
>> Is this really necessary ? With the 2 asserts in the constructors we are
>> making sure that StringRefs point to non-null or their length is zero, and
>> calling memcmp with zero length is defined, no ?
>>
>>
> I removed the two asserts since we now guarantee that Length is 0 if Data
> is null.
>
> I am afraid the check might be necessary, from n869 (a Draft of C99)
>
> > [7.21.1  String function conventions]
> > [#2] [...] Unless  explicitly  stated otherwise  in  the  description  of
> a particular function in this subclause, pointer arguments on such a call
> shall still have valid values, as described in 7.1.4. [...]
>
> [7.21.4.1  The memcmp function] does not state otherwise in any of its
> subclauses.
>
> I've hit the bug on Suse with the memcpy function (on a memcpy(NULL, NULL,
> 0) call) and am now paranoid about it.
>
>
> Ugh, that is good know.
> But could you please rename 'memcmp' to something else (e.g.
> 'compareMemory') ? I understand it was the choice with the least amount of
> changes, but it is confusing, in general, to have member functions with the
> same name as standard library functions.
>
> -Argyrios
>

Done!

There were only 4 call sites so not too invasive.

-- Matthieu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110520/9abdbc72/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm_stringref_undefined_behavior.diff
Type: application/octet-stream
Size: 3014 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110520/9abdbc72/attachment.obj>