diff mbox series

as: fix bpf expression parsing regression

Message ID 20240216140501.1039645-1-hawkinsw@obs.cr
State New
Headers
Series as: fix bpf expression parsing regression |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Testing passed

Commit Message

Will Hawkins Feb. 16, 2024, 2:04 p.m. UTC 
  As a result of a switch instead of an if, as would issue non-specific
error messages when it encountered an operand it could not parse in bpf.
This patch fixes that regression and adds a test to prevent it from
reoccurring.

Tested for bpf-unknown-none on x86_64-redhat-linux.

gas/ChangeLog:

	* config/tc-bpf.c (parse_expression): Change switch to if so that error
	* condition is handled.
	* testsuite/gas/bpf/bpf.exp: Invoke new test.
	* testsuite/gas/bpf/indcall-badoperand.d: New test.
	* testsuite/gas/bpf/indcall-badoperand.l: New test.
	* testsuite/gas/bpf/indcall-badoperand.s: New test.

Signed-off-by: Will Hawkins <hawkinsw@obs.cr>
---
 gas/config/tc-bpf.c                        | 2 +-
 gas/testsuite/gas/bpf/bpf.exp              | 3 +++
 gas/testsuite/gas/bpf/indcall-badoperand.d | 3 +++
 gas/testsuite/gas/bpf/indcall-badoperand.l | 3 +++
 gas/testsuite/gas/bpf/indcall-badoperand.s | 8 ++++++++
 5 files changed, 18 insertions(+), 1 deletion(-)
 create mode 100644 gas/testsuite/gas/bpf/indcall-badoperand.d
 create mode 100644 gas/testsuite/gas/bpf/indcall-badoperand.l
 create mode 100644 gas/testsuite/gas/bpf/indcall-badoperand.s

Comments

Nick Clifton Feb. 19, 2024, 11:16 a.m. UTC | #1 
Hi Will,

> As a result of a switch instead of an if, as would issue non-specific
> error messages when it encountered an operand it could not parse in bpf.
> This patch fixes that regression and adds a test to prevent it from
> reoccurring.
> 
> Tested for bpf-unknown-none on x86_64-redhat-linux.

Approved - please apply.

> -  switch (exp->X_op == O_absent || exp_parse_failed)
> +  if (exp->X_op == O_absent || exp_parse_failed)
>       return NULL;

I have to say that I am surprised that the original version of this
code compiled at all.  Or at least compiled without generating any
kind of warning message.

Cheers
   Nick
Will Hawkins Feb. 19, 2024, 4:32 p.m. UTC | #2 
On Mon, Feb 19, 2024 at 6:16 AM Nick Clifton <nickc@redhat.com> wrote:
>
> Hi Will,
>
> > As a result of a switch instead of an if, as would issue non-specific
> > error messages when it encountered an operand it could not parse in bpf.
> > This patch fixes that regression and adds a test to prevent it from
> > reoccurring.
> >
> > Tested for bpf-unknown-none on x86_64-redhat-linux.
>
> Approved - please apply.
>
> > -  switch (exp->X_op == O_absent || exp_parse_failed)
> > +  if (exp->X_op == O_absent || exp_parse_failed)
> >       return NULL;
>
> I have to say that I am surprised that the original version of this
> code compiled at all.  Or at least compiled without generating any
> kind of warning message.

Funny you mentioned that! I only saw this problem because I compiled
with clang on macOS and it failed to compile. There are warnings for
gcc that will detect this idiom
(https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wswitch-bool)
and an unreachable body of a switch
(https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wswitch-unreachable)
but for some reason (and I tried multiple invocations) this did not
trigger it.

I hope the patch helps!

Thank you for accepting it!

Sincerely,
Will


>
> Cheers
>    Nick
>
>
Jose E. Marchesi Feb. 19, 2024, 6:19 p.m. UTC | #3 
Hi Will.

Thanks for the patch.

> diff --git a/gas/config/tc-bpf.c b/gas/config/tc-bpf.c
> index 43e098c2a86..86489c72898 100644
> --- a/gas/config/tc-bpf.c
> +++ b/gas/config/tc-bpf.c
> @@ -1240,7 +1240,7 @@ parse_expression (char *s, expressionS *exp)
>    s = input_line_pointer;
>    input_line_pointer = saved_input_line_pointer;
>  
> -  switch (exp->X_op == O_absent || exp_parse_failed)
> +  if (exp->X_op == O_absent || exp_parse_failed)
>      return NULL;

Uoh!  How did that end there... ^^

> diff --git a/gas/testsuite/gas/bpf/indcall-badoperand.l b/gas/testsuite/gas/bpf/indcall-badoperand.l
> new file mode 100644
> index 00000000000..d791435a2ac
> --- /dev/null
> +++ b/gas/testsuite/gas/bpf/indcall-badoperand.l
> @@ -0,0 +1,3 @@
> +.*: Assembler messages:
> +.*:7: Error: unrecognized instruction `call %0'
> +.*:7: Error: expected register name, got '%0'
> diff --git a/gas/testsuite/gas/bpf/indcall-badoperand.s b/gas/testsuite/gas/bpf/indcall-badoperand.s
> new file mode 100644
> index 00000000000..cf19c0a56b6
> --- /dev/null
> +++ b/gas/testsuite/gas/bpf/indcall-badoperand.s
> @@ -0,0 +1,8 @@
> +
> +    .text
> +    .align 4
> +main:
> +
> +    mov %r0, 1
> +    call %0

What is this test supposed to test exactly?

> +    exit
Jose E. Marchesi Feb. 19, 2024, 6:52 p.m. UTC | #4 
>> diff --git a/gas/testsuite/gas/bpf/indcall-badoperand.l b/gas/testsuite/gas/bpf/indcall-badoperand.l
>> new file mode 100644
>> index 00000000000..d791435a2ac
>> --- /dev/null
>> +++ b/gas/testsuite/gas/bpf/indcall-badoperand.l
>> @@ -0,0 +1,3 @@
>> +.*: Assembler messages:
>> +.*:7: Error: unrecognized instruction `call %0'
>> +.*:7: Error: expected register name, got '%0'
>> diff --git a/gas/testsuite/gas/bpf/indcall-badoperand.s b/gas/testsuite/gas/bpf/indcall-badoperand.s
>> new file mode 100644
>> index 00000000000..cf19c0a56b6
>> --- /dev/null
>> +++ b/gas/testsuite/gas/bpf/indcall-badoperand.s
>> @@ -0,0 +1,8 @@
>> +
>> +    .text
>> +    .align 4
>> +main:
>> +
>> +    mov %r0, 1
>> +    call %0
>
> What is this test supposed to test exactly?

Never mind, now that I reread your mail it is pretty obvious.

I tried your patch, and I am getting some non-deterministic behavior.
Assembling indcall-badoperand.s the assembler sometimes emits:

  foo.s: Assembler messages:
  foo.s:7: Error: unrecognized instruction `call %0'
  foo.s:7: Error: expected signed 32-bit displacement

and sometimes it emits:

  foo.s: Assembler messages:
  foo.s:7: Error: unrecognized instruction `call %0'
  foo.s:7: Error: expected register name, got '%0'

Running under valgrind seems to settle on this latest, and expected,
output, so this seems to be a memory problem.  I will apply your patch
(on your behalf) only after I find and fix this other bug, or the
buildbots will go banana.

>> +    exit
Jose E. Marchesi Feb. 19, 2024, 7:53 p.m. UTC | #5 
>>> diff --git a/gas/testsuite/gas/bpf/indcall-badoperand.l b/gas/testsuite/gas/bpf/indcall-badoperand.l
>>> new file mode 100644
>>> index 00000000000..d791435a2ac
>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/bpf/indcall-badoperand.l
>>> @@ -0,0 +1,3 @@
>>> +.*: Assembler messages:
>>> +.*:7: Error: unrecognized instruction `call %0'
>>> +.*:7: Error: expected register name, got '%0'
>>> diff --git a/gas/testsuite/gas/bpf/indcall-badoperand.s b/gas/testsuite/gas/bpf/indcall-badoperand.s
>>> new file mode 100644
>>> index 00000000000..cf19c0a56b6
>>> --- /dev/null
>>> +++ b/gas/testsuite/gas/bpf/indcall-badoperand.s
>>> @@ -0,0 +1,8 @@
>>> +
>>> +    .text
>>> +    .align 4
>>> +main:
>>> +
>>> +    mov %r0, 1
>>> +    call %0
>>
>> What is this test supposed to test exactly?
>
> Never mind, now that I reread your mail it is pretty obvious.
>
> I tried your patch, and I am getting some non-deterministic behavior.
> Assembling indcall-badoperand.s the assembler sometimes emits:
>
>   foo.s: Assembler messages:
>   foo.s:7: Error: unrecognized instruction `call %0'
>   foo.s:7: Error: expected signed 32-bit displacement
>
> and sometimes it emits:
>
>   foo.s: Assembler messages:
>   foo.s:7: Error: unrecognized instruction `call %0'
>   foo.s:7: Error: expected register name, got '%0'
>
> Running under valgrind seems to settle on this latest, and expected,
> output, so this seems to be a memory problem.  I will apply your patch
> (on your behalf) only after I find and fix this other bug, or the
> buildbots will go banana.

Hi Will.

Found the cause and fixed it [1] then applied your patch on your behalf,
with a slightly modified ChangeLog.

Thank you!

[1] b86b514aace19799ea141514e16296fb63a089b3
Will Hawkins Feb. 19, 2024, 10:22 p.m. UTC | #6 
On Mon, Feb 19, 2024 at 2:54 PM Jose E. Marchesi
<jose.marchesi@oracle.com> wrote:
>
>
> >>> diff --git a/gas/testsuite/gas/bpf/indcall-badoperand.l b/gas/testsuite/gas/bpf/indcall-badoperand.l
> >>> new file mode 100644
> >>> index 00000000000..d791435a2ac
> >>> --- /dev/null
> >>> +++ b/gas/testsuite/gas/bpf/indcall-badoperand.l
> >>> @@ -0,0 +1,3 @@
> >>> +.*: Assembler messages:
> >>> +.*:7: Error: unrecognized instruction `call %0'
> >>> +.*:7: Error: expected register name, got '%0'
> >>> diff --git a/gas/testsuite/gas/bpf/indcall-badoperand.s b/gas/testsuite/gas/bpf/indcall-badoperand.s
> >>> new file mode 100644
> >>> index 00000000000..cf19c0a56b6
> >>> --- /dev/null
> >>> +++ b/gas/testsuite/gas/bpf/indcall-badoperand.s
> >>> @@ -0,0 +1,8 @@
> >>> +
> >>> +    .text
> >>> +    .align 4
> >>> +main:
> >>> +
> >>> +    mov %r0, 1
> >>> +    call %0
> >>
> >> What is this test supposed to test exactly?
> >
> > Never mind, now that I reread your mail it is pretty obvious.
> >
> > I tried your patch, and I am getting some non-deterministic behavior.
> > Assembling indcall-badoperand.s the assembler sometimes emits:
> >
> >   foo.s: Assembler messages:
> >   foo.s:7: Error: unrecognized instruction `call %0'
> >   foo.s:7: Error: expected signed 32-bit displacement
> >
> > and sometimes it emits:
> >
> >   foo.s: Assembler messages:
> >   foo.s:7: Error: unrecognized instruction `call %0'
> >   foo.s:7: Error: expected register name, got '%0'
> >
> > Running under valgrind seems to settle on this latest, and expected,
> > output, so this seems to be a memory problem.  I will apply your patch
> > (on your behalf) only after I find and fix this other bug, or the
> > buildbots will go banana.
>
> Hi Will.
>
> Found the cause and fixed it [1] then applied your patch on your behalf,
> with a slightly modified ChangeLog.
>
> Thank you!
>

Glad that I could help! I really, really enjoy working on FOSS! Thank
you and Nick for being so welcoming!

Can't wait to hear your thoughts on the gcc patch I submitted!

Sincerely,
Will


> [1] b86b514aace19799ea141514e16296fb63a089b3
diff mbox series

Patch

diff --git a/gas/config/tc-bpf.c b/gas/config/tc-bpf.c
index 43e098c2a86..86489c72898 100644
--- a/gas/config/tc-bpf.c
+++ b/gas/config/tc-bpf.c
@@ -1240,7 +1240,7 @@  parse_expression (char *s, expressionS *exp)
   s = input_line_pointer;
   input_line_pointer = saved_input_line_pointer;
 
-  switch (exp->X_op == O_absent || exp_parse_failed)
+  if (exp->X_op == O_absent || exp_parse_failed)
     return NULL;
 
   /* The expression parser may consume trailing whitespaces.  We have
diff --git a/gas/testsuite/gas/bpf/bpf.exp b/gas/testsuite/gas/bpf/bpf.exp
index dae8bd924d0..5faae5b859e 100644
--- a/gas/testsuite/gas/bpf/bpf.exp
+++ b/gas/testsuite/gas/bpf/bpf.exp
@@ -77,6 +77,9 @@  if {[istarget bpf*-*-*]} {
     run_dump_test disp32-overflow
     run_dump_test imm32-overflow
 
+    # Bad operand (regression)
+    run_dump_test indcall-badoperand
+
     # In Pseudo-C it is not possible to refer to symbols
     # as operands that have the same name than registers.
     run_dump_test regs-for-symbols-pseudoc
diff --git a/gas/testsuite/gas/bpf/indcall-badoperand.d b/gas/testsuite/gas/bpf/indcall-badoperand.d
new file mode 100644
index 00000000000..bf2e9e8e643
--- /dev/null
+++ b/gas/testsuite/gas/bpf/indcall-badoperand.d
@@ -0,0 +1,3 @@ 
+#as: -EL -mno-relax
+#source: indcall-badoperand.s
+#error_output: indcall-badoperand.l
diff --git a/gas/testsuite/gas/bpf/indcall-badoperand.l b/gas/testsuite/gas/bpf/indcall-badoperand.l
new file mode 100644
index 00000000000..d791435a2ac
--- /dev/null
+++ b/gas/testsuite/gas/bpf/indcall-badoperand.l
@@ -0,0 +1,3 @@ 
+.*: Assembler messages:
+.*:7: Error: unrecognized instruction `call %0'
+.*:7: Error: expected register name, got '%0'
diff --git a/gas/testsuite/gas/bpf/indcall-badoperand.s b/gas/testsuite/gas/bpf/indcall-badoperand.s
new file mode 100644
index 00000000000..cf19c0a56b6
--- /dev/null
+++ b/gas/testsuite/gas/bpf/indcall-badoperand.s
@@ -0,0 +1,8 @@ 
+
+    .text
+    .align 4
+main:
+
+    mov %r0, 1
+    call %0
+    exit