2015-11-02 9:54 GMT-07:00 Vincent Povirk <vincent(a)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