We're inconsistently using TextRun::length() and TextRun::charactersLength() to check the length of a TextRun - the assert triggered uses the former while code doing the indexing is using the latter to check if it was in bounds. This however  means that there's no out-of-bounds read here, so I don't see any real security impact here.

Glad to hear that we cannot trigger actual out-of-bounds access.
mbarbella@, should we remove Security flag in that case?

Just to make sure I understand this properly, the assert should be checking m_charactersLength, and that wouldn't fail? If so, then I agree that this is not a security bug.

If the ASSERT checked m_charactersLength it would not fail, no. And to add additional context, m_charactersLength is the length of the underlying buffer (m_data.characters{8,16}).

Thanks for the clarification. Removing the security labels in that case.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7dabd875312333379d052e609cc9aad621de1754

commit 7dabd875312333379d052e609cc9aad621de1754
Author: fs <fs@opera.com>
Date: Wed May 11 08:46:00 2016

Use the right TextRun length when checking for surrogate pairs

The isValidSurrogatePair helper in LayoutSVGInlineText.cpp operates on a
TextRun and an index, and checks if the character at the index is part
of a valid surrogate pair.
To check the trailing character, the next index is checked against the
length of the TextRun (to see if the character exists).
The TextRun used is a "sub run" of the entire text node, which means
that the operator[] implementation expects accesses to be within the sub
run rather than the "full run".
Since this function is always used for runs that are sub runs, it should
use TextRun::length() rather than TextRun::charactersLength() to stay
consistent with the iteration and the code using it.

BUG= 610641 

Review-Url: https://codereview.chromium.org/1961953004
Cr-Commit-Position: refs/heads/master@{#392880}

[add] https://crrev.com/7dabd875312333379d052e609cc9aad621de1754/third_party/WebKit/LayoutTests/svg/text/unpaired-surrogate-with-trailing-char-crash-expected.txt
[add] https://crrev.com/7dabd875312333379d052e609cc9aad621de1754/third_party/WebKit/LayoutTests/svg/text/unpaired-surrogate-with-trailing-char-crash.html
[modify] https://crrev.com/7dabd875312333379d052e609cc9aad621de1754/third_party/WebKit/Source/core/layout/svg/LayoutSVGInlineText.cpp

ClusterFuzz has detected this issue as fixed in range 392865:392881.

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5567839548735488

Fuzzer: miaubiz_svg_fuzzer
Job Type: linux_asan_chrome_mp
Platform Id: linux

Crash Type: ASSERT
Crash Address: 
Crash State:
  ASSERTION FAILED: i < m_len
  blink::LayoutSVGInlineText::addMetricsFromRun
  blink::LayoutSVGInlineText::updateMetricsList
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=387276:387299
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=392865:392881

Minimized Testcase (1.52 Kb): https://cluster-fuzz.appspot.com/download/AMIfv95LjMFkh2WWQ8J4I6mElC461dOmDVcWloT3TFyAuoWv74e9JIJZkZsK0vLvTDIsL4HW2qDLWPxUWLQluvzM-pqAgWWPslPTrnQeQ6v1C7KI7B66IEpGcUYC0jKcIugasFSoT-Z-tws_vokaz9IFnhsecNJ5OA

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.

[Automated comment] Less than 2 weeks to go before stable on M51, manual review required.

Reason for merge request:  issue 613915 

Before we approve merge to M51, Could you please confirm whether this change is baked/verified in Canary and safe to merge?

It landed ~12 days ago, and ClusterFuzz verified it (see comment #10.) 

Approving merge to M51 branch 2704 based on comment #15. Please merge ASAP if possible before 5:00 PM PST today or latest by tomorrow noon, 12:00 PM PST.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/80b28a30853ef60bf7c8c4c2c2b47e08569252e3

commit 80b28a30853ef60bf7c8c4c2c2b47e08569252e3
Author: Fredrik Söderquist <fs@opera.com>
Date: Tue May 24 10:35:08 2016

Use the right TextRun length when checking for surrogate pairs

The isValidSurrogatePair helper in LayoutSVGInlineText.cpp operates on a
TextRun and an index, and checks if the character at the index is part
of a valid surrogate pair.
To check the trailing character, the next index is checked against the
length of the TextRun (to see if the character exists).
The TextRun used is a "sub run" of the entire text node, which means
that the operator[] implementation expects accesses to be within the sub
run rather than the "full run".
Since this function is always used for runs that are sub runs, it should
use TextRun::length() rather than TextRun::charactersLength() to stay
consistent with the iteration and the code using it.

BUG= 610641 

Review-Url: https://codereview.chromium.org/1961953004
Cr-Commit-Position: refs/heads/master@{#392880}
(cherry picked from commit 7dabd875312333379d052e609cc9aad621de1754)

Review URL: https://codereview.chromium.org/2005023005 .

Cr-Commit-Position: refs/branch-heads/2704@{#650}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[add] https://crrev.com/80b28a30853ef60bf7c8c4c2c2b47e08569252e3/third_party/WebKit/LayoutTests/svg/text/unpaired-surrogate-with-trailing-char-crash-expected.txt
[add] https://crrev.com/80b28a30853ef60bf7c8c4c2c2b47e08569252e3/third_party/WebKit/LayoutTests/svg/text/unpaired-surrogate-with-trailing-char-crash.html
[modify] https://crrev.com/80b28a30853ef60bf7c8c4c2c2b47e08569252e3/third_party/WebKit/Source/core/layout/svg/SVGTextMetricsBuilder.cpp