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

[Argyrios Kyrtzidis]

On May 20, 2011, at 11:49 AM, Matthieu Monrocq wrote:

> 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.

Committed in r131747, thanks!

-Argyrios

> 
> -- Matthieu
> <llvm_stringref_undefined_behavior.diff>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110520/ac73702a/attachment.html>