[prev in list] [next in list] [prev in thread] [next in thread] 

List:       binutils
Subject:    Re: [PATCH] gas: fix building tc-bpf.c on s390x
From:       Ilya Leoshkevich via Binutils <binutils () sourceware ! org>
Date:       2023-04-28 9:37:58
Message-ID: 78d8001581eafec9a53ac440d6e43b8c52362068.camel () linux ! ibm ! com
[Download RAW message or body]

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

[prev in list] [next in list] [prev in thread] [next in thread]