Bug 219615 - Make TextRun::subRun stricter
Summary: Make TextRun::subRun stricter
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-12-07 14:11 PST by Rob Buis
Modified: 2021-02-05 21:30 PST (History)
8 users (show)

See Also:


Attachments
Patch (1.43 KB, patch)
2020-12-07 14:14 PST, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (1.40 KB, patch)
2020-12-08 06:39 PST, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rob Buis 2020-12-07 14:11:22 PST
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.
Comment 1 Rob Buis 2020-12-07 14:14:02 PST
Created attachment 415584 [details]
Patch
Comment 2 Darin Adler 2020-12-07 14:52:03 PST
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.
Comment 3 Rob Buis 2020-12-07 21:56:17 PST
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.
Comment 4 Rob Buis 2020-12-08 06:39:05 PST
Created attachment 415636 [details]
Patch
Comment 5 Rob Buis 2020-12-08 06:40:45 PST
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]
Comment 6 Darin Adler 2020-12-08 09:07:53 PST
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.
Comment 7 EWS 2020-12-08 09:22:14 PST
Committed r270541: <https://trac.webkit.org/changeset/270541>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 415636 [details].
Comment 8 Radar WebKit Bug Importer 2020-12-08 09:23:17 PST
<rdar://problem/72095633>
Comment 9 Rob Buis 2020-12-08 09:25:33 PST
@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?
Comment 10 David Kilzer (:ddkilzer) 2021-02-05 09:40:56 PST
(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.
Comment 11 Ryosuke Niwa 2021-02-05 21:30:51 PST
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.