Add range check to allow for NULL to be counted as data or not by DHCP server.

Add range check to allow for NULL to be counted as data or not by DHCP server.

BUG=106851
TEST=On Windows, fire up Chrome and navigate to a couple of web pages, with proxy settings set to auto-detect.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114019

[Jói, 2011-12-09 15:33:55]

pastarmovj: Main reviewer.
eroman: net/OWNERS approval.

Cheers,
Jói

[pastarmovj, 2011-12-09 15:39:12]

LGTM. Thanks for fixing this!

[eroman, 2011-12-09 20:41:32]

http://codereview.chromium.org/8892004/diff/1/net/proxy/dhcp_proxy_script_ada...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/8892004/diff/1/net/proxy/dhcp_proxy_script_ada...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:280: return
std::string(reinterpret_cast<const char *>(wpad_params.Data));
Can we avoid relying on NULL string termination all-together? For instance
removing the debug block and writing:

std::string(reinterpret_cast<const char *>(wpad_params.Data),
wpad_params.nBytesData);

(If the length *does* include the null-termination byte, might have to trim the
last character though).

I couldn't find solid documentation on this either way, but the following
Microsoft example seems to imply you can't rely on the data being NULL
terminated (since they inject one):

http://msdn.microsoft.com/en-us/library/windows/desktop/aa363345(v=vs.85).aspx

            if( DhcpApiHostNameParams.nBytesData ) 
            {

                // Check size with dwHostNameBufSize.

                CopyMemory(
                     pszHostNameBuf, DhcpApiHostNameParams.Data,
                     DhcpApiHostNameParams.nBytesData
                     );
                pszHostNameBuf[DhcpApiHostNameParams.nBytesData] = '\0';
                return TRUE;
            }


LGTM whatever you decide.

[Jói, 2011-12-12 14:33:11]

Thanks Eric, I'm switching to a safer approach as a precaution.

Cheers,
Jói

http://codereview.chromium.org/8892004/diff/1/net/proxy/dhcp_proxy_script_ada...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/8892004/diff/1/net/proxy/dhcp_proxy_script_ada...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:280: return
std::string(reinterpret_cast<const char *>(wpad_params.Data));
On 2011/12/09 20:41:32, eroman wrote:
> Can we avoid relying on NULL string termination all-together? For instance
> removing the debug block and writing:
> 
> std::string(reinterpret_cast<const char *>(wpad_params.Data),
> wpad_params.nBytesData);
> 
> (If the length *does* include the null-termination byte, might have to trim
the
> last character though).
> 
> I couldn't find solid documentation on this either way, but the following
> Microsoft example seems to imply you can't rely on the data being NULL
> terminated (since they inject one):
> 
>
http://msdn.microsoft.com/en-us/library/windows/desktop/aa363345%28v=vs.85%29...
> 
>             if( DhcpApiHostNameParams.nBytesData ) 
>             {
> 
>                 // Check size with dwHostNameBufSize.
> 
>                 CopyMemory(
>                      pszHostNameBuf, DhcpApiHostNameParams.Data,
>                      DhcpApiHostNameParams.nBytesData
>                      );
>                 pszHostNameBuf[DhcpApiHostNameParams.nBytesData] = '\0';
>                 return TRUE;
>             }
> 
> 
> LGTM whatever you decide.

I switched to the belt-and-suspenders approach, which is to first ensure
NULL-termination based on nBytesData, and then return only up to the first
embedded NULL if any.

I'm leaving the DCHECKs in as they were more to ensure we are never getting
double-byte characters (this would be non-standard).

[commit-bot: I haz the power, 2011-12-12 14:36:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joi@chromium.org/8892004/7001

[commit-bot: I haz the power, 2011-12-12 15:36:39]

Change committed as 114019

[eroman, 2011-12-12 20:52:52]

http://codereview.chromium.org/8892004/diff/7001/net/proxy/dhcp_proxy_script_...
File net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc (right):

http://codereview.chromium.org/8892004/diff/7001/net/proxy/dhcp_proxy_script_...
net/proxy/dhcp_proxy_script_adapter_fetcher_win.cc:285:
wpad_params.nBytesData).c_str());
clever approach!