Re: [PATCH 2/4] winex11: Avoid inefficiency and overflow in remove_startup_notification.
31 Oct
2015
31 Oct
'15
7:03 p.m.
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.
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?
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.
2 Nov
2 Nov
4:15 p.m.
New subject: [PATCH 2/4] winex11: Avoid inefficiency and overflow in remove_startup_notification.
2015-10-31 13:03 GMT-06:00 Vincent Povirk <vincent(a)codeweavers.com>om>:
Because I'm checking pos every time that it increments by one.
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];
}
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? 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
4:54 p.m.
New subject: [PATCH 2/4] winex11: Avoid inefficiency and overflow in remove_startup_notification.
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];
}
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? 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.
7:17 p.m.
New subject: [PATCH 2/4] winex11: Avoid inefficiency and overflow in remove_startup_notification.
2015-11-02 9:54 GMT-07:00 Vincent Povirk <vincent(a)codeweavers.com>om>:
I don't think you need to restructure the code, just saying maybe it'd
make sense to split this.
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.
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];
}
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? 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
3101
days inactive
3103
days old
3 comments
2 participants
participants (2)
-
Alex Henrie -
Vincent Povirk