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

Issue 601578 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

Data race in SkROBuffer::Iter::next

Project Member Reported by ClusterFuzz, Apr 7 2016

Issue description

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

Fuzzer: attekett_dom_fuzzer
Job Type: linux_tsan_chrome_mp
Platform Id: linux

Crash Type: Data race READ 8
Crash Address: 0x7d8000088000
Crash State:
  SkROBuffer::Iter::next
  blink::ROBufferSegmentReader::getSomeData
  blink::fill_input_buffer
  

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv96Ot7zP2a3zVaPkxiUWtulhfwLGdWX07zGC9lzb5rFUdOOq_5zK9OLF0-ar0eXe3FmE3eS0szAUgOoyi6HvQp53WSjRXS6b-yGYa7dIo9uy-mbmneOscBFAVVoV9xITYzZGTaAJq4eRn7ws0Y2NWf8UrW6vtQpwVSArXFhWRlc6Pe7IK5M


Filer: manoranjanr

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Cc: reed@chromium.org bsalomon@chromium.org
Labels: Te-Logged
Unable to find the exact culprit, hence looping owners of https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/OWNERS.
Project Member

Comment 2 by ClusterFuzz, Apr 7 2016

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

Fuzzer: attekett_surku_fuzzer
Job Type: linux_tsan_chrome_mp
Platform Id: linux

Crash Type: Data race WRITE 8
Crash Address: 0x7d8000058010
Crash State:
  SkRWBuffer::append
  blink::DeferredImageDecoder::setData
  blink::ImageSource::setData
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_tsan_chrome_mp&range=385466:385470

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv94I-yNT6ACQeBfVtrOhpK2IYMXXKgTZZIJfxLybXP7gVm5z_lIsRWcOzmzt5DzZdlh7Q4JXU0P15O8fex0rWNduSuCsUuYFA1wVHYyn3UnQg20ihyU4rUe72q-hBKOG07D_f8N23SskbhjwoDPHzZ7LrEYR1wsyAKf9S_zcJAfKtDTtL74


Filer: manoranjanr

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

Comment 3 by reed@google.com, Apr 7 2016

Cc: -bsalomon@chromium.org mtkl...@chormium.org herb@chromium.org bunge...@chromium.org reed@google.com
Owner: scroggo@chromium.org
Status: Started (was: Available)
Possibly related to issue 601416, for which I am already reverting the CL that uses SkROBuffer::Iter::next (https://codereview.chromium.org/1812273003)
Status: Fixed (was: Started)
That CL has been reverted, so marking as fixed. I'll address this SkROBuffer before relanding.
Project Member

Comment 6 by ClusterFuzz, Apr 8 2016

ClusterFuzz has detected this issue as fixed in range 385848:385940.

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

Fuzzer: attekett_dom_fuzzer
Job Type: linux_tsan_chrome_mp
Platform Id: linux

Crash Type: Data race READ 8
Crash Address: 0x7d8000088000
Crash State:
  SkROBuffer::Iter::next
  blink::ROBufferSegmentReader::getSomeData
  blink::fill_input_buffer
  
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_tsan_chrome_mp&range=385848:385940

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv96Ot7zP2a3zVaPkxiUWtulhfwLGdWX07zGC9lzb5rFUdOOq_5zK9OLF0-ar0eXe3FmE3eS0szAUgOoyi6HvQp53WSjRXS6b-yGYa7dIo9uy-mbmneOscBFAVVoV9xITYzZGTaAJq4eRn7ws0Y2NWf8UrW6vtQpwVSArXFhWRlc6Pe7IK5M


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.
Project Member

Comment 8 by ClusterFuzz, Apr 9 2016

ClusterFuzz has detected this issue as fixed in range 385848:385940.

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

Fuzzer: attekett_surku_fuzzer
Job Type: linux_tsan_chrome_mp
Platform Id: linux

Crash Type: Data race WRITE 8
Crash Address: 0x7d8000058010
Crash State:
  SkRWBuffer::append
  blink::DeferredImageDecoder::setData
  blink::ImageSource::setData
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_tsan_chrome_mp&range=385466:385470
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_tsan_chrome_mp&range=385848:385940

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv94I-yNT6ACQeBfVtrOhpK2IYMXXKgTZZIJfxLybXP7gVm5z_lIsRWcOzmzt5DzZdlh7Q4JXU0P15O8fex0rWNduSuCsUuYFA1wVHYyn3UnQg20ihyU4rUe72q-hBKOG07D_f8N23SskbhjwoDPHzZ7LrEYR1wsyAKf9S_zcJAfKtDTtL74


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.
Project Member

Comment 9 by bugdroid1@chromium.org, Apr 10 2016

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

commit ae82343d87b4b2b9f2048f7f82c901e23c0304a6
Author: skia-deps-roller <skia-deps-roller@chromium.org>
Date: Sun Apr 10 22:31:43 2016

Roll src/third_party/skia/ e7365065a..82b043e87 (29 commits).

https://chromium.googlesource.com/skia.git/+log/e7365065acfe..82b043e87380

$ git log e7365065a..82b043e87 --date=short --no-merges --format='%ad %ae %s'
2016-04-10 robertphillips Fix roll bot's win gn build
2016-04-10 mtklein arithmetic mode with Sk4f
2016-04-10 update-skps Update SKP version
2016-04-09 reed remove unused BML (binary xml) code
2016-04-08 robertphillips Update LightingImageFilter to sk_sp
2016-04-08 Melnikov.Sergey.V Add AVX/AVX2 support for runtime check
2016-04-08 robertphillips Fix memory leak in SkBlurImageFilter
2016-04-08 senorblanco Add some benches for SkArithmeticMode.
2016-04-08 herb Fix context size for benchmakr.
2016-04-08 robertphillips Add SkFUZZF to help whitelist imagefilter fuzz failures
2016-04-08 robertphillips Fix scaledstrokes GM
2016-04-08 robertphillips Make some GMs animate GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1870133003
2016-04-08 egdaniel Don't create new descriptor set for vulkan uniforms every draw
2016-04-08 herb Add clone to Stage. Rename place to mix and PolymorphicUnion to Stage. Cleanup.
2016-04-08 jvanverth More cleanup in the Vulkan viewer
2016-04-08 reed Fix race condition in SkROBuffer.
2016-04-08 mtklein Serialize invNormRotation in SkLightingShader.
2016-04-08 robertphillips Upgrade SkSpecialImage to have getTextureRef & getROPixels entry points
2016-04-08 halcanary SkPDF: Fix links to be more valid PDF/A
2016-04-08 halcanary SkPDF: fix pessimizing-move in PDFA code
2016-04-08 bungeman Increase request cache size.
2016-04-08 robertphillips Update MatrixConvolutionImageFilter to sk_sp
2016-04-08 jvanverth Add slides to VulkanViewer
2016-04-08 bungeman Test CTFonts for equality, not just name and style.
2016-04-08 bungeman Convert SkRefCnt to std::atomic.
2016-04-08 herb Add specialized rgba8888 unit sampler.
2016-04-08 brianosman Decouple contrast boost from fake gamma.
2016-04-08 robertphillips More valgrind appeasement
2016-04-07 hcm Update Skia milestone to 52

BUG= 601578 ,424082

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=borenet@google.com

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

Cr-Commit-Position: refs/heads/master@{#386319}

[modify] https://crrev.com/ae82343d87b4b2b9f2048f7f82c901e23c0304a6/DEPS

Project Member

Comment 10 by bugdroid1@chromium.org, Apr 14 2016

The following revision refers to this bug:
  https://skia.googlesource.com/skia.git/+/d06920a29fe11c68bde2b93948ec99f277bb8459

commit d06920a29fe11c68bde2b93948ec99f277bb8459
Author: scroggo <scroggo@google.com>
Date: Thu Apr 14 18:40:48 2016

Fixes for SkRWBuffer

Do not call SkBufferHead::validate in SkROBuffer's destructor, which
may be called in a separate thread from SkRWBuffer::append. validate()
reads SkBufferBlock::fUsed, and append() writes to it, resulting in
a data race.

Update some comments to be more clear about how it is safe to use
these classes across threads.

Test the readers in separate threads.

In addition, make sure it is safe to create a reader even when no
data has been appended. Add tests for this case.

Mark a parameter to SkBufferHead::validate() as const, reflecting
its use.

BUG= chromium:601578 
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1871953002

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

[modify] https://crrev.com/d06920a29fe11c68bde2b93948ec99f277bb8459/include/core/SkRWBuffer.h
[modify] https://crrev.com/d06920a29fe11c68bde2b93948ec99f277bb8459/src/core/SkRWBuffer.cpp
[modify] https://crrev.com/d06920a29fe11c68bde2b93948ec99f277bb8459/tests/DataRefTest.cpp

Project Member

Comment 11 by bugdroid1@chromium.org, Apr 14 2016

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

commit f4f2676ab1712ea5ba3a7c38b59bbffa08f096cf
Author: skia-deps-roller <skia-deps-roller@chromium.org>
Date: Thu Apr 14 20:22:07 2016

Roll src/third_party/skia/ bc9517375..d06920a29 (2 commits).

https://chromium.googlesource.com/skia.git/+log/bc9517375685..d06920a29fe1

$ git log bc9517375..d06920a29 --date=short --no-merges --format='%ad %ae %s'
2016-04-14 scroggo Fixes for SkRWBuffer
2016-04-14 herb Add F16 source to the linear pipelin.

BUG= 601578 

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=borenet@google.com

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

Cr-Commit-Position: refs/heads/master@{#387405}

[modify] https://crrev.com/f4f2676ab1712ea5ba3a7c38b59bbffa08f096cf/DEPS

Project Member

Comment 12 by bugdroid1@chromium.org, Apr 14 2016

The following revision refers to this bug:
  https://skia.googlesource.com/skia.git/+/a3760992c93ddb5512d96671831576907441605d

commit a3760992c93ddb5512d96671831576907441605d
Author: bungeman <bungeman@google.com>
Date: Thu Apr 14 21:57:01 2016

Revert of Fixes for SkRWBuffer (patchset #5 id:80001 of https://codereview.chromium.org/1871953002/ )

Reason for revert:
Making MSAN and TSAN rather unhappy.

https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-MSAN/builds/1586

https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-TSAN/builds/5922

Original issue's description:
> Fixes for SkRWBuffer
>
> Do not call SkBufferHead::validate in SkROBuffer's destructor, which
> may be called in a separate thread from SkRWBuffer::append. validate()
> reads SkBufferBlock::fUsed, and append() writes to it, resulting in
> a data race.
>
> Update some comments to be more clear about how it is safe to use
> these classes across threads.
>
> Test the readers in separate threads.
>
> In addition, make sure it is safe to create a reader even when no
> data has been appended. Add tests for this case.
>
> Mark a parameter to SkBufferHead::validate() as const, reflecting
> its use.
>
> BUG= chromium:601578 
> GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1871953002
>
> Committed: https://skia.googlesource.com/skia/+/d06920a29fe11c68bde2b93948ec99f277bb8459

TBR=mtklein@google.com,reed@google.com,scroggo@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= chromium:601578 

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

[modify] https://crrev.com/a3760992c93ddb5512d96671831576907441605d/include/core/SkRWBuffer.h
[modify] https://crrev.com/a3760992c93ddb5512d96671831576907441605d/src/core/SkRWBuffer.cpp
[modify] https://crrev.com/a3760992c93ddb5512d96671831576907441605d/tests/DataRefTest.cpp

Project Member

Comment 13 by bugdroid1@chromium.org, Apr 15 2016

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

commit 2cd561ea2fde1473b2830ce45773defa21365259
Author: skia-deps-roller <skia-deps-roller@chromium.org>
Date: Fri Apr 15 01:04:45 2016

Roll src/third_party/skia/ d06920a29..834d9e109 (7 commits).

https://chromium.googlesource.com/skia.git/+log/d06920a29fe1..834d9e109298

$ git log d06920a29..834d9e109 --date=short --no-merges --format='%ad %ae %s'
2016-04-14 mtklein Revert of skcpu: sse4.1 floor, f16c f16<->f32 (patchset #10 id:180001 of https://codereview.chromium.org/1891513002/ )
2016-04-14 bungeman Revert of Fixes for SkRWBuffer (patchset #5 id:80001 of https://codereview.chromium.org/1871953002/ )
2016-04-14 mtklein Graduate matrix map-point procs out of SkOpts.
2016-04-14 herb Make sure the color profile propagetes through the system.
2016-04-14 reed add index in getPixels for F16
2016-04-14 brianosman Several fixes for fp 16 rendering:
2016-04-14 mtklein skcpu: sse4.1 floor, f16c f16<->f32

BUG= 601578 

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=borenet@google.com

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

Cr-Commit-Position: refs/heads/master@{#387503}

[modify] https://crrev.com/2cd561ea2fde1473b2830ce45773defa21365259/DEPS

Seems like we need a merge to M50. CF is still complaining on M50 (https://cluster-fuzz.appspot.com/testcase?key=5952243483803648).

Thank you!
Status: Started (was: Fixed)
https://codereview.chromium.org/1871953002/ got reverted, and I am relanding it now. Then I can merge to m50
Project Member

Comment 16 by bugdroid1@chromium.org, Apr 22 2016

The following revision refers to this bug:
  https://skia.googlesource.com/skia.git/+/635164028594b4af0086ec85b5e4570dd11091da

commit 635164028594b4af0086ec85b5e4570dd11091da
Author: scroggo <scroggo@google.com>
Date: Fri Apr 22 13:59:01 2016

Fixes for SkRWBuffer

Do not call SkBufferHead::validate in SkROBuffer's destructor, which
may be called in a separate thread from SkRWBuffer::append. validate()
reads SkBufferBlock::fUsed, and append() writes to it, resulting in
a data race.

Update some comments to be more clear about how it is safe to use
these classes across threads.

Test the readers in separate threads.

In addition, make sure it is safe to create a reader even when no
data has been appended. Add tests for this case.

Mark a parameter to SkBufferHead::validate() as const, reflecting
its use.

BUG= chromium:601578 
BUG= chromium:605479 

GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1871953002

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

[modify] https://crrev.com/635164028594b4af0086ec85b5e4570dd11091da/include/core/SkRWBuffer.h
[modify] https://crrev.com/635164028594b4af0086ec85b5e4570dd11091da/src/core/SkRWBuffer.cpp
[modify] https://crrev.com/635164028594b4af0086ec85b5e4570dd11091da/tests/DataRefTest.cpp

I'm not sure this is needed for m50. This data race is in SkRWBuffer, which was first used by Chromium in revision 387344 (https://codereview.chromium.org/1812273003). AFAICT, m50 branched at revision 378081 (i.e. before then). (I haven't figured out how to look at the m50 branch to confirm, but I'm basing this on [1].) Once Skia gets rolled into Chromium I'll remark this as fixed. Please let me know if I'm mistaken and it needs to be merged somewhere.

[1] https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/Branched$3A$20M50$20branch$20is$20here%7Csort:relevance/chromium-dev/ApxePE3kNsg/4JCahcujBgAJ
Project Member

Comment 18 by bugdroid1@chromium.org, Apr 22 2016

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

commit b4fca84246bfff5e37b927ebf9a7e245850ea3b5
Author: skia-deps-roller <skia-deps-roller@chromium.org>
Date: Fri Apr 22 15:39:22 2016

Roll src/third_party/skia/ 488165e68..635164028 (1 commit).

https://chromium.googlesource.com/skia.git/+log/488165e689ba..635164028594

$ git log 488165e68..635164028 --date=short --no-merges --format='%ad %ae %s'
2016-04-22 scroggo Fixes for SkRWBuffer

BUG= 601578 

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
TBR=fmalita@google.com

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

Cr-Commit-Position: refs/heads/master@{#389115}

[modify] https://crrev.com/b4fca84246bfff5e37b927ebf9a7e245850ea3b5/DEPS

Status: Fixed (was: Started)

Comment 20 by rmis...@google.com, Jun 13 2016

Cc: -mtkl...@chormium.org mtklein@chromium.org
Project Member

Comment 21 by sheriffbot@chromium.org, Nov 22 2016

Labels: -Restrict-View-EditIssue
Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

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

Sign in to add a comment