Re: [3/3] wininet: Add missing empty line to end of headers list

[Alexandre Julliard]

Jason Green <jave27(a)gmail.com> writes:
> @@ -4265,6 +4265,24 @@ static INT HTTP_GetResponseHeaders(LPWININETHTTPREQW lpwhr, BOOL clear)
>   }
>      }while(1);
>
> +    /* make sure the response header is terminated with an empty line.  Some apps really
> +       truly care about that empty line being there for some reason.  If it's not present,
> +       just add it to the header. */
> +    if (cchRawHeaders >= 4 && 
> +        (strcmpW(&lpszRawHeaders[cchRawHeaders - 4], szCrLf) || 
> +         strcmpW(&lpszRawHeaders[cchRawHeaders - 2], szCrLf)))
> +    {

This check doesn't seem necessary, I don't see how you could already
have an empty line in the buffer since we only store valid headers.

-- 
Alexandre Julliard
julliard(a)winehq.org

[3/3] wininet: Add missing empty line to end of headers list

[Jason Green]

On Fri, Jun 5, 2009 at 6:21 AM, Alexandre Julliard<julliard(a)winehq.org> wrote:
> Jason Green <jave27(a)gmail.com> writes:
> > @@ -4265,6 +4265,24 @@ static INT HTTP_GetResponseHeaders(LPWININETHTTPREQW lpwhr, BOOL clear)
> >       }
> >      }while(1);
> >
> > +    /* make sure the response header is terminated with an empty line.  Some apps really
> > +       truly care about that empty line being there for some reason.  If it's not present,
> > +       just add it to the header. */
> > +    if (cchRawHeaders >= 4 &&
> > +        (strcmpW(&lpszRawHeaders[cchRawHeaders - 4], szCrLf) ||
> > +         strcmpW(&lpszRawHeaders[cchRawHeaders - 2], szCrLf)))
> > +    {
>
> This check doesn't seem necessary, I don't see how you could already
> have an empty line in the buffer since we only store valid headers.

Hi Alexandre,

Here's the response from the patch author, Eric van Beurden:

This check was added because the login server for one of our games was
sending a final line that either contained only whitespace or was received
as an empty string and that was being discarded.  This left the headers list
without a terminating empty line (ie: ending in "\r\n\r\n").  The game then
went to retrieve and examine the CRLF raw headers and would just spin
endlessly because the terminating empty line was not present.  Adding this
check allowed the game to succeed on the login immediately.

Admittedly, this is more an issue with improper information being sent back
from the server, but it would be better to ensure the terminating empty line
is present in all cases instead of storing potentially invalid headers.

Perhaps a more detailed comment in there would help?

[3/3] wininet: Add missing empty line to end of headers list

[Alexandre Julliard]

Jason Green <jave27(a)gmail.com> writes:
> Hi Alexandre,
>
> Here's the response from the patch author, Eric van Beurden:
>
> This check was added because the login server for one of our games was
> sending a final line that either contained only whitespace or was received
> as an empty string and that was being discarded.  This left the headers list
> without a terminating empty line (ie: ending in "\r\n\r\n").  The game then
> went to retrieve and examine the CRLF raw headers and would just spin
> endlessly because the terminating empty line was not present.  Adding this
> check allowed the game to succeed on the login immediately.

Adding the termination is fine if an app needs it, my point is that your
if() check is useless, because there will never already be a termination
in the buffer at that point.

-- 
Alexandre Julliard
julliard(a)winehq.org