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

[Vincent Povirk]

>> 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.

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

> So, does all that make sense? Would you sign off on this patch if I
> resubmit it keeping the memset?

No, because I'm not convinced the loop is correct.