[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