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

[Shawn Landden]

On Mon, Dec 16, 2013 at 12:19 PM, Thomas H.P. Andersen <phomes at gmail.com> wrote:
> 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;
NULL
> else
>   eq = memchr(data, '=', size);
or keep the existing assert()
and add
if (size == 0)
   eq = NULL;
else
   eq = memchr(data, '=', size);

> _______________________________________________
> systemd-devel mailing list
> systemd-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/systemd-devel