[systemd-devel] [PATCH] journal: fix assert against (theoretical) undefined behavior

[Thomas H.P. Andersen]

On Mon, Dec 16, 2013 at 8:15 PM, Shawn Landden <shawn at churchofgit.com> wrote:
> On Mon, Dec 16, 2013 at 11:06 AM, Thomas H.P. Andersen <phomes at gmail.com> wrote:
>> On Mon, Dec 16, 2013 at 7:50 PM, Shawn Landden <shawn at churchofgit.com> wrote:
>>> On Mon, Dec 16, 2013 at 10:36 AM, Lennart Poettering
>>> <lennart at poettering.net> wrote:
>>>> On Mon, 16.12.13 09:20, Shawn Landden (shawn at churchofgit.com) wrote:
>>>>
>>>>> While all the libc implementations I know return NULL when memchr's size
>>>>> parameter is 0:
>>>>>
>>>>> C11 7.24.1p2: Where an argument declared as "size_t n" specifies the length
>>>>> of the array for a function, n can have the value zero on a call to that
>>>>> function. 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. On such a call, a
>>>>> function that locates a character finds no occurrence, a function that
>>>>> compares two character sequences returns zero, and a function that copies
>>>>> characters copies zero characters.
>>>>>
>>>>> see http://llvm.org/bugs/show_bug.cgi?id=18247
>>>>
>>>> Hmm? But what does that have to do with the requirements we make on our
>>>> own internal functions?
>>> Well, it makes clang's scan-build shut up :), which is how i initially
>>> came across it.
>>
>> Unfortunately there are quite a lot of false positives in scan-build.
>> I have been cleaning it up for a while. I did come across this one
>> earlier. Isn't the issue here that scan-build makes the wrong
>> assumption that data can be null at that point? Generally it gets
>> confused sometimes about functions that return a non-negative value
>> only when setting some data succeeded. At least I think that was the
>> conclusion I came to about the report.
> While there are certainly those (thinking walking a linked-list is
> use-after free,
> _cleanup_ issues: http://llvm.org/bugs/show_bug.cgi?id=3888 )
> this was a real bug that scan_build found,  but which I don't totally agree is
> important. If cause that there wasn't assert() against the first argument to
> memchr() being NULL, instead there was an assert against the first argument
> not being NULL OR size being 0, which (I was corrected to accept) can
> invoke undefined behavior.
After rereading it I see that I was mistaken. I had thought that data
would already be set at that point if size == 0. Sorry for the
confusion.

Would it be enough to just add:
if (data == NULL && size == 0)
  eq = false;
else
  eq = memchr(data, '=', size);