Re: [PATCH] x86: Check unbalanced braces in memory reference

["H.J. Lu" <hjl.tools@gmail.com>]

On Thu, Mar 30, 2023 at 7:54 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 20.03.2023 18:03, H.J. Lu via Binutils wrote:
> > Check unbalanced braces in memory reference to avoid assembler crash
> > caused by
> >
> > commit e87fb6a6d0cdfc0e9c471b7825c20c238c2cf506
> > Author: Jan Beulich <jbeulich@suse.com>
> > Date:   Wed Oct 5 09:16:24 2022 +0200
> >
> >     x86/gas: support quoted address scale factor in AT&T syntax
>
> This claim is wrong, and the "fix" is wrong as well. The assertion is
> correct, and it triggering correctly points out a problem, but elsewhere
> (which makes me suspect you didn't take the time to understand what it
> actually is that is going wrong): The parse_register() call from
> i386_att_operand() ends up zapping the trailing three quotes from the
> example operand in the testcase ('")"""'). Which renders invalid the
> checking done earlier in parse_operands().
>
> This behavior of parse_register() in turn is because of bogus behavior
> in get_symbol_name(): It consumes all pairs of quotes (i.e. the trailing
> three ones) with the apparent goal of concatenating adjacent strings.
> But in this case the function stores two nul characters at different
> positions, yet the caller cannot possibly restore more than one of the
> original characters. Hence the previously properly balanced quoted
> string becomes unbalanced. _This_ is what causes the assertion to
> trigger.
>
> Please revert. I'll see to get to fixing this where it needs fixing,
> unless someone else gets to it earlier. For now it isn't really clear
> to me what the best approach is going to be: Having all callers of
> get_symbol_name() deal with the situation isn't nice. But dealing with
> this in get_symbol_name() isn't nice either, as we'd need to replace
> the "excess" characters by e.g. blanks. Yet code elsewhere often enough
> assumes that adjacent blanks were collapsed by the scrubber. IOW even
> then many/most(/all?) callers may need adjustment.
>
> Possibly get_symbol_name() simply isn't intended for cases where the
> original buffer contents is to remain usable for further processing.
> If so,
> - this property should be called out in the comment ahead of the
>   function,
> - we'd simply need to make a copy before calling the function in
>   parse_register() (or in callers where retaining the original buffer
>   contents matters).

What is the end goal of this complexity?  Assembler is changed
to accept more obscure syntaxes which aren't expected by the
original framework.   I think assembler should be as simple as
possible without those obscure syntaxes

> Nick, Alan - do you have any thoughts here?
>
> Jan



-- 
H.J.