This issue is a security regression. If you are not able to fix this quickly, please revert the change that introduced it.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

simonhong@: Can you please have a look at this security regression? Based on the regression range from Cluster-Fuzz, your CL might have caused it: https://chromium.googlesource.com/chromium/src/+/74e44d1e750a7780d268d7713c0e07a6b349e795.

Hi simonhong@ - you still the best person to look at this?  Thanks!

This is a serious security regression. If you are not able to fix this quickly, please revert the change that introduced it.

If this doesn't affect a release branch, or has not been properly classified for severity, please update the Security_Impact or Security_Severity labels, and remove the ReleaseBlock label. To disable this altogether, apply ReleaseBlock-NA.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

URGENT - PTAL.
Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the M61 branch #3163 ASAP to have enough baking time in Beta before Stable promotion. Thank you!

Know that this issue shouldn't block the release?  Remove the ReleaseBlock-Stable label.

Removing RB-S since this isn't a regression, and removing owner to get this back into the Sheriff's queue since it looks like simonhong@ hasn't been active in a while.

tapted, it looks like you've worked in the harfbuzz file in the recent past, perhaps you could use that experience to take a look at this?  Feel free to bounce this back to me if you're not an appropriate owner (or suggest another one).  Thanks !!!

hm - I work on a lot of things :)

msw reviewed the code in https://codereview.chromium.org/351963002 and might have a running-start on this.

it's possible the DCHECK at https://codereview.chromium.org/351963002/diff/210001/ui/gfx/render_text_harfbuzz.cc#newcode417 is failing

I attempted to repro locally, but it failed to crash all three times.
It's "using gestures and inherently flaky", so the crash may still be real.

I guess we can use CHECK to really crash, or just bail if that DCHECK fails.
I'm working on a fix along this vein, but I doubt it's urgent.

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

commit 9be6d6161179ef98cfae06d5bd9201759ed7c6c9
Author: Michael Wasserman <msw@chromium.org>
Date: Thu Aug 10 05:21:02 2017

Bail on invalid TextRunHarfBuzz::GetClusterAt conditions.

It's unclear to me why this crash would occur.
Bail and print an error if expected conditions aren't met.
Add a unit test to ensure we don't crash.

Bug:  724880 
Change-Id: Ic8f72c392f7490c390981aa43208be226ccac387
Reviewed-on: https://chromium-review.googlesource.com/609133
Reviewed-by: Trent Apted <tapted@chromium.org>
Commit-Queue: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493259}
[modify] https://crrev.com/9be6d6161179ef98cfae06d5bd9201759ed7c6c9/ui/gfx/render_text_harfbuzz.cc
[modify] https://crrev.com/9be6d6161179ef98cfae06d5bd9201759ed7c6c9/ui/gfx/render_text_unittest.cc

I'm calling this fixed... My test works with my code changes and crashes without them.
If anyone gets the error logging (beyond from that test), please post it to this bug.

ClusterFuzz has detected this issue as fixed in range 493241:493282.

Detailed report: https://clusterfuzz.com/testcase?key=6666400843431936

Fuzzer: noel-image-flip
Job Type: linux_asan_chrome_v8_arm
Platform Id: linux

Crash Type: Heap-buffer-overflow READ 4
Crash Address: 0xb3d880cc
Crash State:
  gfx::internal::TextRunHarfBuzz::GetClusterAt
  gfx::internal::TextRunHarfBuzz::GetGraphemeBounds
  gfx::RenderTextHarfBuzz::GetSubstringBounds
  
Sanitizer: address (ASAN)

Recommended Security Severity: Medium

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_v8_arm&range=465354:465390
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_chrome_v8_arm&range=493241:493282

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=6666400843431936

Additional requirements: Requires Gestures

See https://github.com/google/clusterfuzz-tools for more information.

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

ClusterFuzz testcase 6666400843431936 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.

The new unit test HarfBuzz_NoCrashOnTextRunGetClusterAt causes the new error message to be logged:

[1891:1891:0823/135759.482699:15756770311:ERROR:render_text_harfbuzz.cc(714)]  TextRunHarfBuzz error, please report at  crbug.com/724880 : range: {0,4}, rtl: 0, level: '', script: -1, font: 'DejaVu Sans', glyph_count: 4, pos: 0, glyph_to_char: 0->1, 1->1, 2->2, 3->3, 

I assume this is intentional? (Since the test is checking that the condition is hit but it doesn't crash?) Still, it's confusing because everyone who runs these tests will see a call to report it here. Perhaps it should say ("unless this is in a test"). Also this bug isn't currently visible to the public which makes it even more confusing if someone sees it and tries to report it.

I'm not sure how important it is to follow up here. The clusterfuzz test case wouldn't repro and we only have 107 matching crash reports, (which now shouldn't crash, but at worst yield some strange text rendering/interaction):
https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.magic_signature_1.name%3D%27gfx%3A%3Ainternal%3A%3ATextRunHarfBuzz%3A%3AGetClusterAt%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D&unnest=#-property-selector,samplereports:5,+productversion

I couldn't find or devise an example string that yields the invalid glyph_to_char map (granted I didn't dig into any crash minidumps... but I'm not sure they'd have any strings anyway, since they'd be on the heap, not the stack, afaik). So that logging is meant to find an example, in case we ever care to dive deeper into the root defect. Sadly, it was inconvenient and of dubious value to actually include the original logical character string in the logging, so I'm not sure how useful a report would be... If you're so inclined, maybe you'd be interested in plumbing the logical string through to the logging and updating the comment to disregard the HarfBuzz_NoCrashOnTextRunGetClusterAt test itself; but, as I said, it's likely not a high-priority task.

Oh yeah, and we can probably make this bug public after M61 has gone to stable.
(ie. wait until these strings wouldn't actually crash for most users)

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot