[1/3] dpnet: Add check for mismatched string lengths (try 3)

Wed Mar 4 10:23:57 CST 2015

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi,

Am 2015-03-04 um 04:15 schrieb Alistair Leslie-Hughes:
> +        hr = IDirectPlay8Address_AddComponent(localaddr, DPNA_KEY_HOSTNAME, "testing", 8, DPNA_DATATYPE_STRING_ANSI);
> +        ok(hr == S_OK, "got 0x%08x\n", hr);
This still uses a hardcoded size. I recommend to add an array const
char test_str[] = "testing" similar to the localhost WCHAR. That way
you can use the variable in all places.

> +            if (((strlenW((WCHAR*)lpvData)+1)*sizeof(WCHAR)) != dwDataSize)
> +            {
> +                WARN("Invalid STRING size, returning DPNERR_INVALIDPARAM\n");
> +                return DPNERR_INVALIDPARAM;
> +            }
There is a theoretical issue here that I don't know if we should care
about. Assume e.g. dwDataSize is 10, but lpvData points to a blob that
doesn't have a NULL byte before the end of the last defined page. In
this case strlen will search for a string termination and segfault.
strnlen could abort and return an error instead of segfault.

I did some quick testing on Windows and it seems that this function
uses a try / catch block to return gracefully in case of an invalid
pointer. Generally we don't add tests that cause exceptions and I
don't think we should test this here. And I have found no case where
strnlen is used in Wine, and we have no strnlenW, so I don't think we
should use it here. If I pass an invalid non-null pointer I get
0x80004003. If I pass a length = 0 I get DPNERR_INVALIDSTRING.

Also checking !((char *)lpvData)[dwDataSize - 1] would be wrong,
windows gracefully returns DPNERR_INVALIDPARAM on lpvData="123",
size=999999.

> +        hr = IDirectPlay8Address_AddComponent(localaddr, DPNA_KEY_HOSTNAME, "testing", sizeof("Testing")+2, DPNA_DATATYPE_STRING_ANSI);
> +        ok(hr == DPNERR_INVALIDPARAM, "got 0x%08x\n", hr);

Really minor issue: sizeof("testing") vs sizeof("Testing"). The result
is the same, but a stupid compiler might place both strings in the
executable. The variable I suggested above would avoid this issue here
as well.