[webkit-reviews] review requested: [Bug 185048] StringBuilder is fallible : [Attachment 338914] patch

[bugzilla-daemon at webkit.org]

JF Bastien <jfbastien at apple.com> has asked  for review:
Bug 185048: StringBuilder is fallible
https://bugs.webkit.org/show_bug.cgi?id=185048

Attachment 338914: patch

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




--- Comment #1 from JF Bastien <jfbastien at apple.com> ---
Created attachment 338914

  --> https://bugs.webkit.org/attachment.cgi?id=338914&action=review

patch

>From the WTF ChangeLog:

StringBuilder is fallible

StringBuilder can fail when allocating memory. StringBuilder is mostly used
with user input, which often means that just crashing isn't the right outcome.
Our fuzzers often encounter such things for JavaScriptCore, and so far we've
applied point solutions. No more! This patch makes StringBuilder fallible and
enforces this in two ways:

  * Each step of building a string can be checked for failure because they now
return a "Fallible<T>". This can be ignored at every step!
  * The final step of StringBuilder, getting the string out, returns a
"Fallible<String>" which is marked with WARN_UNUSED_RETURN. Failure is
communicated if any of the building steps failed.

Doing this two-step approach means that code can continue operating as it did
before (append a bunch and then get a string) and only needs to care about the
string potentially not being available. This is enforced by the type system:
Fallible<T> is std::expected<T, StringBuilder::Failure>.

The following intermediate functions can now be observed to fail (or ignored):

  * append
  * appendUninitialized
  * appendUninitializedSlow
  * appendQuotedJSONString used to return false on failure, it now returns
Fallible<>
  * resize
  * allocateBuffer
  * allocateBufferUpConvert
  * reallocateBuffer
  * reserveCapacity
  * shrinkToFit
  * operator[]
  * characters8
  * characters16

Failure in these methods comes in two forms: out of memory, and would overflow.
When an operation would overflow it leaves the StringBuilder in its previous
state, allowing users to resume work, keep the string as it existed before
failure. When we just fail allocating it means we expected to succeed, so
clearing the StringBuilder is the right thing to do.

Supporting failure in StringBuilder required adding "try" variants of some
methods in StringImpl.

This patch updates all WebKit code which uses StringBuilder. It leaves all
intermediate function usage alone (code still appends back-to-back, blissfully
unaware of failure), and only checks for failure when retrieving the string.
The pattern used is usually:

  if (auto string = builder.toString())
    foo(*string); // Use string as before...
  else
    CRASH();

This makes failure extremely explicit. There's no true behavior change: what
would have crashed when appending now crashes when retrieving the string.
Pulling CRASH() out will give slightly better crash signatures, and it makes
the code very obvious about failure. I purposefully don't "fix" any failure
because these should be done in a targeted manner with repro tests whenever
possible. I'm very wary of introducing bugs!

When someone inevitably blames code back to this commit (because hey the code
is pretty ugly, and you might be surprised at seeing CRASH() in your beautiful
code) there are a few things which they can do:

  1. Change the else clause to handle failure rather than crash. In
JavaScriptCore that's usually throwOutOfMemoryError.
  2. Change the code to directly use Fallible's .value() member, and let *that*
crash instead. This makes the crash non-obvious in the code, which might be
what you want.
  3. Change the code to detect failure at the earliest step of string building
(likely the append calls), and do something sensible there.

In no circumstance should code just dereference a Fallible without knowing that
its state holds a valid string. This code is undefined behavior according to
the std::expected specification (whether our implementation should do something
else is out of scope for this patch):

  foo(*builder.toString()); // UB when allocation fails.