[PATCH v2 0/2] MR3397: Draft: ucrtbase: Do not modify rewind's stack arguments on i386.

[Victor Hermann Chiletto (＠vitorhnn)]

[A mod for Fallout: New Vegas relies on rewind not modifying the first argument passed to it.](https://github.com/jazzisparis/UIO-User-Interface-Organizer/blob/426d96391f9...) Since it only links to api-ms-win-crt-stdio I've kept the thunk usage to just ucrtbase.

--
  v2: msvcrt, ucrtbase: Introduce wine_i386_rewind.
      ucrtbase/tests: Test some details about rewind's ABI.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3397

[PATCH v2 1/2] ucrtbase/tests: Test some details about rewind's ABI.

[Victor Chiletto]

From: Victor Chiletto <v(a)hnn.net.br>

More specifically: a mod for Fallout: New Vegas relies on rewind not
modifying the first argument passed to it [1].

[1]: https://github.com/jazzisparis/UIO-User-Interface-Organizer/blob/426d96391f9...
---
 dlls/ucrtbase/tests/misc.c | 42 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/dlls/ucrtbase/tests/misc.c b/dlls/ucrtbase/tests/misc.c
index 069de0787b7..1371bc79b32 100644
--- a/dlls/ucrtbase/tests/misc.c
+++ b/dlls/ucrtbase/tests/misc.c
@@ -38,6 +38,10 @@
 #include <winbase.h>
 #include "wine/test.h"
 
+#if defined(__i386__) && defined(__GNUC__)
+#include "wine/asm.h"
+#endif
+
 #define DEFINE_EXPECT(func) \
     static BOOL expect_ ## func = FALSE, called_ ## func = FALSE
 
@@ -1591,6 +1595,41 @@ static void test_fopen_exclusive( void )
     unlink(path);
 }
 
+#if defined(__i386__)
+#if defined(__GNUC__)
+extern CDECL FILE *test_rewind_wrapper(FILE *fp);
+__ASM_GLOBAL_FUNC(test_rewind_wrapper,
+    "pushl 4(%esp)\n\t"
+    "call " __ASM_NAME("rewind") "\n\t"
+    "popl %eax\n\t"
+    "ret")
+#else
+FILE* CDECL test_rewind_wrapper(FILE* fp);
+__declspec(naked) FILE* test_rewind_wrapper(FILE *fp)
+{
+    __asm
+    {
+        push [esp + 4];
+        call rewind;
+        pop eax;
+        ret;
+    }
+}
+#endif
+
+static void test_rewind_i386_abi(void)
+{
+    FILE *fp_in, *fp_out;
+
+    fp_in = fopen("rewind_abi.tst", "wb");
+    fp_out = test_rewind_wrapper(fp_in);
+    todo_wine ok(fp_in == fp_out, "rewind modified the first argument in the stack\n");
+
+    fclose(fp_in);
+    unlink("rewind_abi.tst");
+}
+#endif
+
 START_TEST(misc)
 {
     int arg_c;
@@ -1633,4 +1672,7 @@ START_TEST(misc)
     test_thread_storage();
     test_fenv();
     test_fopen_exclusive();
+#if defined(__i386__)
+    test_rewind_i386_abi();
+#endif
 }
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/3397

[PATCH v2 2/2] msvcrt, ucrtbase: Introduce wine_i386_rewind.

[Victor Chiletto]

From: Victor Chiletto <v(a)hnn.net.br>

This is a small thunk to rewind that preserves the first argument
passed. This is needed because on some versions of GCC, rewind's call to
_unlock_file is tail-call optimized, which modifies the stack.
---
 dlls/msvcrt/file.c          | 13 +++++++++++++
 dlls/ucrtbase/tests/misc.c  |  2 +-
 dlls/ucrtbase/ucrtbase.spec |  3 ++-
 3 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/dlls/msvcrt/file.c b/dlls/msvcrt/file.c
index e72784eef41..14018e7cda9 100644
--- a/dlls/msvcrt/file.c
+++ b/dlls/msvcrt/file.c
@@ -45,6 +45,10 @@
 
 #include "wine/debug.h"
 
+#if defined (__i386__)
+#include "wine/asm.h"
+#endif
+
 WINE_DEFAULT_DEBUG_CHANNEL(msvcrt);
 
 #undef _fstat
@@ -1658,6 +1662,15 @@ int CDECL clearerr_s(FILE* file)
   return 0;
 }
 
+#if defined(__i386__)
+/* INTERNAL: stack preserving thunk for rewind */
+__ASM_GLOBAL_FUNC(wine_i386_rewind,
+                  "pushl 4(%esp)\n\t"
+                  "call "__ASM_NAME("rewind") "\n\t"
+                  "popl %eax\n\t"
+                  "ret")
+#endif
+
 /*********************************************************************
  *		rewind (MSVCRT.@)
  */
diff --git a/dlls/ucrtbase/tests/misc.c b/dlls/ucrtbase/tests/misc.c
index 1371bc79b32..1d978a2fb42 100644
--- a/dlls/ucrtbase/tests/misc.c
+++ b/dlls/ucrtbase/tests/misc.c
@@ -1623,7 +1623,7 @@ static void test_rewind_i386_abi(void)
 
     fp_in = fopen("rewind_abi.tst", "wb");
     fp_out = test_rewind_wrapper(fp_in);
-    todo_wine ok(fp_in == fp_out, "rewind modified the first argument in the stack\n");
+    ok(fp_in == fp_out, "rewind modified the first argument in the stack\n");
 
     fclose(fp_in);
     unlink("rewind_abi.tst");
diff --git a/dlls/ucrtbase/ucrtbase.spec b/dlls/ucrtbase/ucrtbase.spec
index cad6cf1c381..088fafa8494 100644
--- a/dlls/ucrtbase/ucrtbase.spec
+++ b/dlls/ucrtbase/ucrtbase.spec
@@ -2470,7 +2470,8 @@
 @ cdecl remquof(float float ptr)
 @ cdecl remquol(double double ptr) remquo
 @ cdecl rename(str str)
-@ cdecl rewind(ptr)
+@ cdecl -arch=i386 rewind(ptr) wine_i386_rewind
+@ cdecl -arch=!i386 rewind(ptr) rewind
 @ cdecl rint(double) MSVCRT_rint
 @ cdecl rintf(float)
 @ cdecl rintl(double) MSVCRT_rint
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/3397

[Victor Hermann Chiletto (＠vitorhnn)]

v2: Use basic asm for the tests.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3397#note_40106

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/ucrtbase/tests/misc.c:
> +    "call " __ASM_NAME("rewind") "\n\t"
> +    "popl %eax\n\t"
> +    "ret")
> +#else
> +FILE* CDECL test_rewind_wrapper(FILE* fp);
> +__declspec(naked) FILE* test_rewind_wrapper(FILE *fp)
> +{
> +    __asm
> +    {
> +        push [esp + 4];
> +        call rewind;
> +        pop eax;
> +        ret;
> +    }
> +}
> +#endif
> I don't know if it's portable enough. How about changing it so it's done in similar way as in other tests? (see thiscall_thunk in dlls/msvcrt/tests/cpp.c for an example).

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3397#note_40126

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/ucrtbase/tests/misc.c:
> +    {
> +        push [esp + 4];
> +        call rewind;
> +        pop eax;
> +        ret;
> +    }
> +}
> +#endif
> +
> +static void test_rewind_i386_abi(void)
> +{
> +    FILE *fp_in, *fp_out;
> +
> +    fp_in = fopen("rewind_abi.tst", "wb");
> +    fp_out = test_rewind_wrapper(fp_in);
> +    todo_wine ok(fp_in == fp_out, "rewind modified the first argument in the stack\n");
> Please add the test after the fix (or in the same patch). It may fail depending on wine compiler/compilation flags.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3397#note_40127

[Piotr Caban (＠piotr)]

Piotr Caban (@piotr) commented about dlls/msvcrt/file.c:
> return 0;
>  }
>
> +#if defined(__i386__)
> +/* INTERNAL: stack preserving thunk for rewind */
> +__ASM_GLOBAL_FUNC(wine_i386_rewind,
> +                  "pushl 4(%esp)\n\t"
> +                  "call "__ASM_NAME("rewind") "\n\t"
> +                  "popl %eax\n\t"
> +                  "ret")
> +#endif
> Please add CFI annotations and remove `INTERNAL: ` prefix from the comment. Since it's added for a broken app I think it's useful to add a short comment saying "needed for ...".

How about changing `wine_i386_rewind` function name to `rewind_preserve_stack`?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/3397#note_40128