Bug 344139 - vex x86->IR: unhandled instruction bytes: 0x36 0x8A 0x18 0x22 (and many other examples)
Summary: vex x86->IR: unhandled instruction bytes: 0x36 0x8A 0x18 0x22 (and many other...
Status: RESOLVED FIXED
Alias: None
Product: valgrind
Classification: Unclassified
Component: vex (show other bugs)
Version: 3.10 SVN
Platform: Debian unstable Linux
: NOR normal (vote)
Target Milestone: ---
Assignee: Julian Seward
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-02-13 23:22 UTC by Richard Nelson
Modified: 2017-03-06 14:01 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
[PATCH 1/2] Initialize x86 system GDT on first use. (2.29 KB, patch)
2017-01-20 03:25 UTC, Sebastian Lackner
Details
[PATCH 2/2] VEX: Recognize the SS segment prefix on x86. (1.42 KB, patch)
2017-01-20 03:27 UTC, Sebastian Lackner
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Nelson 2015-02-13 23:22:39 UTC
I support an internal tool I was hoping to debug via valgrind - but can't, for the nonce :-(
The tool is written in an private language, and it generates I486 assembly (internal format).

Even with a FLAT address space, the compiler/asm often use SS Segment overrides of the form:
36C60058 MVIB XTEXTTYP(EAX,SS),X'58'
360FB64001 MOVZXB EAX,XTEXTLEN(EAX,SS)
36895813 ST EBX,LINE2ID(EAX,SS)
368A18 L BL,0(EAX,SS)
etc..

There are literally ~1506 of these SS overrides - quite likely due to their being derived from mainframe equivalents.    Changing those bytes to 0x90 is not really feasible, due to the variance in instructions in which they are used.

More information:
$ uname -a;valgrind --version
Linux gothic-ave 3.16-2-amd64 #1 SMP Debian 3.16.3-2 (2014-09-20) x86_64 GNU/Linux
valgrind-3.10.1

==7637== Memcheck, a memory error detector
==7637== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==7637== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==7637== Command: Run/Linux_gcc/Importer/a.out
==7637==
--7637-- Valgrind options:
--7637--    -v
--7637-- Contents of /proc/version:
--7637--   Linux version 3.16-2-amd64 (debian-kernel@lists.debian.org) (gcc vers
--7637-- Arch and hwcaps: X86, LittleEndian, x86-mmxext-sse1-sse2
...
vex x86->IR: unhandled instruction bytes: 0x36 0x8A 0x18 0x22
==7637== valgrind: Unrecognised instruction at address 0x804f4b4.
... sigkill (caught by my handler)
vex x86->IR: unhandled instruction bytes: 0x36 0x89 0x58 0xF
==7637== valgrind: Unrecognised instruction at address 0x804bdc3.

Reproducible: Always

Steps to Reproduce:
1. start program under valgrind (hopefully to track malloc/free and other issues)
2. the interpreter (vex) hits one of these SS overriden instructions
3. bad stuff happens

Actual Results:  
Execution fails upon hitting the first, of many, prefixed instructions

Expected Results:  
Execution completes and delivers a report on how badly memory is managed
Comment 1 Sebastian Lackner 2015-03-05 21:14:45 UTC
The current development version of Wine is also affected by this problem, see:
http://source.winehq.org/git/wine.git/blob/HEAD:/tools/winebuild/relay.c#l873

Not sure about the app for which the bug was originally opened, but at least in this case it would also be fine to completely ignore the SS prefix, it is only required for "unusual" Windows applications. Unfortunately the SIGILL signal kills Wine immediately, which makes it impossible to run for the test suite (without hacking either Wine or Valgrind).

To solve this problem we have basically the following options:

* Just ignore SS prefixes for now, probably show a warning. Julian Seward said that he is not really happy with this idea. ;)

* Try to find some rule which can be used to decide if we can ignore the SS prefix. Unfortunately there is no really good rule. One possible solution would be to check if SS==DS, something like http://ix.io/gK2, but it is of course not completely correct. The whole way how Valgrind handles segment registers looks a bit hacky, because "no prefix" doesn't mean that segment registers are not used. All the implicit segment register usage still seems to be missing in the implementation. Test code for SS==DS and SS!=DS can be found here: http://ix.io/gKy, make sure to compile with -m32.

* Correctly handle the SS prefix when explicitly specified. This would probably be the most consistent solution. Implicit handling is of course still missing, but thats a separate and much more complicated task, which also involves all other segment registers. http://ix.io/gKt shows an example how it could be implemented. This patch also contains a "fix" for a different bug, Valgrind doesn't initialize the system entries of the GDT table, which results in a segmentation fault (test code http://ix.io/gKw, compile with -m32).

I leave it up to the Valgrind experts which of these solutions is preferred.

Best regards,
Sebastian
Comment 2 Austin English 2016-12-05 06:48:51 UTC
To give a bit more info (and for myself in the future ;).

This is still present in development wine (wine-1.9.24-105-g1d3b944) and valgrind (valgrind-3.13.0.SVN, #define VGSVN "16171", #define VEXSVN "3285").

It's not reproducible with every wine unit tests. The first case I found was dlls/advapi32/tests/service.c.

For info on running wine under valgrind, see:
https://wiki.winehq.org/Wine_and_Valgrind

my scripts/suppression files are at:
https://github.com/austin987/wine_misc/tree/master/valgrind

but in short:
# get wine/wine_misc repos
$ cd wine-valgrind
$ ln -s /path/to/wine_misc/valgrind tools/valgrind
$ ./configure && make -j8
$ vi tools/valgrind/vg-wrapper.sh
# edit paths to wine/valgrind, if needed
$ . tools/valgrind/vg-wrapper.sh
$ ./wine start /min notepad
$ cd dlls/advapi32/tests
$ make service.ok
# BUG

If the bug is present, you should see:
../../../tools/runtest -q -P wine -T ../../.. -M advapi32.dll -p advapi32_test.exe.so service && touch service.ok
preloader: Warning: failed to reserve range 00110000-68000000
preloader: Warning: failed to reserve range 7f000000-82000000
err:rpc:I_RpcGetBuffer no binding
err:seh:segv_handler Got unexpected trap 0
wine: Unhandled illegal instruction at address 0x7bc280f5 (thread 006d), starting debugger...
preloader: Warning: failed to reserve range 00110000-68000000
preloader: Warning: failed to reserve range 7f000000-82000000

the key lines being:

err:seh:segv_handler Got unexpected trap 0
wine: Unhandled illegal instruction at address 0x7bc280f5 (thread 006d), starting debugger...

at that point, it will hang indefinitely.

With a patch from Sebastian (for Wine):
diff --git a/dlls/ntdll/signal_i386.c b/dlls/ntdll/signal_i386.c
index 59dca6c..a8cdb96 100644
--- a/dlls/ntdll/signal_i386.c
+++ b/dlls/ntdll/signal_i386.c
@@ -2076,6 +2076,15 @@ static void segv_handler( int signal, siginfo_t *siginfo, void *sigcontext )
         return;
     }
 
+    if (!get_trap_code(context) &&
+        siginfo->si_addr == (void *)EIP_sig(context) &&
+        *(char *)EIP_sig(context) == 0x36)
+    {
+        FIXME("---> working around Valgrind SIGILL exception\n");
+        EIP_sig(context)++;
+        return;
+    }
+
     /* check for page fault inside the thread stack */
     if (get_trap_code(context) == TRAP_x86_PAGEFLT &&
         (char *)siginfo->si_addr >= (char *)NtCurrentTeb()->DeallocationStack &&

the tests will pass and not hang.
Comment 3 Sebastian Lackner 2017-01-20 03:25:03 UTC
Created attachment 103551 [details]
[PATCH 1/2] Initialize x86 system GDT on first use.

Thanks Julian for taking a look at the proposed patches. This series is an improved version of method 3, which initializes the system GDT on first use. Feedback welcome!

Testcase (crashes without the patch):

--- snip ---
/* gcc -m32 -o test test.c */
int main (int argc, char *argv[])
{
  asm(".byte 0x3E\n" \
      "movl (%esp),%eax\n");
  return 0;
}
--- snip ---

Please note that this issue is unrelated to the SS segment problem.
Comment 4 Sebastian Lackner 2017-01-20 03:27:26 UTC
Created attachment 103552 [details]
[PATCH 2/2] VEX: Recognize the SS segment prefix on x86.
Comment 5 Julian Seward 2017-01-20 10:03:20 UTC
> Created attachment 103551 [details]
> [PATCH 1/2] Initialize x86 system GDT on first use.

Committed, valgrind r16204.

> Created attachment 103552 [details]
> [PATCH 2/2] VEX: Recognize the SS segment prefix on x86.

Committed, vex r3299.

Thanks for the patches!
Comment 6 Julian Seward 2017-01-20 10:08:37 UTC
Richard .. 2 years later .. can we close this now?