[PATCH] D86422: [ELF] .note.gnu.property: error for invalid pr_datasize

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 3 inline comments as done.
MaskRay added inline comments.


================
Comment at: lld/ELF/InputFiles.cpp:797
+    if (data.size() < sizeof(Elf_Nhdr) || data.size() < nhdr->getSize())
+      reportFatal(data, "data is too short");
 
----------------
psmith wrote:
> We may be able to be more precise with the size as I think data.size() = sizeof(Elf_Nhdr) + nhdr->getSize()
This is precise: `nhdr->getSize() = sizeof(Elf_Nhdr) + n_descsz`.




================
Comment at: lld/ELF/InputFiles.cpp:818
+      if (desc.size() < 8 + size)
+        reportFatal(desc, "program property is too short");
 
----------------
grimar wrote:
> grimar wrote:
> > 2 points:
> > 
> > 1) You should be able to use `read32` from `Target.h`:
> > 
> > ```
> > inline uint32_t read32(const void *p) {
> >   return llvm::support::endian::read32(p, config->endianness);
> > }
> > ```
> > 
> > 2) It probably would read slightly better if it was:
> > 
> > ```
> > if (desc.size() < 8)
> >   reportFatal(...);
> > 
> > uint32_t type = read32(desc.data());
> > uint32_t size = read32(desc.data() + 4);
> > desc = desc.drop_front(8);
> > 
> > if (desc.size() < size)
> >   reportFatal(desc, "program property is too short");
> > ```
> > 
> > i.e
> > a) I'd not start reading the `size` + `type` when the data is truncated (just like the code on the left).
> > b) Early dropping of 8 bytes at start should allow to avoid the need of `+ 8` in the code below.
> > I'd not start reading the size + type when the data is truncated
> 
> The code indeed does not starts reading them, I meant I'd just return earlier when it is already known that we can't read the header.
1. This file does not include Target.h. I don't want to additional introduce an inter-module dependency. I think read32 with an explicit template argument is equally readable (slightly more efficient in performance and better codegen).

2. Changed.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86422/new/

https://reviews.llvm.org/D86422