[PATCH 2/4] winex11: Avoid inefficiency and overflow in remove_startup_notification.

[Alex Henrie]

2015-11-02 9:54 GMT-07:00 Vincent Povirk <vincent at codeweavers.com>:
>>> It seems like you're doing two unrelated things here? I'd expect the
>>> process of building the string to send and sending it to be
>>> independent.
>>
>> I don't think the other code we have that interacts with X11 is
>> similar enough to this to justify creating a helper code and moving
>> some of this code there. Also, I'm not trying to redesign the entire
>> startup notification feature, I'm just trying to fix a potential
>> buffer overflow while making this function a bit more efficient and
>> understandable.
>
> I don't think you need to restructure the code, just saying maybe it'd
> make sense to split this.
>
>>>>      if ((src = strstr( id, "_TIME" ))) update_user_time( atol( src + 5 ));
>>>>
>>>> -    pos = snprintf(message, sizeof(message), "remove: ID=");
>>>> -    message[pos++] = '"';
>>>> -    for (i = 0; id[i] && pos < sizeof(message) - 2; i++)
>>>> +    pos = sprintf(message, "remove: ID=\"");
>>>> +    for (i = 0; id[i]; i++)
>>>>      {
>>>>          if (id[i] == '"' || id[i] == '\\')
>>>> +        {
>>>> +            if (pos == sizeof(message) - 3) break;
>>>>              message[pos++] = '\\';
>>>> +        }
>>>> +        if (pos == sizeof(message) - 2) break;
>>>>          message[pos++] = id[i];
>>>>      }
>>>>      message[pos++] = '"';
>>>>      message[pos++] = '\0';
>>>
>>> Your use of == instead of > or >= in these checks makes me
>>> uncomfortable. Given that a single iteration can increment pos twice,
>>> why can't we have a situation where (pos > sizeof(message) - 3),
>>> inside the if statement?
>>
>> Because I'm checking pos every time that it increments by one.
>
> You're checking it, but not for the same value. Why can't we have this happen:
>  * id[i] is 'a', and pos == sizeof(message) - 3
>  * We check whether pos == sizeof(message) - 2, it isn't so we keep going.
>  * We write 'a' into message and increment pos, now it's sizeof(message) - 2
>  * We increment i and continue with the loop, now id[i] is '"'.
>  * We check whether pos == sizeof(message) - 3, it isn't, we write a \
> and increment pos to sizeof(message) - 1
>  * There's nothing to stop the loop from continuing when we run out of buffer.

Okay, I see what you mean now, thanks.

> A simpler approach might be to change the buffer sizes so that we
> can't overrun the second buffer.

After looking at it again, I agree that this is the right way to go.
If we make message 4 KiB, there is no chance of overflow.
Coincidentally, the spec
<http://standards.freedesktop.org/startup-notification-spec/startup-notification-latest.txt>
suggests that "4k of data" is the maximum that the X server will
accept anyway. I will upload a revised patch to
<https://github.com/alexhenrie/wine/commits/master> shortly and if
there are no objections, I'll send it tonight.

-Alex