[PATCH] D138830: [llvm] Check for overflows when computing load-command's addresses.

[James Y Knight via Phabricator via llvm-commits]

jyknight added inline comments.


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:202
+    if (Overflowed ||
+        Ret > reinterpret_cast<long unsigned int>(Obj.getData().end()))
       return malformedError("load command " + Twine(LoadCommandIndex) +
----------------
oontvoo wrote:
> tschuett wrote:
> > alexander-shaposhnikov wrote:
> > > nit: I'd probably report a separate message for the overflow case (cause it'd make debugging a tiny bit easier)
> > nit: could you instead use an integer with a defined bit width: uint64_t?
> > nit: could you instead use an integer with a defined bit width: uint64_t?
> 
> How about `uintptr_t` ?
`long unsigned int` should be uintptr_t here.
And I think the comparison should be `>=`?


================
Comment at: llvm/lib/Object/MachOObjectFile.cpp:230
+  bool Overflowed = false;
+  // FIXME: Perhaps this validation on the Obj file should have been
+  // done somewhere else (earlier).
----------------
Probably all of the checks here can go away, except to check that L.Ptr + L.C.cmdsize doesn't overflow.

All the range checks are done by getLoadCommandInfo anyways?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138830