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

[Jan Beulich <jbeulich@suse.com>]

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

Nick, Alan - do you have any thoughts here?

Jan