This is the mail archive of the mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2] Fix strstr bug with huge needles (bug 23637)

On 19/09/2018 09:24, Szabolcs Nagy wrote:
> On 19/09/18 16:59, Adhemerval Zanella wrote:
>> On 19/09/2018 08:31, Szabolcs Nagy wrote:
>>> On 17/09/18 11:37, Wilco Dijkstra wrote:
>>>> As reported in ,
>>>> the generic strstr in GLIBC 2.28 fails to match huge needles.  The optimized
>>>> AVAILABLE macro reads ahead a large fixed amount to reduce the overhead of
>>>> repeatedly checking for the end of the string.  However if the needle length
>>>> is larger than this, two_way_long_needle may confuse this as meaning the end
>>>> of the string and return NULL.  This is fixed by adding the needle length to
>>>> the amount to read ahead.
>>> i think this should be fixed and backported asap as it can
>>> easily cause user visible misbehaviour.
>>>> diff --git a/string/strstr.c b/string/strstr.c
>>>> index 33acdc5442d3e9031298a56e4f52a19e2ffa7d84..f74d7189ed1319f6225525cc2d32380745de1523 100644
>>>> --- a/string/strstr.c
>>>> +++ b/string/strstr.c
>>>> @@ -33,8 +33,9 @@
>>>>      #define RETURN_TYPE char *
>>>>    #define AVAILABLE(h, h_l, j, n_l)                       \
>>>> -  (((j) + (n_l) <= (h_l)) || ((h_l) += __strnlen ((void*)((h) + (h_l)), 512), \
>>>> -                             (j) + (n_l) <= (h_l)))
>>>> +  (((j) + (n_l) <= (h_l)) \
>>>> +   || ((h_l) += __strnlen ((void*)((h) + (h_l)), (n_l) + 512), \
>>>> +       (j) + (n_l) <= (h_l)))
>>> so the only diff is the +n_l in the strnlen limit.
>>> (same in strcasestr).
>>> i think we can safely assume that n_l + 512 does not
>>> overflow. (user object cannot be so close to SIZE_MAX,
>>> but may be glibc should document its assumptions about
>>> the maximum object size somewhere).
>>> the fix looks ok to me.
>> Good catch, I would prefer to use a saturated math here instead and not
>> make such assumptions (even though it would incur in a small performance
>> hit for a usercase unlikely to happen)
> note that even if all of the address space is available to
> userspace, it has to contain glibc .text and .data as well
> as a stack and the needle cannot cover all those.
> there is existing n_l + 256 elsewhere in the code as well.
> and needle and haystack have to overlap for needle to be that big
> and it's not clear if in that case this code path can be hit.
> if you still think it should be fixed then n_l | 512 works too,
> but i think this is not worth changing, i mentioned the
> problem because it's easier to review such code if glibc
> has an explicit statement that size+1M < SIZE_MAX or similar.

I agree that a possible overflow issue won't happen and I don't
have a strong opinion whether indeed changing the code to handle
would yield any gain. Your rationale is good enough to keep the
code as is.

As a side note, I think we should backport it to 2.28.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]