New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 710753 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 1
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 3
Type: Bug



Sign in to add a comment

Data race in sw::Renderer::draw

Project Member Reported by ClusterFuzz, Apr 12 2017

Issue description

Cc: msrchandra@chromium.org
Components: Internals>GPU>SwiftShader
Labels: Test-Predator-Wrong-CLs M-59
Owner: capn@chromium.org
Status: Assigned (was: Untriaged)
Predator and CL did not find any possible suspects.
Using Code Search for the file, "Renderer.cpp" assigning to concern owner.
Suspecting Commit#
https://swiftshader.googlesource.com/SwiftShader.git/+/0808b4f0e17f57dc892dc586103e9a945a7f51d8

@Nicolas Capens -- Could you please look into the issue, kindly re-assign if this is not related to your changes.
Thank You.

Comment 2 by capn@chromium.org, Apr 12 2017

Labels: -Pri-2 Pri-3
Status: Started (was: Assigned)
Thanks for the report. This is indeed related to my change. We've enabled the use of SwiftShader (OpenGL ES 2.0 implementation on the CPU) as a WebGL fallback on Linux (was already enabled on Windows).

The threading code hasn't really changed in years, and we consistently pass the WebGL and dEQP conformance test suites. So to the best of my knowledge it's actually robust. That said, we implemented our own exponential back-off mutex lock, and it may rely on memory ordering guarantees offered by x86 but not necessarily other CPUs. Is it possible TSAN is detecting a false positive?

I'll have a deeper look at the test case, but overall I'm not too worried since SwiftShader has been in use for several years in other projects.

Comment 3 by capn@chromium.org, Apr 13 2017

Cc: sugoi@chromium.org capn@chromium.org
 Issue 710973  has been merged into this issue.

Comment 4 by capn@chromium.org, Apr 13 2017

 Issue 710822  has been merged into this issue.

Comment 5 by capn@chromium.org, Apr 13 2017

 Issue 710789  has been merged into this issue.

Comment 6 by capn@chromium.org, Apr 13 2017

 Issue 710787  has been merged into this issue.

Comment 7 by capn@chromium.org, Apr 13 2017

As far as I can tell so far there could be a data race iff the code was running on a CPU with weak memory ordering. Specifically, data that was written by a first thread before our lock is acquired by a second thread could still have the old values when read by the second thread.

A store-release of BackoffLock's "mutex" variable should fix the issue on CPUs with weak memory ordering.

Since SwiftShader only supports x86 for now, and it has strong memory ordering, there shouldn't be any actual problem. What confuses me a bit is that TSAN can apparently detect a weak memory ordering issue on an architecture with strong memory ordering?

Ironically the use of <atomic>, necessary to specify store-release semantics, is banned by Chromium: https://chromium-cpp.appspot.com/#blacklist_lib_list. But a quick code search reveals that it is in fact used in several places, so it's probably more of a precaution. Since x86 offers strong memory ordering, std::memory_order_release should have no effect anyway except for silencing TSAN.
Project Member

Comment 8 by ClusterFuzz, Apr 15 2017

Labels: ClusterFuzz-Verified
Status: Verified (was: Started)
ClusterFuzz testcase 5361075651608576 is verified as fixed, so closing issue.

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

Comment 9 by capn@chromium.org, Apr 17 2017

Cc: jbau...@chromium.org
 Issue 712015  has been merged into this issue.

Comment 10 by capn@chromium.org, Apr 17 2017

Labels: -ClusterFuzz-Verified ClusterFuzz-Wrong
Status: Started (was: Verified)

Comment 11 by capn@chromium.org, Apr 18 2017

 Issue 711849  has been merged into this issue.
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 19 2017

The following revision refers to this bug:
  https://swiftshader.googlesource.com/SwiftShader.git/+/dc7759ccc3151a1aefefa9f86610e66f6fe9311c

commit dc7759ccc3151a1aefefa9f86610e66f6fe9311c
Author: Nicolas Capens <capn@google.com>
Date: Wed Apr 19 15:30:28 2017

Fix potential data race in mutex lock implementation.

Writing or reading an ordinary volatile boolean does not provide memory
ordering guarantees. Use atomic<bool> with release and acquire semantics
instead. On x86 this should have no impact on the generated code, since
it architecturally offers strong memory ordering guarantees.

 Bug chromium:710753 

Change-Id: I5aa2a6eeecfcd691a0aa8d06fd067f9d59ab13c3
Reviewed-on: https://swiftshader-review.googlesource.com/9328
Reviewed-by: Corentin Wallez <cwallez@google.com>
Reviewed-by: Nicolas Capens <capn@google.com>
Tested-by: Nicolas Capens <capn@google.com>

[modify] https://crrev.com/dc7759ccc3151a1aefefa9f86610e66f6fe9311c/src/Common/MutexLock.hpp

Project Member

Comment 13 by bugdroid1@chromium.org, Apr 19 2017

The following revision refers to this bug:
  https://swiftshader.googlesource.com/SwiftShader.git/+/dc7759ccc3151a1aefefa9f86610e66f6fe9311c

commit dc7759ccc3151a1aefefa9f86610e66f6fe9311c
Author: Nicolas Capens <capn@google.com>
Date: Wed Apr 19 15:30:28 2017

Fix potential data race in mutex lock implementation.

Writing or reading an ordinary volatile boolean does not provide memory
ordering guarantees. Use atomic<bool> with release and acquire semantics
instead. On x86 this should have no impact on the generated code, since
it architecturally offers strong memory ordering guarantees.

 Bug chromium:710753 

Change-Id: I5aa2a6eeecfcd691a0aa8d06fd067f9d59ab13c3
Reviewed-on: https://swiftshader-review.googlesource.com/9328
Reviewed-by: Corentin Wallez <cwallez@google.com>
Reviewed-by: Nicolas Capens <capn@google.com>
Tested-by: Nicolas Capens <capn@google.com>

[modify] https://crrev.com/dc7759ccc3151a1aefefa9f86610e66f6fe9311c/src/Common/MutexLock.hpp

Project Member

Comment 14 by bugdroid1@chromium.org, Apr 19 2017

The following revision refers to this bug:
  https://swiftshader.googlesource.com/SwiftShader.git/+/dc7759ccc3151a1aefefa9f86610e66f6fe9311c

commit dc7759ccc3151a1aefefa9f86610e66f6fe9311c
Author: Nicolas Capens <capn@google.com>
Date: Wed Apr 19 15:30:28 2017

Fix potential data race in mutex lock implementation.

Writing or reading an ordinary volatile boolean does not provide memory
ordering guarantees. Use atomic<bool> with release and acquire semantics
instead. On x86 this should have no impact on the generated code, since
it architecturally offers strong memory ordering guarantees.

 Bug chromium:710753 

Change-Id: I5aa2a6eeecfcd691a0aa8d06fd067f9d59ab13c3
Reviewed-on: https://swiftshader-review.googlesource.com/9328
Reviewed-by: Corentin Wallez <cwallez@google.com>
Reviewed-by: Nicolas Capens <capn@google.com>
Tested-by: Nicolas Capens <capn@google.com>

[modify] https://crrev.com/dc7759ccc3151a1aefefa9f86610e66f6fe9311c/src/Common/MutexLock.hpp

Project Member

Comment 15 by bugdroid1@chromium.org, Apr 19 2017

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

commit 7c9bd5f0202de1500d75c22654b2e22f0bc34312
Author: capn <capn@chromium.org>
Date: Wed Apr 19 17:28:33 2017

Roll SwiftShader 30385f0..dc7759c

https://swiftshader.googlesource.com/SwiftShader.git/+log/30385f0..dc7759c
- Fixes ThinLTO linking.
- Fixes TSAN data race.

BUG= chromium:686980 ,  chromium:710753 

TBR=kbr@chromium.org

TEST=bots

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel

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

[modify] https://crrev.com/7c9bd5f0202de1500d75c22654b2e22f0bc34312/DEPS

Project Member

Comment 16 by bugdroid1@chromium.org, Apr 25 2017

The following revision refers to this bug:
  https://swiftshader.googlesource.com/SwiftShader.git/+/35e0ba7753cab88905b428b65577669620798507

commit 35e0ba7753cab88905b428b65577669620798507
Author: Nicolas Capens <capn@google.com>
Date: Tue Apr 25 17:04:23 2017

Use pthread mutexes on all Linux platforms.

 Bug chromium:710753 

Change-Id: Ie74d86de027417579c9c6949aabad1c3ae89ebfc
Reviewed-on: https://swiftshader-review.googlesource.com/9329
Reviewed-by: Corentin Wallez <cwallez@google.com>
Reviewed-by: Nicolas Capens <capn@google.com>
Tested-by: Nicolas Capens <capn@google.com>

[modify] https://crrev.com/35e0ba7753cab88905b428b65577669620798507/src/Common/MutexLock.hpp

Project Member

Comment 17 by bugdroid1@chromium.org, May 18 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chrome/tools/buildspec/+/4c54802b8c32deb32093688a836a8c17cdfe8bc6

commit 4c54802b8c32deb32093688a836a8c17cdfe8bc6
Author: Nicolas Capens <capn@google.com>
Date: Thu May 18 15:31:13 2017

Project Member

Comment 18 by ClusterFuzz, Jun 14 2017

ClusterFuzz has detected this issue as fixed in range 479057:479065.

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

Fuzzer: inferno_layout_test_unmodified
Job Type: linux_tsan_chrome_mp
Platform Id: linux

Crash Type: Data race READ 4
Crash Address: 0x7bb00000c70c
Crash State:
  sw::Renderer::draw
  es2::Device::drawPrimitive
  es2::Context::drawArrays
  
Sanitizer: thread (TSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_tsan_chrome_mp&range=463855:463874
Fixed: https://clusterfuzz.com/revisions?job=linux_tsan_chrome_mp&range=479057:479065

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


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.
Labels: -ClusterFuzz-Wrong
We have made a bunch of changes on ClusterFuzz side, so resetting ClusterFuzz-Wrong label.
Status: Fixed (was: Started)

Sign in to add a comment