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

[H.J. Lu]

On Wed, Jan 5, 2022 at 11:19 AM Vladimir Mezentsev via Binutils
<binutils@sourceware.org> 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 ?
>
>
> >   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
> >

Hi, Vladimir,

Please check it in.

Thanks.

-- 
H.J.