Bug 219615 – Make TextRun::subRun stricter

• Component: Text
• Status: RESOLVED FIXED

[Rob Buis]

Make TextRun::subRun stricter, besides the start offset being less than the run
length, the sub run start offset plus sub run length should not exceed the
overall run length.

[Rob Buis]

Created attachment 415584 [details]
Patch

[Darin Adler]

Comment on attachment 415584 [details]
Patch

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

> Source/WebCore/platform/graphics/TextRun.h:69
> -        ASSERT_WITH_SECURITY_IMPLICATION(startOffset < m_text.length());
> +        RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(startOffset < m_text.length() && (startOffset + length) <= m_text.length());

The old assertion is redundant. We already have those assertions in data8 and data16. So we could consider only the new assertion, and not repeating the old assertion.

Bold to change this to RELEASE_ASSERT, but strange to change this, and not data8, data16, operator[] or other idioms that are not member functions. Not sure what the rationale is.

[Rob Buis]

Comment on attachment 415584 [details]
Patch

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

>> Source/WebCore/platform/graphics/TextRun.h:69
>> +        RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(startOffset < m_text.length() && (startOffset + length) <= m_text.length());
> 
> The old assertion is redundant. We already have those assertions in data8 and data16. So we could consider only the new assertion, and not repeating the old assertion.
> 
> Bold to change this to RELEASE_ASSERT, but strange to change this, and not data8, data16, operator[] or other idioms that are not member functions. Not sure what the rationale is.

I believe there will be a style warning when keeping ASSERT_WITH_SECURITY_IMPLICATION.

[Rob Buis]

Created attachment 415636 [details]
Patch

[Rob Buis]

Comment on attachment 415584 [details]
Patch

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

>>> Source/WebCore/platform/graphics/TextRun.h:69
>>> +        RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(startOffset < m_text.length() && (startOffset + length) <= m_text.length());
>> 
>> The old assertion is redundant. We already have those assertions in data8 and data16. So we could consider only the new assertion, and not repeating the old assertion.
>> 
>> Bold to change this to RELEASE_ASSERT, but strange to change this, and not data8, data16, operator[] or other idioms that are not member functions. Not sure what the rationale is.
> 
> I believe there will be a style warning when keeping ASSERT_WITH_SECURITY_IMPLICATION.

I removed the old assertion and brought back the ASSERT_WITH_SEC. FWIW this is the style error:
ERROR: Source/WebCore/platform/graphics/TextRun.h:69:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]

[Darin Adler]

Comment on attachment 415584 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/TextRun.h:69
>>>> +        RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(startOffset < m_text.length() && (startOffset + length) <= m_text.length());
>>> 
>>> The old assertion is redundant. We already have those assertions in data8 and data16. So we could consider only the new assertion, and not repeating the old assertion.
>>> 
>>> Bold to change this to RELEASE_ASSERT, but strange to change this, and not data8, data16, operator[] or other idioms that are not member functions. Not sure what the rationale is.
>> 
>> I believe there will be a style warning when keeping ASSERT_WITH_SECURITY_IMPLICATION.
> 
> I removed the old assertion and brought back the ASSERT_WITH_SEC. FWIW this is the style error:
> ERROR: Source/WebCore/platform/graphics/TextRun.h:69:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]

I think David Kilzer created that style warning. Might be worth consulting with him.

[EWS]

Committed r270541: <https://trac.webkit.org/changeset/270541>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415636 [details].

[Radar WebKit Bug Importer]

<rdar://problem/72095633>

[Rob Buis]

@ddkilzer: is the style check maybe too strong for this kind of patch? I chose to ignore it:
ERROR: Source/WebCore/platform/graphics/TextRun.h:69:  Please replace ASSERT_WITH_SECURITY_IMPLICATION() with RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]

Is it maybe better to have it as a warning or advice?

[David Kilzer (:ddkilzer)]

(In reply to Rob Buis from comment #9)
> @ddkilzer: is the style check maybe too strong for this kind of patch? I
> chose to ignore it:
> ERROR: Source/WebCore/platform/graphics/TextRun.h:69:  Please replace
> ASSERT_WITH_SECURITY_IMPLICATION() with
> RELEASE_ASSERT_WITH_SECURITY_IMPLICATION().  [security/assertion] [5]
> 
> Is it maybe better to have it as a warning or advice?

We want to decrease use of ASSERT_WITH_SECURITY_IMPLICATION() over time because if the security issue is bad enough to assert in a Debug build, then we are likely hiding the issue in Release builds.

Notice that git/commit-queue doesn't enforce this, so you were still able to land the patch.  The point is that this is to make you think twice about using ASSERT_WITH_SECURITY_IMPLICATION() going forward.

[Ryosuke Niwa]

We probably can't use release assert here since TextRun needs to be lightweight & fast. Might be worth experimenting with trying to use release assert & run all the benchmarks we have to see if it negatively impacts it or not.