Data race in blink::Document::baseURLForOverride |
|||||||
Issue descriptionDetailed report: https://cluster-fuzz.appspot.com/testcase?key=5457617800658944 Fuzzer: inferno_twister_custom_bundle Job Type: linux_tsan_chrome_mp Platform Id: linux Crash Type: Data race READ 4 Crash Address: 0x7b080000f8a8 Crash State: blink::Document::baseURLForOverride blink::Document::completeURL blink::Document::virtualCompleteURL Sanitizer: thread (TSAN) Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_tsan_chrome_mp&range=441524:441984 Reproducer Testcase: https://cluster-fuzz.appspot.com/download/AMIfv964bGp2r-j9sA65SFok_fKq5lyp3F2eh3-KCIoWGNorOxWfnjhgrDeqiHcAW56Bhfb-xf4nn9QFa9GqeFQu5r9DUr1QyHlLI70D8W_sHPnhokT8kovApEFEDrrKwqvPiiLwsqepXtr7Higy-mG92qqbbqsGkbUTbaAvN-5wahsOGGLya1p3qhHqcXbnUqzlmr2dh94_6qJBajBWoBCMkHRuIFXKoOds3dYGro6UjzeI3CHnuqgmjCYp7-Bm3y1ccSwWYiGj31gE4pIzlHgIZF35C4VV-vt8ZwcSLZifnu6LU7Lso_77GrNlP5EB3yIWHbf_32cgj0bnLAsASJSm2d09U3VU_7LCgPiQXitK6ZR-IKM8mp7atbzrJIH5YJZF02btItwXoBV5DrIGPlHm6wMxSJg6jw?testcase_id=5457617800658944 Issue filed automatically. See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
,
Feb 8 2017
Hm this seems like a real race, but the regression range doesn't seem to make too much sense. My guess is that this is fallout from a combination of esprehns change [1] and the change listed in #1. Basically, my guess is we avoided the race before because we calculated updateContainsOnlyASCII() before the worker thread got to do it, via some code path that used to call equal() on the main thread that now calls stringViewEqual(). Probably the safest thing is to make sure these static local Strings have all their optimizations applied right away. [1] https://codereview.chromium.org/2614123002/
,
Feb 8 2017
Is the race safe since we're computing what is effectively an idempotent value? I don't know if this is undefined behavior because of the bitfields and how compilers do that stuff. I think we compute the hash of static strings up front though, so we should probably also compute the isascii bit... Actually can we just assert all static strings are ASCII and force the bit true?
,
Feb 8 2017
I think technically per C++ semantics an "idempotent" race is still a race, but I have less experience with how much it matters in practice. I like the idea to assert all static strings are ASCII and force the bit, but this one will be a bit uglier because it's just a static KURL and doesn't actually point to an underlying static string. Additionally, KURLs themselves are not immutable like StringImpl, so merely having blankURL accessed on multiple threads is buggy. One solution would be to make a version of "static" KURLs that does something fancy, but it seems over-engineered. I would prefer to keep KURLs thread unsafe and do something like thread safe static local, or remove accesses from other threads. One more thing is that there were recently some bugs in Chrome that failed isAboutBlank checks when the URL looks like "about:blank?somequery". GURL was updated to be robust to these cases and maybe we should make KURL too.
,
Mar 10 2017
ClusterFuzz testcase 5457617800658944 is flaky and no longer reproduces, so closing issue. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
,
Mar 10 2017
I don't think clusterfuzz is wrong per-se, but we should still probably fix this.
,
Jul 10 2017
I don't have bandwidth for this ATM. I think we could probably have a version of static, const KURLs that are immutable. I had a patch up here that I didn't finish: https://codereview.chromium.org/2680933005/
,
Jul 10
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 11
Unable to find actual suspect through code search and also observing no CL's under regression range, hence adding appropriate label and requesting someone from dev team to look in to this issue. Thanks! |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by msrchandra@chromium.org
, Feb 6 2017Labels: Test-Predator-Correct-CLs M-58
Owner: csharrison@chromium.org
Status: Assigned (was: Untriaged)