Re: [1/3] dpnet: Add check for mismatched string lengths (try 3)
-----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 SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJU9zGdAAoJEN0/YqbEcdMw8MkP/0Mf46JYgZmA3IwqErfl5lRf fLq1ekHnsKWVXA8pMvl3A0bm2j9fP7UAErFo6mA+oW4KkqHtkqw9z87boJ5Ul5ko wy1T12fizUXXtxdq4Q+TKLCCl+SUX/CUS36Cvw7BeyHGldlUHUrgwQcv2UQg3T1Y dfug2KtrH1M8xTcZBmF9lRMGUqAUpcPxdH0wPh7AoOJsmNpXQUL3jZofsc2s6x7r JFffHxG2EdX96VSjhC+IG3vQLwH+SzlocblcGFFxhv/fjf0u0lgSZegmJVxDxfqc CVWSvMCpnXsd6O57ft8b25eJCmOkZLpYkvaOPiakj+Lz4I8h+lGzGck/Wfvy+bR2 Z0gqrNVjbABkYTlYHw6AsLHnliOnbvs3PROMG3mYsmmGOaLWIwFYhmIFBQCE6Kzv EWnvr3JSux2M689zurKvvJF/GYyS52SntMSpmJmcZ5TndFVi/xspL/+AUUMkr6tQ KK/ykTayv65ioa5T+VCFVclatama9wUcBkHQIU/ej9c4Njj9yXqTlBS7FC5Jieqo GBVHwuv0jNqMHBcAT5jR/V09wfbb3dmoz4yzdKJFGNRfpTeDFfF8qEbOO4AIN3Vy TRQQJJXXoz+UO+Ml0ZfYHxBsOnXSmiIHC8Pcdd9MTsY9XPUw1fFi+zLpXu8urm9Q vb7jaq/Ns9dXETrLE1m9 =jMxz -----END PGP SIGNATURE-----
-----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.
-----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJU9zQxAAoJEN0/YqbEcdMwQFUP/ApILUlGHoweVmzD8a9gzDnp wyEZiJV50LwbAUMaFG7BeZjvDfKGxPD8cDNJpHYoA8CBYg8knv6d/7QR8qab3pg7 zGAtX/82S2P0RnBxl9YrY4ruMUvXaej2sWJnxJkchffP1iWQ0ioKzPhTh7aHZnZH ZKey82eyZ3vk3COzVIoVV+Qpw7WDqFcVOm7//s2RxOiTFntM+IzD1kTWWZ3WrJxh 8fsI+7/tPCxsy7/ayotttFAoge7e9awOy7U07C1R9vhc2dNgAY0qy+S8ZBZ1OnLE EKjNxM2SGImtdEyPyXoCM9TfS9PtlnABKNSDyw9L6544LznIWf9u2QVkmXbMM+EP cof3TFolRjKwq1M2WFHlnQxDb1CE9gfmLyIps2NenjOxzlC2efp/PnvZK3lrhhNi F/Vi1T6QM4JcREo++oyb//bEUqefq4v3sSzxfq0f1vEgd0Xa3aoRogocaCgsGtaL uRrkqWyt2HLtvsuawOg3PxiP7UQKrqmrKjKuw/UxPBU+7ATgNOLhIkmyQqDIdCgD pOOxErtwRxsSA1kYTphlcVkKLi6sJq0V2GXV303Y+SBvqS0g91xm3ejk8MCL+X7z Lt+4tIzkvFttapDvmgsCaRhfOQ2xriLE6stP5OWRmzionQu3l1vMhR0ZP/ADHal8 Y7rB5wrSwFmfF+i7m5Gl =oWSc -----END PGP SIGNATURE-----
participants (1)
-
Stefan Dösinger