[PATCH] opcodes/i386-dis.c is not thread-safe

[Vladimir Mezentsev]

On 1/5/22 03:42, Jan Beulich wrote:
> On 05.01.2022 08:07, Vladimir Mezentsev via Binutils wrote:
>> From: Vladimir Mezentsev <vladimir.mezentsev@oracle.com>
>>
>> We plan to use print_insn_i386_att, print_insn_i386_intel and
>> print_insn_i386 in a multithreaded application.
>> These functions are not thread safe due to the use of static variables.
>>
>> Tested on x86_64-pc-linux-gnu.
>>
>> opcodes/ChangeLog:
>> 2022-01-04 Vladimir Mezentsev  <vladimir.mezentsev@oracle.com>
>>
>> 	* opcodes/i386-dis.c: Make print_insn_i386_att, print_insn_i386_intel
>> 	and print_insn_i386 thread-safe
> While I appreciate this step, I don't think the result is quite thread-
> safe yet.

  Why ?


>   In particular the various abort() invocations aren't going to
> play well with a multi-threaded consumer of the library. And we know
> that there have been bugs in this area, i.e. where abort() would have
> got triggered by certain invalid encodings;

It is a real bug if we see abort() in one thread application.

But in a multi-threaded application, the problem is very easy to reproduce.
The problem is the shared variables are used in opcodes/i386-dis.c 
without synchronization.
For example:

% grep -n need_modrm i386-dis.c
2404:static unsigned char need_modrm; <<<<<< the static shared variable
2439:#define MODRM_CHECK  if (!need_modrm) abort () <<<<<< Can be 
abort() because need_modrm was reset for the other instruction.
9682:      need_modrm = twobyte_has_modrm[threebyte];         <<<<<< 
Settings for two byte instruction
9688:      need_modrm = onebyte_has_modrm[*codep];<<<<<< Settings for 
the other instruction



-Vladimir

>   I'm pretty certain we haven't
> found (and eliminated) all of them just yet.
>
> Jan
>