* PR28977 tc-i386.c internal error in parse_register
@ 2022-03-18 6:56 Alan Modra
2022-03-18 7:12 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Alan Modra @ 2022-03-18 6:56 UTC (permalink / raw)
To: binutils
PR 28977
* config/tc-i386.c (parse_register): Handle X_op not O_register
as for a non-reg_section symbol. Simplify array bounds check.
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 1cc14feeccf..8ef71b62e42 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -12952,17 +12952,18 @@ parse_register (char *reg_string, char **end_op)
{
const expressionS *e = symbol_get_value_expression (symbolP);
- know (e->X_op == O_register);
- know (e->X_add_number >= 0
- && (valueT) e->X_add_number < i386_regtab_size);
- r = i386_regtab + e->X_add_number;
- if (!check_register (r))
+ if (e->X_op == O_register
+ && (valueT) e->X_add_number < i386_regtab_size)
{
- as_bad (_("register '%s%s' cannot be used here"),
- register_prefix, r->reg_name);
- r = &bad_reg;
+ r = i386_regtab + e->X_add_number;
+ if (!check_register (r))
+ {
+ as_bad (_("register '%s%s' cannot be used here"),
+ register_prefix, r->reg_name);
+ r = &bad_reg;
+ }
+ *end_op = input_line_pointer;
}
- *end_op = input_line_pointer;
}
*input_line_pointer = c;
input_line_pointer = save;
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: PR28977 tc-i386.c internal error in parse_register
2022-03-18 6:56 PR28977 tc-i386.c internal error in parse_register Alan Modra
@ 2022-03-18 7:12 ` Jan Beulich
2022-03-18 7:32 ` Alan Modra
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2022-03-18 7:12 UTC (permalink / raw)
To: Alan Modra; +Cc: binutils
On 18.03.2022 07:56, Alan Modra via Binutils wrote:
> PR 28977
> * config/tc-i386.c (parse_register): Handle X_op not O_register
> as for a non-reg_section symbol. Simplify array bounds check.
Hmm, isn't it that ...
> --- a/gas/config/tc-i386.c
> +++ b/gas/config/tc-i386.c
> @@ -12952,17 +12952,18 @@ parse_register (char *reg_string, char **end_op)
> {
... the if() right outside of context here is pointing at the actual
problem? Why would "s=%rdx % %rcx" result in a reg_section expression?
Imo this clearly ought to be expr_section.
Jan
> const expressionS *e = symbol_get_value_expression (symbolP);
>
> - know (e->X_op == O_register);
> - know (e->X_add_number >= 0
> - && (valueT) e->X_add_number < i386_regtab_size);
> - r = i386_regtab + e->X_add_number;
> - if (!check_register (r))
> + if (e->X_op == O_register
> + && (valueT) e->X_add_number < i386_regtab_size)
> {
> - as_bad (_("register '%s%s' cannot be used here"),
> - register_prefix, r->reg_name);
> - r = &bad_reg;
> + r = i386_regtab + e->X_add_number;
> + if (!check_register (r))
> + {
> + as_bad (_("register '%s%s' cannot be used here"),
> + register_prefix, r->reg_name);
> + r = &bad_reg;
> + }
> + *end_op = input_line_pointer;
> }
> - *end_op = input_line_pointer;
> }
> *input_line_pointer = c;
> input_line_pointer = save;
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: PR28977 tc-i386.c internal error in parse_register
2022-03-18 7:12 ` Jan Beulich
@ 2022-03-18 7:32 ` Alan Modra
2022-03-18 9:39 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Alan Modra @ 2022-03-18 7:32 UTC (permalink / raw)
To: Jan Beulich; +Cc: binutils
On Fri, Mar 18, 2022 at 08:12:46AM +0100, Jan Beulich wrote:
> On 18.03.2022 07:56, Alan Modra via Binutils wrote:
> > PR 28977
> > * config/tc-i386.c (parse_register): Handle X_op not O_register
> > as for a non-reg_section symbol. Simplify array bounds check.
>
> Hmm, isn't it that ...
>
> > --- a/gas/config/tc-i386.c
> > +++ b/gas/config/tc-i386.c
> > @@ -12952,17 +12952,18 @@ parse_register (char *reg_string, char **end_op)
> > {
>
> ... the if() right outside of context here is pointing at the actual
> problem? Why would "s=%rdx % %rcx" result in a reg_section expression?
> Imo this clearly ought to be expr_section.
Perhaps, but we are off in the weeds anyway.
The original fuzzer input had a completely crazy expression for "s".
s=%ymm5%%%!%%%%!%%%%%%%\x1d%%%%%%%%%%%%%%��������������������������%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%!%ebp%%%%%%%%%%%%%%%%%%M%%%%%%[s<��%[s<�����%%%%%��������������/+�������������%%%%[s<��%[s<�����%%%%'%%%%%%%%%%%%%%%%;%%%%%%!%%%%!%%%%%%%%%%%%%#NO
My testcase was a little tidier but still gives:
mad.s: Error: invalid operands (*GAS `reg' section* and *GAS `reg' section* sections) for `%' when setting `s'
The aim of the patch is to stop an abort *before* we decide the
expression is invalid. i386 parse_register was being called via
md_parse_name in gas/expr.c:operand.
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: PR28977 tc-i386.c internal error in parse_register
2022-03-18 7:32 ` Alan Modra
@ 2022-03-18 9:39 ` Jan Beulich
2022-03-18 12:03 ` Alan Modra
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2022-03-18 9:39 UTC (permalink / raw)
To: Alan Modra; +Cc: binutils
On 18.03.2022 08:32, Alan Modra wrote:
> On Fri, Mar 18, 2022 at 08:12:46AM +0100, Jan Beulich wrote:
>> On 18.03.2022 07:56, Alan Modra via Binutils wrote:
>>> PR 28977
>>> * config/tc-i386.c (parse_register): Handle X_op not O_register
>>> as for a non-reg_section symbol. Simplify array bounds check.
>>
>> Hmm, isn't it that ...
>>
>>> --- a/gas/config/tc-i386.c
>>> +++ b/gas/config/tc-i386.c
>>> @@ -12952,17 +12952,18 @@ parse_register (char *reg_string, char **end_op)
>>> {
>>
>> ... the if() right outside of context here is pointing at the actual
>> problem? Why would "s=%rdx % %rcx" result in a reg_section expression?
>> Imo this clearly ought to be expr_section.
>
> Perhaps, but we are off in the weeds anyway.
>
> The original fuzzer input had a completely crazy expression for "s".
> s=%ymm5%%%!%%%%!%%%%%%%\x1d%%%%%%%%%%%%%%��������������������������%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%!%ebp%%%%%%%%%%%%%%%%%%M%%%%%%[s<��%[s<�����%%%%%��������������/+�������������%%%%[s<��%[s<�����%%%%'%%%%%%%%%%%%%%%%;%%%%%%!%%%%!%%%%%%%%%%%%%#NO
>
> My testcase was a little tidier but still gives:
>
> mad.s: Error: invalid operands (*GAS `reg' section* and *GAS `reg' section* sections) for `%' when setting `s'
That's only with your change in place, I assume? I'm not seeing
this with your change not in place (i.e. on a slightly older
tree I'm working with).
> The aim of the patch is to stop an abort *before* we decide the
> expression is invalid. i386 parse_register was being called via
> md_parse_name in gas/expr.c:operand.
It still feels like your change is merely hiding a problem elsewhere.
Going from your example (and observing where the abort actually is
reported) I added a 3rd instance of x=s. Then the abort continues to
be reported on the 2nd instance. If things were working consistently,
I would expect it to happen either on the first instance or at the
end of the file (in this latter case the location reported would
simply be bogus).
I think the original know(e->X_op == O_register) was actually quite
appropriate when seeing a reg_section symbol come in. No reg_section
symbols violating this should ever be constructed, at least not for
x86. There might be architectures where such makes sense, albeit
code like this in expr.c:
else if (mode != expr_defer && segment == reg_section)
{
expressionP->X_op = O_register;
expressionP->X_add_number = S_GET_VALUE (symbolP);
}
makes me think such should never be put into existence. I seem to
have a vague recollection of, very long ago, having discussed with
you already the question of too little use of expr_section in the
course of expression evaluation (without any actual outcome as far
as changes to the code).
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: PR28977 tc-i386.c internal error in parse_register
2022-03-18 9:39 ` Jan Beulich
@ 2022-03-18 12:03 ` Alan Modra
2022-03-21 16:59 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Alan Modra @ 2022-03-18 12:03 UTC (permalink / raw)
To: Jan Beulich; +Cc: binutils
On Fri, Mar 18, 2022 at 10:39:42AM +0100, Jan Beulich wrote:
> On 18.03.2022 08:32, Alan Modra wrote:
> > My testcase was a little tidier but still gives:
> >
> > mad.s: Error: invalid operands (*GAS `reg' section* and *GAS `reg' section* sections) for `%' when setting `s'
>
> That's only with your change in place, I assume?
Yes.
> > The aim of the patch is to stop an abort *before* we decide the
> > expression is invalid. i386 parse_register was being called via
> > md_parse_name in gas/expr.c:operand.
>
> It still feels like your change is merely hiding a problem elsewhere.
If you think the patch is wrong, please feel free to revert it. Since
we are getting into this discussion, I guess I should have asked
permission.
I don't doubt there are problems elsewhere, but I have some experience
tweaking the expression evaluation code (as do you) and don't want to
go down that rabbit hole for the sake of correcting the ssegment in
erroneous expressions.
I most definitely do not want to spend a large amount of my time
fixing things that never occur in real assembly source, as opposed to
the ridiculuous stuff fed to gas by fuzzers.
> Going from your example (and observing where the abort actually is
> reported) I added a 3rd instance of x=s. Then the abort continues to
> be reported on the 2nd instance. If things were working consistently,
> I would expect it to happen either on the first instance or at the
> end of the file (in this latter case the location reported would
> simply be bogus).
Oh yes, I noticed the same. Just one instance of x=s doesn't hit the
problem either. There were rather a lot more duplicated assignments
in the original 17k of fuzzer garbage, and the symbol name wasn't a
nice "x"!
> I think the original know(e->X_op == O_register) was actually quite
> appropriate when seeing a reg_section symbol come in. No reg_section
> symbols violating this should ever be constructed, at least not for
> x86.
Might be difficult to enforce if forward references are allowed.
Heh, if you really want to get your teeth into this, try this one:
b=a
a=%rax
xor b,%rax
> There might be architectures where such makes sense, albeit
> code like this in expr.c:
>
> else if (mode != expr_defer && segment == reg_section)
> {
> expressionP->X_op = O_register;
> expressionP->X_add_number = S_GET_VALUE (symbolP);
> }
>
> makes me think such should never be put into existence. I seem to
> have a vague recollection of, very long ago, having discussed with
> you already the question of too little use of expr_section in the
> course of expression evaluation (without any actual outcome as far
> as changes to the code).
>
> Jan
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: PR28977 tc-i386.c internal error in parse_register
2022-03-18 12:03 ` Alan Modra
@ 2022-03-21 16:59 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2022-03-21 16:59 UTC (permalink / raw)
To: Alan Modra; +Cc: binutils
On 18.03.2022 13:03, Alan Modra wrote:
> Heh, if you really want to get your teeth into this, try this one:
>
> b=a
> a=%rax
> xor b,%rax
Now this one isn't very difficult to resolve (and actually make
work). What looks harder is how to make cleanly fail e.g.
b=a
xor b,%rax
a=%rax
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-21 16:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-18 6:56 PR28977 tc-i386.c internal error in parse_register Alan Modra
2022-03-18 7:12 ` Jan Beulich
2022-03-18 7:32 ` Alan Modra
2022-03-18 9:39 ` Jan Beulich
2022-03-18 12:03 ` Alan Modra
2022-03-21 16:59 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).