-----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.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Hi,
One more thing:
size = sizeof(localhost); + hr =IDirectPlay8Address_GetComponentByName(localaddr,
DPNA_KEY_HOSTNAME, buffer, &size, &type); Please use size = sizeof(buffer) here. Buffer is the variable you pass to GetComponentByName.
Cheers, Stefan
Am 2015-03-04 um 17:23 schrieb Stefan Dösinger:
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.
-
Stefan Dösinger