Hi,
This is a design question, not a patch series. I have not started an
implementation and would like maintainer direction before doing so.
While reviewing the libdw DWARF parsers, I noticed an asymmetry in the
reading layer.
get_uleb128/get_sleb128 take an end pointer, and explicitly named unchecked
LEB variants exist for already validated input. The incrementing fixed-width
readers, however, such as:
read_2ubyte_unaligned_inc
read_3ubyte_unaligned_inc
read_4ubyte_unaligned_inc
read_8ubyte_unaligned_inc
read_addr_unaligned_inc
do not take an end pointer. Each caller must perform its own bounds check
before the read. This creates a caller-discipline requirement that has
occasionally been missed.
Examples involving this fixed-width-read pattern include:
- .debug_macro line_offset before read_addr_unaligned_inc,
- .debug_str_offsets unit_length before a fixed four-byte read,
- .debug_rnglists unit_length before a fixed four-byte read,
- .debug_loclists unit_length before a fixed four-byte read.
There are also related one-byte cursor cases, such as minimum_instr_len via
*linep++ and the CFI advance_loc1 fix via *program++. These have a similar
caller-side boundary-check requirement but are not uses of the fixed-width
reader macros.
I want to be precise: this proposal concerns the fixed-width-read subclass,
not every possible libdw bounds error, and I am not claiming that all recent
parser fixes share one root cause.
Sayed, you have recently contributed several bounds fixes in this area. I am
asking whether a central helper could complement that work rather than cut
across or supersede it.
A preliminary source census found about 42 incrementing fixed-width read
call
sites in libdw. For the sites reviewed, an end, limit, bytes_end, or
equivalent
boundary was already available in the surrounding parser, so a checked
reader
would not appear to require new boundary plumbing.
One possible shape, expressed in existing project style rather than as a new
public API, would be:
- a checked fixed-width reader receives the cursor and end boundary;
- a short read follows the caller's existing goto/error path, similar to
cfi_assert or "goto invalid";
- the cursor advances only after a complete successful read;
- explicitly named _unchecked variants remain for proven prevalidated
paths,
mirroring the existing LEB checked/unchecked split;
- migration could be staged, starting with historically affected parsers;
- the current ambiguous raw forms could eventually be restricted if that
is
considered desirable.
Valid-input behavior and the public ABI/API should remain unchanged. The
intent
would only be to reject truncated input earlier and more uniformly. Any
implementation would also include before/after measurements for line-program
and CFI parsing.
I would appreciate your guidance on the following:
1. Would you prefer a central checked fixed-width reader family, or
continued
explicit caller-side checks?
2. Should failure use a cfi_assert/goto-style macro, a bool-returning
inline
helper, or another existing project idiom?
3. Should an initial effort cover only libdw, or all users of these shared
macros, including libdwfl, readelf and libcpu?
4. Would a checked-by-default plus explicitly named _unchecked split be
acceptable, following the existing LEB design?
5. Would you prefer one complete conversion or a staged series beginning
with
the historically affected parser families?
If you would rather continue handling these checks locally on a case-by-case
basis, that is also a perfectly reasonable answer, and I will not pursue the
larger change.
Thanks,
Karan
