[PATCH] D43958: [llvm-readobj][ELF] Move ELF note parsing into lib/Object

Rafael Avila de Espindola via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 6 10:14:07 PST 2018


Scott Linder via Phabricator <reviews at reviews.llvm.org> writes:

> scott.linder updated this revision to Diff 137080.
> scott.linder added a comment.
>
> This patch should make the interfaces safe in any context. Bounds checks are done using sizes, and are completed before pointers are created. The checks are done only as necessary: the size of the container (section or program header) is checked before creating the iterator, the size of each `Nhdr` is checked before a pointer to it is created, and the sizes of the name/descriptor are checked before the StringRef/ArrayRef are created.
>
> Programmer errors, like continuing to iterate after reaching the end iterator or providing a non-success Error when creating the start iterator, are caught with asserts.

Thanks, that is exactly what we should aim for.

> +  auto ProcessNote = [&](const Elf_Note Note) {

Don't you want a reference?


> +      PrintHeader(P.p_offset, P.p_filesz);
> +      Error Err = Error::success();
> +      for (const auto &Note : Obj->notes(P, Err))
> +        ProcessNote(Note);
> +      if (Err)
> +        error(std::move(Err));

Don't you need to check Err on every loop iteration?

> +  Elf_Note_Iterator_Impl &operator++() {
> +    assert(Nhdr && "incremented ELF note end iterator");
> +    size_t NhdrSize = Nhdr->getSize();
> +    if (NhdrSize >= RemainingSize) {

The idea seems to be that we can create an iterator to a note that is
truncated, but we will detect that if we try to iterate past it or
access the truncated part with getDesc/getName.

That is fine and error handling at its most precise. On the other hand,
it might be simpler to error on an attempt at creating an iterator that
points to the truncated note. I am OK with both.

LGTM with the above nits.

Cheers,
Rafael


More information about the llvm-commits mailing list