[PATCH resent 1/2] kernel32/tests: Test partially overlapped buffers for MultiByteToWideChar

[Gabriel Ivăncescu]

Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---
 dlls/kernel32/tests/codepage.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/dlls/kernel32/tests/codepage.c b/dlls/kernel32/tests/codepage.c
index b15a3a1..2e47868 100644
--- a/dlls/kernel32/tests/codepage.c
+++ b/dlls/kernel32/tests/codepage.c
@@ -314,12 +314,23 @@ static void test_overlapped_buffers(void)
     char buf[256];
     int ret;
 
+    /* limit such that strA's NUL terminator overlaps strW's NUL */
+    size_t overlap_limit = (sizeof(strW)-sizeof(strA)) - (sizeof(strW[0])-sizeof(strA[0]));
+
     SetLastError(0xdeadbeef);
     memcpy(buf + 1, strW, sizeof(strW));
     ret = WideCharToMultiByte(CP_ACP, 0, (WCHAR *)(buf + 1), -1, buf, sizeof(buf), NULL, NULL);
     ok(ret == sizeof(strA), "unexpected ret %d\n", ret);
     ok(!memcmp(buf, strA, sizeof(strA)), "conversion failed: %s\n", buf);
     ok(GetLastError() == 0xdeadbeef, "GetLastError() is %u\n", GetLastError());
+
+    SetLastError(0xdeadbeef);
+    memcpy(buf + overlap_limit, strA, sizeof(strA));
+    ret = MultiByteToWideChar(CP_ACP, 0, buf + overlap_limit, -1, (WCHAR *)buf, sizeof(buf) / sizeof(WCHAR));
+    ok(ret == ARRAY_SIZE(strW), "unexpected ret %d\n", ret);
+todo_wine
+    ok(!memcmp(buf, strW, sizeof(strW)), "conversion failed: %s\n", wine_dbgstr_wn((WCHAR *)buf, ARRAY_SIZE(strW)));
+    ok(GetLastError() == 0xdeadbeef, "GetLastError() is %u\n", GetLastError());
 }
 
 static void test_string_conversion(LPBOOL bUsedDefaultChar)
-- 
2.19.1

[PATCH resent 2/2] libs/port/mbtowc: Convert 1 character at a time if dst overlaps into src

[Gabriel Ivăncescu]

Cause of bug discovered by Jason Edmeades.

Some applications partially overlap the two buffers. Handle such rare corner
cases without affecting performance in the general case.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=38558
Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
---

There are no risks of regression here, as the only case where it is slower
now is when previously gave a completely wrong result.

I assume performance is important for this function, since it had the switch
statement in the first place...

 dlls/kernel32/tests/codepage.c |  1 -
 libs/port/mbtowc.c             | 10 ++++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/dlls/kernel32/tests/codepage.c b/dlls/kernel32/tests/codepage.c
index 2e47868..38f094f 100644
--- a/dlls/kernel32/tests/codepage.c
+++ b/dlls/kernel32/tests/codepage.c
@@ -328,7 +328,6 @@ static void test_overlapped_buffers(void)
     memcpy(buf + overlap_limit, strA, sizeof(strA));
     ret = MultiByteToWideChar(CP_ACP, 0, buf + overlap_limit, -1, (WCHAR *)buf, sizeof(buf) / sizeof(WCHAR));
     ok(ret == ARRAY_SIZE(strW), "unexpected ret %d\n", ret);
-todo_wine
     ok(!memcmp(buf, strW, sizeof(strW)), "conversion failed: %s\n", wine_dbgstr_wn((WCHAR *)buf, ARRAY_SIZE(strW)));
     ok(GetLastError() == 0xdeadbeef, "GetLastError() is %u\n", GetLastError());
 }
diff --git a/libs/port/mbtowc.c b/libs/port/mbtowc.c
index 4977c82..6b12e81 100644
--- a/libs/port/mbtowc.c
+++ b/libs/port/mbtowc.c
@@ -65,6 +65,16 @@ static inline int mbstowcs_sbcs( const struct sbcs_table *table, int flags,
         ret = -1;
     }
 
+    /* when dst overlaps into src we need to handle 1 char at a time */
+    if ((UINT_PTR)src - (UINT_PTR)dst < srclen * sizeof(*dst))
+    {
+        do
+            *dst++ = cp2uni[*src++];
+        while (--srclen);
+        return ret;
+    }
+
+    /* dst doesn't overlap into src */
     for (;;)
     {
         switch(srclen)
-- 
2.19.1

[PATCH resent 2/2] libs/port/mbtowc: Convert 1 character at a time if dst overlaps into src

[Alexandre Julliard]

Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
> Cause of bug discovered by Jason Edmeades.
>
> Some applications partially overlap the two buffers. Handle such rare corner
> cases without affecting performance in the general case.
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=38558
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> ---
>
> There are no risks of regression here, as the only case where it is slower
> now is when previously gave a completely wrong result.
>
> I assume performance is important for this function, since it had the switch
> statement in the first place...

Yes, that's why it would be better to do it the same way as the wcstombs
case.

Note that unless you can show that it affects more apps than the one
from the bug report, it will still have to wait until after code freeze.

-- 
Alexandre Julliard
julliard(a)winehq.org

[PATCH resent 2/2] libs/port/mbtowc: Convert 1 character at a time if dst overlaps into src

[Gabriel Ivăncescu]

On 12/19/18 15:16, Alexandre Julliard wrote:
> Gabriel Ivăncescu <gabrielopcode(a)gmail.com> writes:
> > Cause of bug discovered by Jason Edmeades.
> >
> > Some applications partially overlap the two buffers. Handle such rare corner
> > cases without affecting performance in the general case.
> >
> > Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=38558
> > Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com>
> > ---
> >
> > There are no risks of regression here, as the only case where it is slower
> > now is when previously gave a completely wrong result.
> >
> > I assume performance is important for this function, since it had the switch
> > statement in the first place...
>
> Yes, that's why it would be better to do it the same way as the wcstombs
> case.
>
> Note that unless you can show that it affects more apps than the one
> from the bug report, it will still have to wait until after code freeze.

Oh ok, I'll revise and send it after the code freeze then.

[Marvin]

Hi,

While running your changed tests on Windows, I think I found new failures.
Being a bot and all I'm not very good at pattern recognition, so I might be
wrong, but could you please double-check?

Full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=45846

Your paranoid android.


=== debian9 (32 bit Japanese:Japan report) ===

kernel32:
codepage.c:332: Test succeeded inside todo block: conversion failed: L"just a test\0000"

=== debian9 (32 bit Chinese:China report) ===

kernel32:
codepage.c:332: Test succeeded inside todo block: conversion failed: L"just a test\0000"

[Gabriel Ivăncescu]

I guess the tests should be placed after the patch (or in the same 
patch) and get rid of the todo_wine since it seems fragile on the debian 
VM with locales. Do you think it's worth to do that?


On 12/19/18 14:28, Marvin wrote:
> Hi,
>
> While running your changed tests on Windows, I think I found new failures.
> Being a bot and all I'm not very good at pattern recognition, so I might be
> wrong, but could you please double-check?
>
> Full results can be found at:
> https://testbot.winehq.org/JobDetails.pl?Key=45846
>
> Your paranoid android.
>
> === debian9 (32 bit Japanese:Japan report) ===
>
> kernel32:
> codepage.c:332: Test succeeded inside todo block: conversion failed: L"just a test\0000"
>
> === debian9 (32 bit Chinese:China report) ===
>
> kernel32:
> codepage.c:332: Test succeeded inside todo block: conversion failed: L"just a test\0000"