[PATCH] gas: fix building tc-bpf.c on s390x

[PATCH] gas: fix building tc-bpf.c on s390x

[Ilya Leoshkevich]

char is unsigned on s390x, so there are a lot of warnings like:

    gas/config/tc-bpf.c: In function 'get_token':
    gas/config/tc-bpf.c:900:14: error: comparison is always false due to limited range of data type [-Werror=type-limits]
      900 |       if (ch == EOF || len > MAX_TOKEN_SZ)
          |              ^~

Make ch explicitly signed.

There is also:

    gas/config/tc-bpf.c:735:30: error: 'bpf_endianness' may be used uninitialized in this function [-Werror=maybe-uninitialized]
      735 |    dst, be ? size[endianness - BPF_BE16] : size[endianness - BPF_LE16]);
          |                   ~~~~~~~~~~~^~~~~~~~~~

-Wmaybe-uninitialized doesn't seem to understand the FSM; just
initialize bpf_endianness to silence it.
---
 gas/config/tc-bpf.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gas/config/tc-bpf.c b/gas/config/tc-bpf.c
index 171fc682806..adcf5edc645 100644
--- a/gas/config/tc-bpf.c
+++ b/gas/config/tc-bpf.c
@@ -885,7 +885,7 @@ get_token (const char **insn, char *token, size_t *tlen)
     } while (0)
 
   const char *str = *insn;
-  char ch, ch2 = 0;
+  signed char ch, ch2 = 0;
   enum bpf_token_type ttype = BPF_UNKNOWN;
   size_t len = 0;
   const char *expr = NULL;
@@ -1362,7 +1362,7 @@ bpf_pseudoc_to_normal_syntax (const char *str, char **errmsg)
     } while (0)
 
   enum bpf_token_type ttype;
-  enum bpf_token_type bpf_endianness,
+  enum bpf_token_type bpf_endianness = BPF_UNKNOWN,
 		      bpf_atomic_insn;
   enum bpf_token_type bpf_jmp_op = BPF_JEQ; /* Arbitrary.  */
   enum bpf_token_type bpf_cast = BPF_CAST_U8; /* Arbitrary.  */
-- 
2.40.0

Re: [PATCH] gas: fix building tc-bpf.c on s390x

[Jan Beulich]

On 27.04.2023 14:56, Ilya Leoshkevich via Binutils wrote:
> --- a/gas/config/tc-bpf.c
> +++ b/gas/config/tc-bpf.c
> @@ -885,7 +885,7 @@ get_token (const char **insn, char *token, size_t *tlen)
>      } while (0)
>  
>    const char *str = *insn;
> -  char ch, ch2 = 0;
> +  signed char ch, ch2 = 0;

But this doesn't make things any better. If you grep for uses of EOF in
gas, you'll find that the corresponding variables typically are of type
int, and that's what I expect you want to use here as well.

> @@ -1362,7 +1362,7 @@ bpf_pseudoc_to_normal_syntax (const char *str, char **errmsg)
>      } while (0)
>  
>    enum bpf_token_type ttype;
> -  enum bpf_token_type bpf_endianness,
> +  enum bpf_token_type bpf_endianness = BPF_UNKNOWN,

The variable surely wants an initializer, but I'm uncertain whether the
one you picked is suitable. I don't know bpf, but I see only two options:
There is a default endianness, in which case that wants to be the
initializer. Or endianness needs to be specified explicitly before any
of the constructs leading to build_bpf_endianness() may be used. In that
case the initializer chosen is perhaps fine, but the variable then still
having that value would need to be diagnosed. With what you've done we
now end up with an out of bounds array access in build_bpf_endianness().

You would better have Cc-ed the arch maintainer anyway; doing so now.

Jan

Re: [PATCH] gas: fix building tc-bpf.c on s390x

[Ilya Leoshkevich]

On Fri, 2023-04-28 at 08:42 +0200, Jan Beulich wrote:
> On 27.04.2023 14:56, Ilya Leoshkevich via Binutils wrote:
> > --- a/gas/config/tc-bpf.c
> > +++ b/gas/config/tc-bpf.c
> > @@ -885,7 +885,7 @@ get_token (const char **insn, char *token,
> > size_t *tlen)
> >      } while (0)
> >  
> >    const char *str = *insn;
> > -  char ch, ch2 = 0;
> > +  signed char ch, ch2 = 0;
> 
> But this doesn't make things any better. If you grep for uses of EOF
> in
> gas, you'll find that the corresponding variables typically are of
> type
> int, and that's what I expect you want to use here as well.

You are right, int is better. Thanks.

> > @@ -1362,7 +1362,7 @@ bpf_pseudoc_to_normal_syntax (const char
> > *str, char **errmsg)
> >      } while (0)
> >  
> >    enum bpf_token_type ttype;
> > -  enum bpf_token_type bpf_endianness,
> > +  enum bpf_token_type bpf_endianness = BPF_UNKNOWN,
> 
> The variable surely wants an initializer, but I'm uncertain whether
> the
> one you picked is suitable. I don't know bpf, but I see only two
> options:
> There is a default endianness, in which case that wants to be the
> initializer. Or endianness needs to be specified explicitly before
> any
> of the constructs leading to build_bpf_endianness() may be used. In
> that
> case the initializer chosen is perhaps fine, but the variable then
> still
> having that value would need to be diagnosed. With what you've done
> we
> now end up with an out of bounds array access in
> build_bpf_endianness().
> 
> You would better have Cc-ed the arch maintainer anyway; doing so now.

In that regard the patch does not make things worse.
If we end up not initializing the variable on the intended path, with
today's code we would still have an OOB access (but with a random
offset). The goal here is only to silence the warning, which I believe
is emitted incorrectly.

If I read the FSM correctly, this can only happen due to a bug, no
user input (valid or invalid) should be leading to this. So, just to be
on the safe side, I would add gas_assert() to build_bpf_endianness().

> Jan