Re: Munge /r and /n in HTTP_HttpSendRequestA

[Rolf Kalbermatter]

Uwe Bonnes <bon(a)elektron.ikp.physik.tu-darmstadt.de> wrote:
> This makes Xilinx webupdate.exe see the Servicepack
>
> +	else
> +	    /* remove \r and \n*/
> +	{
> +	    int nLen = strlen(lpwhr->lpszPath);
> +	    while ((lpwhr->lpszPath[nLen-1] == '\r')||(lpwhr->lpszPath[nLen-1] == '\n'))
> +	    {
> +		nLen--;
> +		lpwhr->lpszPath[nLen]='\0';
> +	    }

Mmmh, what happens if the passed in path would only exist of \r\n? A for or while loop
without a guaranteed termination condition always seems suspect to me. I guess this
would crash with an access violation, and most probably overwrite a few bytes of memory
with 0 before that happens. Not good I think although I think it couldn't be effectively
exploited as buffer overrun!

   while (nLen > 0 && ((lpwhr->lpszPath[nLen-1] == '\r') || (lpwhr->lpszPath[nLen-1] == '\n')))

might be the better solution. Though I still wonder about the [nLen-1] in above term.
It seems [nLen] instead would be actually the right thing to do here which would make it 

   while (nLen >= 0 && ((lpwhr->lpszPath[nLen] == '\r') || (lpwhr->lpszPath[nLen] == '\n')))



Rolf Kalbermatter

Munge /r and /n in HTTP_HttpSendRequestA

[Uwe Bonnes]

> > > > > "Rolf" == Rolf Kalbermatter <rolf.kalbermatter(a)citeng.com> writes:

Rolf> Uwe Bonnes <bon(a)elektron.ikp.physik.tu-darmstadt.de> wrote:
    >> This makes Xilinx webupdate.exe see the Servicepack
    >> 
    >> + else + /* remove \r and \n*/ + { + int nLen =
    >> strlen(lpwhr->lpszPath); + while ((lpwhr->lpszPath[nLen-1] ==
    >> '\r')||(lpwhr->lpszPath[nLen-1] == '\n')) + { + nLen--; +
    >> lpwhr->lpszPath[nLen]='\0'; + }

    Rolf> Mmmh, what happens if the passed in path would only exist of \r\n?
    Rolf> A for or while loop without a guaranteed termination condition
    Rolf> always seems suspect to me. I guess this would crash with an
    Rolf> access violation, and most probably overwrite a few bytes of
    Rolf> memory with 0 before that happens. Not good I think although I
    Rolf> think it couldn't be effectively exploited as buffer overrun!

    Rolf>    while (nLen > 0 && ((lpwhr->lpszPath[nLen-1] == '\r') ||
    Rolf> (lpwhr->lpszPath[nLen-1] == '\n')))

    Rolf> might be the better solution. Though I still wonder about the
    Rolf> [nLen-1] in above term.  It seems [nLen] instead would be actually
    Rolf> the right thing to do here which would make it

    Rolf>    while (nLen >= 0 && ((lpwhr->lpszPath[nLen] == '\r') ||
    Rolf> (lpwhr->lpszPath[nLen] == '\n')))


The test for nLen is a good idea.  But 
with nLen = strlen(lpwhr->lpszPath); lpwhr->lpszPath[nLen] should give the
terminating NULL to my understanding...

Or am I off by one?

Bye

    Rolf> Rolf Kalbermatter

-- 
Uwe Bonnes                bon(a)elektron.ikp.physik.tu-darmstadt.de

Institut fuer Kernphysik  Schlossgartenstrasse 9  64289 Darmstadt
--------- Tel. [phone] -------- Fax. [phone] ----------