[PATCH] D70762: scudo: Add initial memory tagging support.

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:242
+      //
+      // This condition is not necessarily unlikely, but since memset is costly,
+      // we might as well mark it as such. When memory tagging is enabled,
----------------
Maybe move this part of the comment to where the memset is?


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:248
+        Chunk::UnpackedHeader Header;
+        uptr BlockEnd = BlockUptr + PrimaryT::getSizeByClassId(ClassId);
+        // If possible, try to reuse the UAF tag that was set by deallocate().
----------------
There already is a `BlockEnd` variable outside this scope used for the Secondary, maybe reuse it?
It might be cleaner to initialize it where the allocate is for the Primary, but then performance will suffer due to the extra `getSizeByClassId` in the fast path, which is not ideal.
If you want to keep it in this block, it seems to be `const`.


================
Comment at: compiler-rt/lib/scudo/standalone/combined.h:280
+          uptr PrevEnd = TaggedUserPtr + Header.SizeOrUnusedBytes;
+          uptr NextPage = roundUpTo(TaggedUserPtr, getPageSizeCached());
+          if (NextPage < PrevEnd && loadTag(NextPage) != NextPage)
----------------
`const`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70762