[PATCH 1/2] ntdll/tests: Add a test for segment register contents in x86_64 exception handlers.

[Zebediah Figura]

Signed-off-by: Zebediah Figura 
---
 dlls/ntdll/tests/exception.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c
index da88ca449fb..e2a8071168d 100644
--- a/dlls/ntdll/tests/exception.c
+++ b/dlls/ntdll/tests/exception.c
@@ -2812,7 +2812,7 @@ static void test___C_specific_handler(void)
 /* This is heavily based on the i386 exception tests. */
 static const struct exception
 {
-    BYTE     code[18];      /* asm code */
+    BYTE     code[24];      /* asm code */
     BYTE     offset;        /* offset of faulting instruction */
     BYTE     length;        /* length of faulting instruction */
     NTSTATUS status;        /* expected status code */
@@ -2921,6 +2921,14 @@ static const struct exception
 /* 35 */
     { { 0xa3, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xc3 },  /* movl %eax,0xffffffffffffffff; ret */
       0, 9, STATUS_ACCESS_VIOLATION, 2, { 1, 0xffffffffffffffff } },
+
+    /* test exception with cleared %ds and %es */
+    { { 0x8c, 0xd8, 0x50, 0x8c, 0xc0, 0x50, 0x31, 0xc0, 0x8e, 0xd8, 0x8e, 0xc0,
+        0xfa, 0x58, 0x8e, 0xc0, 0x58, 0x8e, 0xd8, 0xc3 },
+          /* mov %ds,%eax; pushq %rax; mov %es,%eax; pushq %rax; xorl %eax,%eax; mov %eax,%ds; mov %eax,%es;
+           * cli; popq %rax; mov %eax,%es; popq %rax; mov %eax,%ds; ret */
+      12, 1, STATUS_PRIVILEGED_INSTRUCTION, 0 },
+
     { { 0xf1, 0x90, 0xc3 },  /* icebp; nop; ret */
       1, 1, STATUS_SINGLE_STEP, 0 },
     { { 0xcd, 0x2c, 0xc3 },
@@ -2992,6 +3000,9 @@ static DWORD WINAPI handler( EXCEPTION_RECORD *rec, ULONG64 frame,
         "%u: Unexpected exception address %p/%p\n", entry,
         rec->ExceptionAddress, (char*)context->Rip );

+    todo_wine ok( context->SegDs == context->SegSs,
+        "%u: ds %#x does not match ss %#x\n", entry, context->SegDs, context->SegSs );
+
     if (except->status == STATUS_BREAKPOINT && is_wow64)
         parameter_count = 1;
     else if (except->alt_status == 0 || rec->ExceptionCode != except->alt_status)

[PATCH 2/2] ntdll: Report SegDs to be identical to SegSs in x86_64 exception handlers.

[Zebediah Figura]

Based on a patch by Dávid Török.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47970
Signed-off-by: Zebediah Figura 
---
Actually, the behaviour of segment registers on x86_64 Windows is more
complicated:

* By default, %ss == %ds == %es == %gs == 0x2b; %cs == 0x33; %fs == 0x53.

* NtGetContextThread() does not capture the actual values, but instead returns
  the default values.

* All system calls restore all segments to their default values, including
  NtContinue().

* RtlRestoreContext() also restores all segments to their default values.

* SegDs, SegEs, SegFs, SegGs all restore default values in exception handlers;
  SegSs returns the actual value of %ss.

* RtlCaptureContext() does capture the actual values.

I did not test:

* whether %cs is correctly captured or restored in any functions;

* whether segment registers are restored from exception handlers (either to
  the default values or to the values set in the context).

We could, to a point, emulate the above, but it gets tricky. According to the
Intel 64 and IA-32 Architectures Software Developer's Manual:

* %es, %ds, %ss can point to any segment—the entire contents of the descriptor
  table are effectively ignored (if I'm reading Volume 3 §3.4.4 right). However,
  they have to point to a *valid* segment (cf. Volume 1 §3.4.2.1).

* %cs is more restricted—the attributes in the descriptor table are also
  validated (Volume 3 §5.2.1).

* I don't even know about %fs and %gs; the manual is too vague and confusing.

dlls/ntdll/tests/exception.c    | 2 +-
 dlls/ntdll/unix/signal_x86_64.c | 8 +++-----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/dlls/ntdll/tests/exception.c b/dlls/ntdll/tests/exception.c
index e2a8071168d..90af9de1bba 100644
--- a/dlls/ntdll/tests/exception.c
+++ b/dlls/ntdll/tests/exception.c
@@ -3000,7 +3000,7 @@ static DWORD WINAPI handler( EXCEPTION_RECORD *rec, ULONG64 frame,
         "%u: Unexpected exception address %p/%p\n", entry,
         rec->ExceptionAddress, (char*)context->Rip );

-    todo_wine ok( context->SegDs == context->SegSs,
+    ok( context->SegDs == context->SegSs,
         "%u: ds %#x does not match ss %#x\n", entry, context->SegDs, context->SegSs );

if (except->status == STATUS_BREAKPOINT && is_wow64)
diff --git a/dlls/ntdll/unix/signal_x86_64.c b/dlls/ntdll/unix/signal_x86_64.c
index 7dab5fbf4ed..814f2824129 100644
--- a/dlls/ntdll/unix/signal_x86_64.c
+++ b/dlls/ntdll/unix/signal_x86_64.c
@@ -1500,11 +1500,6 @@ static void save_context( struct xcontext *xcontext, const ucontext_t *sigcontex
     context->SegFs  = FS_sig(sigcontext);
     context->SegGs  = GS_sig(sigcontext);
     context->EFlags = EFL_sig(sigcontext);
-#ifdef DS_sig
-    context->SegDs  = DS_sig(sigcontext);
-#else
-    __asm__("movw %%ds,%0" : "=m" (context->SegDs));
-#endif
 #ifdef ES_sig
     context->SegEs  = ES_sig(sigcontext);
 #else
@@ -1515,6 +1510,9 @@ static void save_context( struct xcontext *xcontext, const ucontext_t *sigcontex
 #else
     __asm__("movw %%ss,%0" : "=m" (context->SegSs));
 #endif
+   /* Legends of Runeterra depends on having SegDs == SegSs in an exception
+    * handler. */
+    context->SegDs  = context->SegSs;
     context->Dr0    = amd64_thread_data()->dr0;
     context->Dr1    = amd64_thread_data()->dr1;
     context->Dr2    = amd64_thread_data()->dr2;

[PATCH 2/2] ntdll: Report SegDs to be identical to SegSs in x86_64 exception handlers.

[Marvin]

Hi,

While running your changed tests, 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=87681

Your paranoid android.

=== debiant2 (build log) ===

Task: WineTest did not produce the win32 report

=== debiant2 (build log) ===

Task: Could not create the wow32 wineprefix: Failed to disable the crash dialogs: 
Task: WineTest did not produce the 0 report

[PATCH 2/2] ntdll: Report SegDs to be identical to SegSs in x86_64 exception handlers.

[Alexandre Julliard]

Zebediah Figura  writes:
> @@ -1500,11 +1500,6 @@ static void save_context( struct xcontext *xcontext, const ucontext_t *sigcontex
>      context->SegFs  = FS_sig(sigcontext);
>      context->SegGs  = GS_sig(sigcontext);
>      context->EFlags = EFL_sig(sigcontext);
> -#ifdef DS_sig
> - context->SegDs  = DS_sig(sigcontext);
>
> -#else
> - __asm__("movw %%ds,%0" : "=m" (context->SegDs));
>
> -#endif
>  #ifdef ES_sig
>      context->SegEs  = ES_sig(sigcontext);
>  #else
> @@ -1515,6 +1510,9 @@ static void save_context( struct xcontext *xcontext, const ucontext_t *sigcontex
>  #else
>      __asm__("movw %%ss,%0" : "=m" (context->SegSs));
>  #endif
> - /* Legends of Runeterra depends on having SegDs == SegSs in an exception
> -
> - handler. */
> - context->SegDs  = context->SegSs;

Is there a reason you are not fixing %es too while you are at it?

[PATCH 2/2] ntdll: Report SegDs to be identical to SegSs in x86_64 exception handlers.

[Zebediah Figura (she/her)]

On 3/26/21 6:19 AM, Alexandre Julliard wrote:
> Zebediah Figura  writes:
> > @@ -1500,11 +1500,6 @@ static void save_context( struct xcontext *xcontext, const ucontext_t *sigcontex
> >       context->SegFs  = FS_sig(sigcontext);
> >       context->SegGs  = GS_sig(sigcontext);
> >       context->EFlags = EFL_sig(sigcontext);
> > -#ifdef DS_sig
> > - context->SegDs  = DS_sig(sigcontext);
> >
> > -#else
> > - __asm__("movw %%ds,%0" : "=m" (context->SegDs));
> >
> > -#endif
> >   #ifdef ES_sig
> >       context->SegEs  = ES_sig(sigcontext);
> >   #else
> > @@ -1515,6 +1510,9 @@ static void save_context( struct xcontext *xcontext, const ucontext_t *sigcontex
> >   #else
> >       __asm__("movw %%ss,%0" : "=m" (context->SegSs));
> >   #endif
> > - /* Legends of Runeterra depends on having SegDs == SegSs in an exception
> > -
> > - handler. */
> > - context->SegDs  = context->SegSs;
>
> Is there a reason you are not fixing %es too while you are at it?

The application doesn't care about %es, and I wanted to make the patch 
as uninvasive as possible. A proper fix could mean quite a lot of 
changes (some riskier than others), and it isn't clear to me where a 
line can reasonably be drawn.

[PATCH 2/2] ntdll: Report SegDs to be identical to SegSs in x86_64 exception handlers.

[Alexandre Julliard]

"Zebediah Figura (she/her)"  writes:
> On 3/26/21 6:19 AM, Alexandre Julliard wrote:
> > Zebediah Figura  writes:
> > > @@ -1500,11 +1500,6 @@ static void save_context( struct xcontext *xcontext, const ucontext_t *sigcontex
> > >       context->SegFs  = FS_sig(sigcontext);
> > >       context->SegGs  = GS_sig(sigcontext);
> > >       context->EFlags = EFL_sig(sigcontext);
> > > -#ifdef DS_sig
> > > - context->SegDs  = DS_sig(sigcontext);
> > >
> > > -#else
> > > - __asm__("movw %%ds,%0" : "=m" (context->SegDs));
> > >
> > > -#endif
> > >   #ifdef ES_sig
> > >       context->SegEs  = ES_sig(sigcontext);
> > >   #else
> > > @@ -1515,6 +1510,9 @@ static void save_context( struct xcontext *xcontext, const ucontext_t *sigcontex
> > >   #else
> > >       __asm__("movw %%ss,%0" : "=m" (context->SegSs));
> > >   #endif
> > > - /* Legends of Runeterra depends on having SegDs == SegSs in an exception
> > > -
> > > - handler. */
> > > - context->SegDs  = context->SegSs;
> >
> > Is there a reason you are not fixing %es too while you are at it?
>
> The application doesn't care about %es, and I wanted to make the patch
> as uninvasive as possible. A proper fix could mean quite a lot of 
> changes (some riskier than others), and it isn't clear to me where a
> line can reasonably be drawn.

It would be interesting to have at least tests for the other
registers.

As far as the fix, I'm not sure we want to do it so early in the signal
handling, we may need the actual segment values when running 32-bit code
inside a 64-bit process.

[Marvin]

Hi,

While running your changed tests, 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=87680

Your paranoid android.

=== debiant2 (build log) ===

Task: WineTest did not produce the win32 report

=== debiant2 (build log) ===

Task: Could not create the wow32 wineprefix: Failed to disable the crash dialogs: 
Task: WineTest did not produce the 0 report