Re: [PATCH] pass correct buffersize to LCMapStringW
"Marcus Meissner" <marcus(a)jet.franken.de> wrote:
diff --git a/dlls/kernel32/tests/locale.c b/dlls/kernel32/tests/locale.c index f792da2..a49953b 100644 --- a/dlls/kernel32/tests/locale.c +++ b/dlls/kernel32/tests/locale.c @@ -1268,7 +1268,7 @@ static void test_LCMapStringW(void) /* otherwise src == dst should fail */ SetLastError(0xdeadbeef); ret = LCMapStringW(LOCALE_USER_DEFAULT, LCMAP_SORTKEY | LCMAP_UPPERCASE, - buf, 10, buf, sizeof(buf)); + buf, 10, buf, sizeof(buf)/sizeof(WCHAR)); ok(GetLastError() == ERROR_INVALID_FLAGS /* NT */ || GetLastError() == ERROR_INVALID_PARAMETER /* Win9x */, "unexpected error code %d\n", GetLastError()); @@ -1276,40 +1276,40 @@ static void test_LCMapStringW(void)
/* test whether '\0' is always appended */ ret = LCMapStringW(LOCALE_USER_DEFAULT, LCMAP_SORTKEY, - upper_case, -1, buf, sizeof(buf)); + upper_case, -1, buf, sizeof(buf)/sizeof(WCHAR)); ok(ret, "LCMapStringW must succeed\n"); ret2 = LCMapStringW(LOCALE_USER_DEFAULT, LCMAP_SORTKEY, - upper_case, lstrlenW(upper_case), buf2, sizeof(buf2)); + upper_case, lstrlenW(upper_case), buf2, sizeof(buf2)/sizeof(WCHAR)); ok(ret, "LCMapStringW must succeed\n"); ok(ret == ret2, "lengths of sort keys must be equal\n"); ok(!lstrcmpA(p_buf, p_buf2), "sort keys must be equal\n");
/* test LCMAP_SORTKEY | NORM_IGNORECASE */ ret = LCMapStringW(LOCALE_USER_DEFAULT, LCMAP_SORTKEY | NORM_IGNORECASE, - upper_case, -1, buf, sizeof(buf)); + upper_case, -1, buf, sizeof(buf)/sizeof(WCHAR)); ok(ret, "LCMapStringW must succeed\n"); ret2 = LCMapStringW(LOCALE_USER_DEFAULT, LCMAP_SORTKEY, - lower_case, -1, buf2, sizeof(buf2)); + lower_case, -1, buf2, sizeof(buf2)/sizeof(WCHAR)); ok(ret2, "LCMapStringW must succeed\n"); ok(ret == ret2, "lengths of sort keys must be equal\n"); ok(!lstrcmpA(p_buf, p_buf2), "sort keys must be equal\n");
/* test LCMAP_SORTKEY | NORM_IGNORENONSPACE */ ret = LCMapStringW(LOCALE_USER_DEFAULT, LCMAP_SORTKEY | NORM_IGNORENONSPACE, - lower_case, -1, buf, sizeof(buf)); + lower_case, -1, buf, sizeof(buf)/sizeof(WCHAR)); ok(ret, "LCMapStringW must succeed\n"); ret2 = LCMapStringW(LOCALE_USER_DEFAULT, LCMAP_SORTKEY, - lower_case, -1, buf2, sizeof(buf2)); + lower_case, -1, buf2, sizeof(buf2)/sizeof(WCHAR)); ok(ret2, "LCMapStringW must succeed\n"); ok(ret == ret2, "lengths of sort keys must be equal\n"); ok(!lstrcmpA(p_buf, p_buf2), "sort keys must be equal\n");
/* test LCMAP_SORTKEY | NORM_IGNORESYMBOLS */ ret = LCMapStringW(LOCALE_USER_DEFAULT, LCMAP_SORTKEY | NORM_IGNORESYMBOLS, - lower_case, -1, buf, sizeof(buf)); + lower_case, -1, buf, sizeof(buf)/sizeof(WCHAR)); ok(ret, "LCMapStringW must succeed\n"); ret2 = LCMapStringW(LOCALE_USER_DEFAULT, LCMAP_SORTKEY, - symbols_stripped, -1, buf2, sizeof(buf2)); + symbols_stripped, -1, buf2, sizeof(buf2)/sizeof(WCHAR)); ok(ret2, "LCMapStringW must succeed\n"); ok(ret == ret2, "lengths of sort keys must be equal\n"); ok(!lstrcmpA(p_buf, p_buf2), "sort keys must be equal\n");
LCMAP_SORTKEY takes the target buffer size in bytes in both A and W versions.
@@ -1332,7 +1332,7 @@ static void test_LCMapStringW(void)
/* test srclen = 0 */ SetLastError(0xdeadbeef); - ret = LCMapStringW(LOCALE_USER_DEFAULT, 0, upper_case, 0, buf, sizeof(buf)); + ret = LCMapStringW(LOCALE_USER_DEFAULT, 0, upper_case, 0, buf, sizeof(buf)/sizeof(WCHAR)); ok(!ret, "LCMapStringW should fail with srclen = 0\n"); ok(GetLastError() == ERROR_INVALID_PARAMETER, "unexpected error code %d\n", GetLastError());
The size of the target buffer doesn't matter at all in this case, since the API is supposed to fail due to source length being 0. -- Dmitry.
Am Sonntag, den 14.09.2008, 23:32 +0900 schrieb Dmitry Timoshkov:
ok(ret, "LCMapStringW must succeed\n"); ret2 = LCMapStringW(LOCALE_USER_DEFAULT, LCMAP_SORTKEY, - symbols_stripped, -1, buf2, sizeof(buf2)); + symbols_stripped, -1, buf2, sizeof(buf2)/sizeof(WCHAR)); ok(ret2, "LCMapStringW must succeed\n"); ok(ret == ret2, "lengths of sort keys must be equal\n"); ok(!lstrcmpA(p_buf, p_buf2), "sort keys must be equal\n");
LCMAP_SORTKEY takes the target buffer size in bytes in both A and W versions.
Do you have any references for this claim? Wine doesn't implement it, MSDN mentions only characters and there is no Wine API test to prove your statement.
- ret = LCMapStringW(LOCALE_USER_DEFAULT, 0, upper_case, 0, buf, sizeof(buf)); + ret = LCMapStringW(LOCALE_USER_DEFAULT, 0, upper_case, 0, buf, sizeof(buf)/sizeof(WCHAR)); ok(!ret, "LCMapStringW should fail with srclen = 0\n"); The size of the target buffer doesn't matter at all in this case, since the API is supposed to fail due to source length being 0.
Even if the size doesn't matter, this line should get fixed, as the Wine tests are a kind of of Win32 API reference by example. IMHO you shouldn't include such misleading parameters as the size in the wrong unit into API usage examples. Regards, Michael Karcher
"Michael Karcher" <wine(a)mkarcher.dialup.fu-berlin.de> wrote:
ok(ret, "LCMapStringW must succeed\n"); ret2 = LCMapStringW(LOCALE_USER_DEFAULT, LCMAP_SORTKEY, - symbols_stripped, -1, buf2, sizeof(buf2)); + symbols_stripped, -1, buf2, sizeof(buf2)/sizeof(WCHAR)); ok(ret2, "LCMapStringW must succeed\n"); ok(ret == ret2, "lengths of sort keys must be equal\n"); ok(!lstrcmpA(p_buf, p_buf2), "sort keys must be equal\n");
LCMAP_SORTKEY takes the target buffer size in bytes in both A and W versions.
Do you have any references for this claim? Wine doesn't implement it, MSDN mentions only characters and there is no Wine API test to prove your statement.
Wine does implement that, and MSDN clearly describes the case of generating a sort key too.
- ret = LCMapStringW(LOCALE_USER_DEFAULT, 0, upper_case, 0, buf, sizeof(buf)); + ret = LCMapStringW(LOCALE_USER_DEFAULT, 0, upper_case, 0, buf, sizeof(buf)/sizeof(WCHAR)); ok(!ret, "LCMapStringW should fail with srclen = 0\n"); The size of the target buffer doesn't matter at all in this case, since the API is supposed to fail due to source length being 0.
Even if the size doesn't matter, this line should get fixed, as the Wine tests are a kind of of Win32 API reference by example. IMHO you shouldn't include such misleading parameters as the size in the wrong unit into API usage examples.
This particular test doesn't depend on the size of the target buffer, be it 0, -1, or whatever. -- Dmitry.
Am Montag, den 15.09.2008, 00:01 +0900 schrieb Dmitry Timoshkov:
"Michael Karcher" <wine(a)mkarcher.dialup.fu-berlin.de> wrote:
LCMAP_SORTKEY takes the target buffer size in bytes in both A and W versions. Do you have any references for this claim? Wine doesn't implement it, MSDN mentions only characters and there is no Wine API test to prove your statement. Wine does implement that, and MSDN clearly describes the case of generating a sort key too. Ugh. Sorry. Now I see. Ugly API, but Microsofts fault. Sort key generation and string mapping shouldn't have been in the same function at first.
Regards, Michael Karcher
2008/9/14 Dmitry Timoshkov <dmitry(a)codeweavers.com>:
"Michael Karcher" <wine(a)mkarcher.dialup.fu-berlin.de> wrote:
- ret = LCMapStringW(LOCALE_USER_DEFAULT, 0, upper_case, 0, buf, sizeof(buf)); + ret = LCMapStringW(LOCALE_USER_DEFAULT, 0, upper_case, 0, buf, sizeof(buf)/sizeof(WCHAR)); ok(!ret, "LCMapStringW should fail with srclen = 0\n"); The size of the target buffer doesn't matter at all in this case, since the API is supposed to fail due to source length being 0.
Even if the size doesn't matter, this line should get fixed, as the Wine tests are a kind of of Win32 API reference by example. IMHO you shouldn't include such misleading parameters as the size in the wrong unit into API usage examples.
This particular test doesn't depend on the size of the target buffer, be it 0, -1, or whatever.
I don't understand your objection to fixing this. If by chance Microsoft happen to change the API to not fail when the source length is 0 then this will suddenly become a possible buffer overrun. Why not just pass in a correct length as the destination length or just pass in 0? -- Rob Shearman
"Rob Shearman" <robertshearman(a)gmail.com> wrote:
- ret = LCMapStringW(LOCALE_USER_DEFAULT, 0, upper_case, 0, buf, sizeof(buf)); + ret = LCMapStringW(LOCALE_USER_DEFAULT, 0, upper_case, 0, buf, sizeof(buf)/sizeof(WCHAR)); ok(!ret, "LCMapStringW should fail with srclen = 0\n"); The size of the target buffer doesn't matter at all in this case, since the API is supposed to fail due to source length being 0.
Even if the size doesn't matter, this line should get fixed, as the Wine tests are a kind of of Win32 API reference by example. IMHO you shouldn't include such misleading parameters as the size in the wrong unit into API usage examples.
This particular test doesn't depend on the size of the target buffer, be it 0, -1, or whatever.
I don't understand your objection to fixing this. If by chance Microsoft happen to change the API to not fail when the source length is 0 then this will suddenly become a possible buffer overrun.
If the source length is 0, then there is nothing to write to the destination, and therefore there shouldn't be a buffer overrun problem.
Why not just pass in a correct length as the destination length or just pass in 0?
Because that wouldn't change anything IMO. -- Dmitry.
2008/9/16 Dmitry Timoshkov <dmitry(a)codeweavers.com>:
"Rob Shearman" <robertshearman(a)gmail.com> wrote:
- ret = LCMapStringW(LOCALE_USER_DEFAULT, 0, upper_case, 0, buf, sizeof(buf)); + ret = LCMapStringW(LOCALE_USER_DEFAULT, 0, upper_case, 0, buf, sizeof(buf)/sizeof(WCHAR)); ok(!ret, "LCMapStringW should fail with srclen = 0\n"); The size of the target buffer doesn't matter at all in this case, since the API is supposed to fail due to source length being 0.
Even if the size doesn't matter, this line should get fixed, as the Wine tests are a kind of of Win32 API reference by example. IMHO you shouldn't include such misleading parameters as the size in the wrong unit into API usage examples.
This particular test doesn't depend on the size of the target buffer, be it 0, -1, or whatever.
Why not just pass in a correct length as the destination length or just pass in 0?
Because that wouldn't change anything IMO.
It's a bug lurking. Someone could copy and paste that code, changing the source length and the source string but forget to change the destination length, because at first glance it looks correct. Furthermore, it will flag up as a warning in static analysis tools that detect this and result in noise where it's hard to find the real bugs. -- Rob Shearman
Ok I went back blew away my git tree and got the latest one out there... I then did one patch apply using one of my patches... and started rethinking the way I was doing? drawStridedSlow. Looking further into the code I found there was another function further down that did the switch but it was for another shader type not the one I am working on... it uses glVertexAttrib4svARB instead of glMultiTexCoord4NsvARB. So I created a new inline function like the other (dont think it should be a inline but following the other convention). Now here is the question... In looking in drawprim in the function drawStridedSlow I came across this : ??????????????? int??? coordIdx = This->stateBlock->textureState[textureNo][WINED3DTSS_TEXCOORDINDEX]; ??????????????? int texture_idx = This->texUnitMap[textureNo]; ??????????????? float *ptrToCoords = NULL; ??????????????? float? s = 0.0, t = 0.0, r = 0.0, q = 0.0; ??????????????? if (coordIdx > 7) { ??????????????????? VTRACE(("tex: %d - Skip tex coords, as being system generated\n", textureNo)); ??????????????????? continue; ??????????????? } else if (coordIdx < 0) { ??????????????????? FIXME("tex: %d - Coord index %d is less than zero, expect a crash.\n", textureNo, coordIdx); ??????????????????? continue; ??????????????? } And as far as I can tell from that code it limits the total number texture coordinates to 8 max. I cant find anywhere in the dx9 docs where it says that it is a hard limit. I do see that it is based more on the video card limit than anything else... So the question is do we need this? If so why? so I understand...if not should I remove as part off this patch or submit it as another? Next Question : Once I finish the texture patch I am working on do I do a git commit of the patch then a git refresh of the tree before I do the patch export to send in? or whats the process so I dont get the messed up tree like I had before... Thanks Guys Chris
participants (4)
-
celticht32＠aol.com -
Dmitry Timoshkov -
Michael Karcher -
Rob Shearman