[webkit-reviews] review granted: [Bug 176874] AddressSanitizer: stack-buffer-underflow in JSC::Probe::Page::Page : [Attachment 320822] 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 176874: AddressSanitizer: stack-buffer-underflow in JSC::Probe::Page::Page
https://bugs.webkit.org/show_bug.cgi?id=176874

Attachment 320822: proposed patch.

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




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

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

> Source/JavaScriptCore/assembler/ProbeStack.h:77
> +	       size_t numberOfChunks = sizeof(T) / s_chunkSize;

I think you want to round up not down here, especially when s_chunkSize doesn't
evenly divide sizeof(T).
Also, you could just static_assert(sizeof(T) % s_chunkSize == 0) if you want

> Source/JavaScriptCore/assembler/ProbeStack.h:122
> +    static constexpr size_t s_pageSize = is64Bit() ? 512 : 256; // 64 *
sizeof(uintptr_t)

Why not just write 64 * sizeof(uintptr_t) here instead of the comment?

> Source/JavaScriptCore/assembler/ProbeStack.h:132
> +    static constexpr size_t s_chunkSizeShift = is64Bit() ? 3 : 2;

maybe add:
static_assert(1 << chunkSizeShift == sizeof(uintptr_t), "")?

> Source/JavaScriptCore/assembler/testmasm.cpp:603
> +	       stack.set<double>(p++, 1.234567);

why is this needed?

> Source/JavaScriptCore/assembler/testmasm.cpp:634
> +	       CHECK_EQ(stack.get<double>(p++), 1.234567);

ditto

> Source/WTF/wtf/StdLibExtras.h:209
> +    return
reinterpret_cast<T*>(roundUpToMultipleOf<divisor>(reinterpret_cast<size_t>(x)))
;

Please add:
static_assert(sizeof(size_t) == sizeof(uintptr_t)
or
please use uintptr_t