[Webkit-unassigned] [Bug 185048] StringBuilder is fallible

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=185048

JF Bastien <jfbastien at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|webkit-unassigned at lists.web |jfbastien at apple.com
                   |kit.org                     |
 Attachment #338914|                            |review?, commit-queue?
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180426/a3aa6335/attachment-0001.html>