[webkit-reviews] review granted: [Bug 202085] Introducing Integrity audit functions. : [Attachment 379429] proposed patch.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 202085: Introducing Integrity audit functions.
https://bugs.webkit.org/show_bug.cgi?id=202085

Attachment 379429: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=379429&action=review




--- Comment #14 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 379429
  --> https://bugs.webkit.org/attachment.cgi?id=379429
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=379429&action=review

r=me

> Source/JavaScriptCore/heap/Heap.cpp:1273
> +	   vm().random().setSeed(cryptographicallyRandomNumber()); // Piggy
back on the GC to randomize a bit.

nit: not a fan of comments that just says what the code is doing

> Source/JavaScriptCore/tools/VMInspectorInlines.h:39
> +#undef AUDIT_CONDITION
> +#undef AUDIT_VERIFY

why? Seems slightly gross, and shouldn't be needed

> Source/JavaScriptCore/tools/VMInspectorInlines.h:56
> +    if (isDynamicallySizedType(cellType)) {

nit: seems like this should be a helper that exists outside of VMInspector,
something like:

size_t cellSize(JSCell*);

It can then branch on isDynamicallySizedType or look in classinfo.

> Source/JavaScriptCore/tools/VMInspectorInlines.h:90
> +	   AUDIT_VERIFY(action, verifier, cellSize <= allocatorCellSize, cell,
cellType, cellSize, allocatorCellSize);

might be nice to also assert the dynamically sized cell size is >= class info's
cell size