Re: [PATCH] eif: cope with huge section offsets

[Dorjoy Chowdhury <dorjoychy111@gmail.com>]

On Wed, Nov 6, 2024 at 11:58 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On Wed, Nov 6, 2024 at 6:54 PM Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
> >
> > On 11/6/24 09:49, Paolo Bonzini wrote:
> > > On Wed, Nov 6, 2024 at 6:47 PM Pierrick Bouvier
> > > <pierrick.bouvier@linaro.org> wrote:
> > >
> > >>>        for (int i = 0; i < MAX_SECTIONS; ++i) {
> > >>>            header->section_offsets[i] = be64_to_cpu(header->section_offsets[i]);
> > >>> +        if (header->section_offsets[i] > OFF_MAX) {
> > >>
> > >> Maybe we could add a comment that sections_offsets is unsigned, as it
> > >> can be confusing to read value > INT_MAX without more context.
> > >
> > > It does sound like OFF_MAX is related to section_offsets[], but it's
> > > actually related to off_t.  So the comparison is with the maximum
> > > value of off_t, which is signed.
> > >
> > > The problem would happen even if section_offsets[] was signed (for
> > > example off_t could be 32-bit).
> >
> > I'm a bit confused.
> > It works because section_offsets[i] is unsigned. If it was signed, and
> > sizeof(off_t) is 8, we can never satisfy "(int64) > INT64_MAX".
>
> The fact that you cannot satisfy "int64 > INT64_MAX" just means that
> on this system that erroneous condition is unreachable, but it could
> be reachable on others. (Actually the fact that section_offsets[] is
> unsigned does matter, because otherwise you'd nede a check against 0
> as well. But it doesn't matter for the correctness of *this* check
> against OFF_MAX).
>

I think instead of putting the check for > OFF_MAX inside
read_eif_header, it would make more sense to put the check in the
read_eif_file function before the fseek line where we are actually
doing the seeking, no? What do you think?

Regards,
Dorjoy