Re: [PATCH] Fix an out of bounds array access in, find_epilogue_using_linetable

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 4/5/24 17:11, Tom de Vries wrote:
> 
> Looking at the "pointer exactly at the upper bound of the line table" problem a bit more, I think it can happen that it->epilogue_begin is evaluated for an end_sequence entry.  My guess is that this is harmless, but it's better to avoid it.
> 
> Atm it's hard to reason about what happens because we try to handle several initial scenarios in the loop.  It's best to untangle this:
> - handle exceptional situations before the loop, and
> - reserve the loop to handle the steady state: iterating over
>   proper line-table entries of the current function.
> 
>>> I think things can be solved simpler by:
>>> - using a trivial for loop that expresses the boundary of the line
>>>    table, and
>>> - moving the while condition into the loop, while
>>> - handling the exceptional case explictly, outside the loop:
>>> ...
>>>        if (it == &linetable->item[linetable->nitems])
>>>          it--;
>>>
>>
>> no, that is also wrong if linetable->nitems == 0, `it` is
>> &linetable->item[-1], and the comparing that value to
>> &linetable->item[0] yields an undefined boolean value.
> 
> I see, I forgot about that, thanks for pointing that out.  It's good to make explicit in the implementation that and where we're avoid this.
> 
> Btw, if "linetable->nitems == 0" indeed can happen, it's best handled using an early-out.
> 
>> And as I said above looking at the line table entry
>> that belongs to the next function is asking for trouble.
>>
>> Even decrementing `it` below the first element of an array
>> is undefined behavior, avoiding that is the main reason
>> why I wrote the if condition in this way.
>>
>> I would agree to add a comment here which explains how the
>> loop works.
>>
>> OK?
>>
> 
> I've written a version that avoids the undefined behaviour decrement, clearly advertises this, handles cornercases explicitly and adds and reorganizes comments, as well as adds some asserts.  I've submitted it as a v3, which also adds an additional test-case.
> 

Ah okay,
I forgot about the end_sequence, that should be at the upper bound of the function range,
but it should not be possible, that it has an epilogue-begin bit set in this line table
item, see dwarf_finish_line, there is only a IS_STMT flag, no LEF_EPILOGUE_BEGIN:

dwarf_finish_line (struct gdbarch *gdbarch, struct subfile *subfile,
                   unrelocated_addr address, struct dwarf2_cu *cu,
                   bool end_sequence) 
{           
  if (subfile == NULL)
    return;

  if (dwarf_line_debug)
    {
      gdb_printf (gdb_stdlog,
                  "Finishing current line, file %s, address %s\n",
                  lbasename (subfile->name.c_str ()),
                  paddress (gdbarch, (CORE_ADDR) address));
    }

  dwarf_record_line_1 (gdbarch, subfile, end_sequence ? 0 : -1, address,
                       LEF_IS_STMT, cu);
}

So it should not be necessary to check end-sequence for epilogue-begin bit.
 
But even if an epilogue-begin line table entry would be at the same address,
that can only happen due to an invalid debug info: That would by my reasoning
be impossible, because that would imply a zero byte epilogue and that
cannot be correct because the epilogue consists of at least a return instruction.
Therefore I'd suggest to skip that line table entry, because even if it has the
epilogue-begin bit set, it can only be invalid and it is better ignored.


Thanks,
Bernd.



> Thanks,
> - Tom
>