[prev in list] [next in list] [prev in thread] [next in thread] 

List:       wine-devel
Subject:    Re: [PATCH 2/4] winex11: Avoid inefficiency and overflow in remove_startup_notification.
From:       Vincent Povirk <vincent () codeweavers ! com>
Date:       2015-10-31 19:03:10
Message-ID: CAG_NDKopJfYOtXTB=8WBcThy1+yF2GZbOF-PDRbd85C3Mcadxw () mail ! gmail ! com
[Download RAW message or body]

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.


[prev in list] [next in list] [prev in thread] [next in thread]