[PATCH 1/3] wininet/tests: Test InternetCrackUrl when given a bogus length
Windows will stop processing at the first null, even when given an explicit length. Signed-off-by: Tim Clem <tclem(a)codeweavers.com> --- dlls/wininet/tests/url.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/dlls/wininet/tests/url.c b/dlls/wininet/tests/url.c index bce7dc76c59..681b6ef0b32 100644 --- a/dlls/wininet/tests/url.c +++ b/dlls/wininet/tests/url.c @@ -653,6 +653,15 @@ static void InternetCrackUrl_test(void) GLE = GetLastError(); ok(ret == FALSE, "Expected InternetCrackUrl to fail\n"); ok(GLE == ERROR_INTERNET_UNRECOGNIZED_SCHEME, "Expected GLE to represent a failure\n"); + + /* Windows treats dwUrlLength as a maximum - if there is a null before + * that length, it stops there. */ + copy_compsA(&urlSrc, &urlComponents, 32, 1024, 1024, 1024, 1024, 1024); + ret = InternetCrackUrlA("http://x.org", 13 /* includes the nul */, 0, &urlComponents); + ok(ret, "InternetCrackUrlA failed with error %d\n", GetLastError()); + todo_wine + ok(urlComponents.dwHostNameLength == 5, + "Expected dwHostNameLength of 5, got %d\n", urlComponents.dwHostNameLength); } static void InternetCrackUrlW_test(void) @@ -825,6 +834,19 @@ static void InternetCrackUrlW_test(void) ok(r, "InternetCrackUrlW failed unexpectedly\n"); ok(!strcmp_wa(host, "x.org"), "host is %s, should be x.org\n", wine_dbgstr_w(host)); todo_wine ok(urlpart[0] == 0, "urlpart should be empty\n"); + + /* Windows treats dwUrlLength as a maximum - if there is a null before + * that length, it stops there. */ + host[0] = 0; + memset(&comp, 0, sizeof(comp)); + comp.dwStructSize = sizeof(comp); + comp.lpszHostName = host; + comp.dwHostNameLength = ARRAY_SIZE(host); + r = InternetCrackUrlW(url3, 13 /* includes the nul */, 0, &comp); + ok(r, "InternetCrackUrlW failed with error %d\n", GetLastError()); + todo_wine + ok(comp.dwHostNameLength == 5, + "Expected dwHostNameLength of 5, got %d\n", comp.dwHostNameLength); } static void fill_url_components(URL_COMPONENTSA *lpUrlComponents) -- 2.32.0
The analogous heap_strndupW already does this. Fixes InternetCrackUrlA behavior when passed a dwUrlLength that's past the end of the string. Signed-off-by: Tim Clem <tclem(a)codeweavers.com> --- dlls/wininet/internet.h | 5 ++++- dlls/wininet/tests/url.c | 1 - 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/dlls/wininet/internet.h b/dlls/wininet/internet.h index 0efea473255..e9d68e2c2d9 100644 --- a/dlls/wininet/internet.h +++ b/dlls/wininet/internet.h @@ -152,7 +152,10 @@ static inline WCHAR *heap_strndupAtoW(const char *str, int len_a, DWORD *len_w) if(str) { size_t len; - if(len_a < 0) len_a = strlen(str); + if(len_a < 0) + len_a = strlen(str); + else if(len_a > 0) + len_a = strnlen(str, len_a); len = MultiByteToWideChar(CP_ACP, 0, str, len_a, NULL, 0); ret = heap_alloc((len+1)*sizeof(WCHAR)); if(ret) { diff --git a/dlls/wininet/tests/url.c b/dlls/wininet/tests/url.c index 681b6ef0b32..2f3ea9c3d14 100644 --- a/dlls/wininet/tests/url.c +++ b/dlls/wininet/tests/url.c @@ -659,7 +659,6 @@ static void InternetCrackUrl_test(void) copy_compsA(&urlSrc, &urlComponents, 32, 1024, 1024, 1024, 1024, 1024); ret = InternetCrackUrlA("http://x.org", 13 /* includes the nul */, 0, &urlComponents); ok(ret, "InternetCrackUrlA failed with error %d\n", GetLastError()); - todo_wine ok(urlComponents.dwHostNameLength == 5, "Expected dwHostNameLength of 5, got %d\n", urlComponents.dwHostNameLength); } -- 2.32.0
Hi Tim, On 7/1/21 7:40 PM, Tim Clem wrote:
@@ -152,7 +152,10 @@ static inline WCHAR *heap_strndupAtoW(const char *str, int len_a, DWORD *len_w)
if(str) { size_t len; - if(len_a < 0) len_a = strlen(str); + if(len_a < 0) + len_a = strlen(str); + else if(len_a > 0) + len_a = strnlen(str, len_a);
The problem seems specific to InternetCrackUrlA, I'm not sure if we should change general helper. Thanks, Jacek
Hm, I see your point, and the third patch in this set actually fixes the behavior of both
CrackURLA and W, since the first calls the second. But, heap_strndupAtoW is definitely
not doing what it's supposed to do in this case - all other strndups, including
heap_strndupW in the same header, tread the length as a maximum and stop at the first
null. I worry that leaving this as-is is asking for trouble.
July 2, 2021 9:13 AM, "Jacek Caban" <jacek(a)codeweavers.com> wrote:
> Hi Tim,
>
> On 7/1/21 7:40 PM, Tim Clem wrote:
>
>> @@ -152,7 +152,10 @@ static inline WCHAR *heap_strndupAtoW(const char *str, int len_a, DWORD
>> *len_w)
>>> if(str) {
>> size_t len;
>> - if(len_a < 0) len_a = strlen(str);
>> + if(len_a < 0)
>> + len_a = strlen(str);
>> + else if(len_a > 0)
>> + len_a = strnlen(str, len_a);
>
> The problem seems specific to InternetCrackUrlA, I'm not sure if we should change general helper.
>
> Thanks,
>
> Jacek
Signed-off-by: Tim Clem <tclem(a)codeweavers.com> --- dlls/wininet/internet.c | 13 ++++++++++++- dlls/wininet/tests/url.c | 1 - 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/dlls/wininet/internet.c b/dlls/wininet/internet.c index 1b6f060d607..c5b70e60d0c 100644 --- a/dlls/wininet/internet.c +++ b/dlls/wininet/internet.c @@ -1636,7 +1636,18 @@ BOOL WINAPI InternetCrackUrlW(const WCHAR *lpszUrl, DWORD dwUrlLength, DWORD dwF SetLastError(ERROR_INVALID_PARAMETER); return FALSE; } - if (!dwUrlLength) dwUrlLength = lstrlenW(lpszUrl); + + if (!dwUrlLength) + dwUrlLength = lstrlenW(lpszUrl); + else { + /* Windows stops at a null, regardless of what dwUrlLength says. */ + DWORD len = wcsnlen(lpszUrl, dwUrlLength); + if (dwUrlLength != len) { + TRACE("dwUrlLength %u is past a null terminator. Treating length as %u.\n", + dwUrlLength, len); + dwUrlLength = len; + } + } if (dwFlags & ICU_DECODE) { diff --git a/dlls/wininet/tests/url.c b/dlls/wininet/tests/url.c index 2f3ea9c3d14..c097a6f52f9 100644 --- a/dlls/wininet/tests/url.c +++ b/dlls/wininet/tests/url.c @@ -843,7 +843,6 @@ static void InternetCrackUrlW_test(void) comp.dwHostNameLength = ARRAY_SIZE(host); r = InternetCrackUrlW(url3, 13 /* includes the nul */, 0, &comp); ok(r, "InternetCrackUrlW failed with error %d\n", GetLastError()); - todo_wine ok(comp.dwHostNameLength == 5, "Expected dwHostNameLength of 5, got %d\n", comp.dwHostNameLength); } -- 2.32.0
Hi Tim, On 7/1/21 7:40 PM, Tim Clem wrote:
+ /* Windows treats dwUrlLength as a maximum - if there is a null before + * that length, it stops there. */ + copy_compsA(&urlSrc, &urlComponents, 32, 1024, 1024, 1024, 1024, 1024); + ret = InternetCrackUrlA("http://x.org", 13 /* includes the nul */, 0, &urlComponents); + ok(ret, "InternetCrackUrlA failed with error %d\n", GetLastError()); + todo_wine + ok(urlComponents.dwHostNameLength == 5, + "Expected dwHostNameLength of 5, got %d\n", urlComponents.dwHostNameLength);
That test is fine on its own, but for the fix it would be also interesting to know what happens if you pass even larger value (that is, what if the length includes some characters after trailing null byte)? Thanks, Jacek
Counter-Strike: Source was doing exactly that when trying to parse URLs for custom maps. They seem to pass the length of the whole buffer they allocated, all the time (512). InternetCrackURL tries to parse the entirety of the buffer, so it it will try to copy whatever it finds into the appropriate places. In CS:S's case, the buffer looked like this: "http://example.com/map.bsp\0\0(other non-printable garbage)", and CrackURL tried to copy "map.bsp\0\0..." into lpszUrlPath. I assume if the things after the null looked like valid URL components, it would silently put a string with a null into one of the URL_COMPONENTS fields. For instance, I bet (but didn't test) that "http://x\0.org:8080/path" would parse, and just leave "x\0.org" in lpszHostName. Notably, if you look at the ICU_DECODE path in InternetCrackURLW, it first duplicates the URL with heap_strndupW, which will stop at the first null, so that case currently has different (correct) behavior. July 2, 2021 9:11 AM, "Jacek Caban" <jacek(a)codeweavers.com> wrote:
Hi Tim,
On 7/1/21 7:40 PM, Tim Clem wrote:
+ /* Windows treats dwUrlLength as a maximum - if there is a null before + * that length, it stops there. */ + copy_compsA(&urlSrc, &urlComponents, 32, 1024, 1024, 1024, 1024, 1024); + ret = InternetCrackUrlA("http://x.org", 13 /* includes the nul */, 0, &urlComponents); + ok(ret, "InternetCrackUrlA failed with error %d\n", GetLastError()); + todo_wine + ok(urlComponents.dwHostNameLength == 5, + "Expected dwHostNameLength of 5, got %d\n", urlComponents.dwHostNameLength);
That test is fine on its own, but for the fix it would be also interesting to know what happens if you pass even larger value (that is, what if the length includes some characters after trailing null byte)?
Thanks,
Jacek
Hi Tim, On 7/2/21 6:45 PM, Tim Clem wrote:
For instance, I bet (but didn't test) that"http://x\0.org:8080/path" would parse, and just leave "x\0.org" in lpszHostName.
I meant to suggest extending tests with something like this. Thanks, Jacek
participants (2)
-
Jacek Caban -
Tim Clem