[webkit-reviews] review granted: [Bug 184883] Correctly detect string overflow when using the 'Function' constructor : [Attachment 353290] proposed patch.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 29 16:28:27 PDT 2018


Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 184883: Correctly detect string overflow when using the 'Function'
constructor
https://bugs.webkit.org/show_bug.cgi?id=184883

Attachment 353290: proposed patch.

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




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

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

> Source/WTF/ChangeLog:40
> +	      However, I was not able to get clang to export the explicitly
instantiated
> +	      instances (despite the methods having being decorated with
WTF_EXPORT_PRIVATE).
> +	      Since the StringBuilder is a transient object (not stored for a
long time on
> +	      the heap), and the runtime check of the boolean is not that
costly compared
> +	      to other work that StringBuilder routinely does (e.g. memcpy),
using the
> +	      new ConditionalCrashOnOverflow (which does a runtime check to
determine to
> +	      crash or not on overflow) is fine.

I don't think this is the end of the world, but it seems like this would be
strictly better if you made it a template parameter. Can you at least file a
bug? Not doing something because it's tricky to get it to compile correctly
isn't a very convincing reason.

> Source/WTF/wtf/CheckedArithmetic.h:231
> +    typedef typename RemoveChecked<T>::CleanType CleanType;

nit: I'd use 'using' here:
using CleanType = typename RemoveChecked<T>::CleanType;

> Source/WTF/wtf/text/StringBuilder.cpp:40
> +    return std::max(requiredLength, std::max(minimumCapacity,
std::min(capacity * 2,
static_cast<unsigned>(roundUpToMultipleOf<16>(String::MaxLength)))));

Why 16? Just because that's the minimum? If so, you should use it by name.

Why not just round to multiple of 2?

> Source/WTF/wtf/text/StringBuilder.cpp:43
> +

please revert


More information about the webkit-reviews mailing list