2015-10-31 13:03 GMT-06: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.
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.
As for the memset, I didn't examine that as thoroughly, but it's a good policy to not send uninitialized data over the wire. Still, we could move the memset outside the loop and know that won't happen.
That's a good point, and not just because we don't accidentally want to send sensitive information over the wire: If the program is running over X11 forwarding, zeroing out unused bytes improves performance by improving the compression ratio. In the end, I think it's best to leave the memset how it was before (inside the loop), especially given that it's unusual for this loop to execute more than once.
So, does all that make sense? Would you sign off on this patch if I resubmit it keeping the memset?
-Alex