Bug 100173 – Re-order variables in BidiRun and LayoutState

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Chris Evans]

(pending)

[Chris Evans]

Ok, with some variable re-ordering in a couple of the layout objects, it's possible to
1) Achieve some space savings.
2) Enhance security against use-after-free slightly.

The space savings are to be honest far more interesting. BidiRun goes from 32 bytes to 24 bytes on 64-bit (20 bytes to 16 bytes on 32-bit), and despite the name, BidiRun is used very copiously in text rendering in general (e.g. triggers liberally on western language pages.)

The use-after-free situation is subtle but worth documenting. Most object slots in the RenderArena now start with either a valid vtable pointer when allocated or a poisoned freelist pointer (high bit set) when freed. The exceptions are BidiRun and LayoutState. Pre-patch, the attacker gets some control of every byte of the first sizeof(void*) bytes, which is an unfortunately overlap with a vtable pointer. Post-patch, this is not the case.

[Chris Evans]

Created attachment 171360 [details]
Patch

[Chris Evans]

Abhishek, I'll deal with the RenderArena memory leak in a separate bug / patch.

[WebKit Review Bot]

Attachment 171360 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/text/BidiResolver.h:152:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Evans]

Created attachment 171361 [details]
Patch

[Abhishek Arya]

Comment on attachment 171361 [details]
Patch

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

> Source/WebCore/rendering/LayoutState.h:119
> +    bool m_dummy;

Do you really need this m_dummy ?

> Source/WebCore/ChangeLog:8
> +        Saves 8 bytes out of 48 per BidiRun on 64-bit.

Please do explain like "Saves 8 bytes out of 48 per BidiRun on 64-bit by packing boolean m_hasHyphen in parent class BidiCharacterRun". Also, please explain how reordering in LayoutState helps to improve security (like which vars are directly controller by attacker and what breaks the exploit). This explanation really helps people to understand this change and also prevent from making changes to it.

[Eric Seidel (no email)]

Comment on attachment 171361 [details]
Patch

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

> Source/WebCore/rendering/LayoutState.h:116
>      bool m_clipped;
>      bool m_isPaginated;

If we're saving memory, why not make these bitfields?

> Source/WebCore/platform/text/BidiResolver.h:152
> +    bool m_hasHyphen; // Used by BidiRun subclass; packed here to save space.

This might be considered a layering violation, since the whole idea with BidiCharacterRun is to allow paltform-only code to be able to use WebCore's bidi-resolver.  BidiRun, etc. are the DOM-aware subclasses.

[Chris Evans]

@eseidel: Thanks for the comments and sorry for the slow action. I was scared of the efficiency of bitfields but after an hour playing with compilers and asm, I see that my fears were largely unfounded. In fact, clang (as used on Mac AFAIK) generates very compact / fast initialization code for bitfields.
Your point about layering violation is noted. I'll rename the field to reflect it's generic subclass-defined efficient storage and abstract away m_hasHyphen into a simple getter.

@inferno: with the bitfields approach suggested by @eseidel, the "dummy" variable is unnecessary. I'll recalculate the savings and update the ChangeLog with more verbosity as requested.

Updated patch forthcoming.

[Chris Evans]

Created attachment 172675 [details]
Addresses comments

[Eric Seidel (no email)]

Comment on attachment 172675 [details]
Addresses comments

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

Otherwsie LGTM.

> Source/WebCore/platform/text/BidiResolver.h:151
> +    bool m_subClass1:1; // Opaque subclass storage; packed here to save space.

I would just call this m_hasHyphen and add a comment that it's used by a subclass (and that it's a layering violation, but that we do it to save N bytes per object).

[Chris Evans]

Created attachment 172704 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 172704 [details]
Patch for landing

Clearing flags on attachment: 172704

Committed r133713: <http://trac.webkit.org/changeset/133713>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

Your dream was not realized.  There are now members after m_next. :(