[cfe-commits] Undefined Behavior when invoking StringRef constructor on a null pointer

[Matthieu Monrocq]

Following Benjamin's remark, here is a 3rd iteration of the patch:

- allow  Data  to be null as it is the current behavior and unknown amount
of code relies on this property meaning that the StringRef is "void", but
assert that the pointer provided to  strlen  is non-null to avoid undefined
behavior and provide a clear diagnosis in case this condition is violated
- since  Data  can be null, provide a specialized version of  memcmp  (like
it was done for  min  and  max) that allows either pointer to be null
whenever the number of bytes to compare is zero

Arguably, the static function could be named otherwise (rawcompare ?) and
the calls to memcmp replaced.

This allows a straightforward use of the compare methods that does not
require the caller to check that either is null beforehand (which is quite
delicate in a call to  sort  or similar algorithm).

It should be transparent for those using the StringRef correctly already.


Please review.
Matthieu.


2011/4/30 Matthieu Monrocq <matthieu.monrocq at gmail.com>

> Hi Benjamin,
>
> well, it might not crash actually, that's the issue with undefined
> behavior, so I'll just add the assert.
>
> Thanks for the link :)
> Matthieu.
>
>
> 2011/4/30 Benjamin Kramer <benny.kra at googlemail.com>
>
>>
>> On 30.04.2011, at 12:18, Matthieu Monrocq wrote:
>>
>> > The  llvm::StringRef  class has a constructor taking a  const char*
>>  parameter.
>> >
>> > This constructor is extremely simple, and I am afraid too simple. It
>> directly invokes  ::strlen  on the parameter, without checking whether or
>> not the pointer is null or not.
>> >
>> > Unfortunately as many C functions, strlen is not required by the
>> standard to check its input, and indeed popular implementations assume that
>> the input is not null, which results in undefined behavior.
>> >
>> > As far as I see it, there are two ways to deal with this:
>> > - using an assert, to check that the input is non-null. It does not
>> slow-down the program built with asserts disabled, but does not allow us to
>> invoke StringRef on null pointers
>> > - using a simple inlined test (ternary operator ?:) to either invoke
>> strlen or set the length to 0. Makes migrating from  const char*  to
>>  llvm::StringRef  easier.
>> >
>> > I've used the second approach in the patch enclosed (which gmail
>> thoroughly refused to. I have not measured the performance impact though.
>>
>> Hi Matthieu,
>>
>> We had this discussion before <
>> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20090928/088120.html
>> >.
>>
>> The result was that we don't want to have a NULL check in StringRef's ctor
>> because it would slow down many users of StringRef and instead code that
>> passes NULL to StringRef should be fixed.
>>
>> An assert would be ok, but I don't think it's needed because strlen(NULL)
>> is going to crash anyway.
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110430/600d92d2/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm_stringref_ub.diff
Type: application/octet-stream
Size: 1373 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110430/600d92d2/attachment.obj>