2015-11-02 9:54 GMT-07:00 Vincent Povirk vincent@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];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