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

[Jan Beulich]

On 05.01.2022 20:19, Vladimir Mezentsev wrote:
> 
> 
> 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 ?

I gave an example (still visible below).

>>   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.

Yes, but the impact is worse in a multi-threaded one.

> 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

I haven't put under question that what you do is a necessary step. All
I'm saying is that it's not enough to claim thread safety.

Jan