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++] = '\\';
}
} message[pos++] = '"'; message[pos++] = '\0';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.
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.