<br><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
From: Matthieu Monrocq <<a href="mailto:matthieu.monrocq@gmail.com">matthieu.monrocq@gmail.com</a>><br>
Subject: [cfe-commits] Undefined Behavior when invoking StringRef<br>
constructor on a null pointer<br>
To: <a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
Message-ID: <<a href="mailto:BANLkTimhEP5e%2BRw3RsDZNYDqRncqDFotcw@mail.gmail.com">BANLkTimhEP5e+Rw3RsDZNYDqRncqDFotcw@mail.gmail.com</a>><br>
Content-Type: text/plain; charset="iso-8859-1"<br>
<br>
The llvm::StringRef class has a constructor taking a const char*<br>
parameter.<br>
<br>
This constructor is extremely simple, and I am afraid too simple. It<br>
directly invokes ::strlen on the parameter, without checking whether or<br>
not the pointer is null or not.<br>
<br>
Unfortunately as many C functions, strlen is not required by the standard to<br>
check its input, and indeed popular implementations assume that the input is<br>
not null, which results in undefined behavior.<br>
<br>
As far as I see it, there are two ways to deal with this:<br>
- using an assert, to check that the input is non-null. It does not<br>
slow-down the program built with asserts disabled, but does not allow us to<br>
invoke StringRef on null pointers<br>
- using a simple inlined test (ternary operator ?:) to either invoke strlen<br>
or set the length to 0. Makes migrating from const char* to<br>
llvm::StringRef easier.<br>
<br>
I've used the second approach in the patch enclosed (which gmail thoroughly<br>
refused to. I have not measured the performance impact though.<br>
<br>
Please review.<br>
Matthieu.<br></blockquote></div><br>Hum, looking further it seems to be only the top of the iceberg.<br><br>1. The default constructor makes Data null, so having another constructor doing so does not violate any invariant<br>
2. However I am afraid that not all functions guard agaisnt null properly<br><br>Even though llvm::StringRef::str has a special case for Data being null, many others (comparisons at least) use memcmp without this validation, and memcmp requires its arguments to be non-null (and we hit that bug on SLES 10 with memcpy recently at work, even with a null length the arguments should be non-null, otherwise undefined behavior is invoked).<br>
<br>Normally the C Standard precise that all functions taking pointers should be provided with non-null pointers, unless otherwise precised in their specific description.<br><br>You can see the reference for memcpy on StackOverflow: <a href="http://stackoverflow.com/questions/5243012/is-it-guaranteed-to-be-safe-to-perform-memcpy0-0-0/5243068#5243068">http://stackoverflow.com/questions/5243012/is-it-guaranteed-to-be-safe-to-perform-memcpy0-0-0/5243068#5243068</a><br>
<br>And this mail thread (I should really find a copy of the C final draft...) suggests it also applies to memcmp: <a href="http://www.mail-archive.com/bug-gnulib@gnu.org/msg10149.html">http://www.mail-archive.com/bug-gnulib@gnu.org/msg10149.html</a><br>
<br><br>It seems to me that there are two roads:<br>- Removing this possibily of being null: simple enough, we just check it at construction and substitute an empty c-string instead<br>- Allowing it, but catching all the bugs associated<br>
<br>I am not confident in my ability to catch all uses of Data right now and to remember this in the future when maintaining the class.<br><br>The first road thus seems saner and I have implemented it in a second iteration of the patch.<br>
<br><br>However it seems that a number of utilities (among which llvm/lib/Support/CommandLine.cpp) differentiate between an empty c-string and a null pointer.<br><br>I don't mind trying to fix all those who relied on the fact that Data could be null, but there is also a chance that I'll miss some uses.<br>
<br>Thoughts ?<br><br>Matthieu.<br>