On Wed, Nov 6, 2024 at 6:54 PM Pierrick Bouvier
<pierrick.bouv...@linaro.org> wrote:
>
> On 11/6/24 09:49, Paolo Bonzini wrote:
> > On Wed, Nov 6, 2024 at 6:47 PM Pierrick Bouvier
> > <pierrick.bouv...@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).
Paolo
