danakj@, git blame shows your commits for some of stack frames, could you please take a look or suggest another owner?

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

void GLES2Implementation::WaitSyncTokenCHROMIUM(const GLbyte* sync_token) {
  if (sync_token) {
    // Copy the data over before data access to ensure alignment.                                                              
    SyncToken sync_token_data;
    memcpy(&sync_token_data, sync_token, sizeof(SyncToken));
    if (sync_token_data.HasData()) {    <------------ This is where it claims to have uninitialized data.
      if (!sync_token_data.verified_flush() &&
          !gpu_control_->CanWaitUnverifiedSyncToken(&sync_token_data)) {
        SetGLError(GL_INVALID_VALUE, "glWaitSyncTokenCHROMIUM",
                   "Cannot wait on sync_token which has not been verified");
        return;
      }

The caller is from DrawingBuffer.cpp:573

    gl->Flush();
    GLbyte syncToken[24];
    gl->GenSyncTokenCHROMIUM(fenceSync, syncToken);
    m_gl->WaitSyncTokenCHROMIUM(syncToken);

That GLbyte array is memcpy'd to sync_token_data, which is said to be uninitialized.

https://cs.chromium.org/chromium/src/gpu/command_buffer/common/sync_token.h?rcl=0&l=100

SyncToken is a complex struct. It's not totally clear a 24-byte-array is correct..

Why are we using byte arrays instead of SyncToken structs in the API? Is it just cuz of WebGraphicsContext3D history?

Well fwiw this compiled ok:

    GLbyte syncToken[24];
    static_assert(sizeof(syncToken) == sizeof(gpu::SyncToken), "bad array size for SyncToken");
    gl->GenSyncTokenCHROMIUM(fenceSync, syncToken);
    m_gl->WaitSyncTokenCHROMIUM(syncToken);

So the array is the right size.

GenSyncTokenCHROMIUM does:

  SyncToken sync_token_data(gpu_control_->GetNamespaceID(),
                            gpu_control_->GetExtraCommandBufferData(),
                            gpu_control_->GetCommandBufferID(), fence_sync);
  sync_token_data.SetVerifyFlush();
  memcpy(sync_token, &sync_token_data, sizeof(sync_token_data));

So we have a memcpy into the array, and a memcpy out. I guess in order to be uninitialized, SyncToken struct would need to leave a field uninitialized. But the constructor initializes everything: https://cs.chromium.org/chromium/src/gpu/command_buffer/common/sync_token.cc?rcl=1468826599&l=19

So.. idk..

>> But the constructor initializes everything
Nope. 
The CTOR w/o parameters does not initialize command_buffer_id_

Oh, GenSyncTokenCHROMIUM has a bunch of early outs, but returns void.

Should it be initializing the array regardless?

https://cs.chromium.org/chromium/src/gpu/command_buffer/client/gles2_implementation.cc?rcl=0&l=5812

> The CTOR w/o parameters does not initialize command_buffer_id_

That's true but that type has a default constructor: https://cs.chromium.org/chromium/src/gpu/command_buffer/common/id_type.h?rcl=1468826599&l=51

=> piman i need some feedback on GenSyncTokenCHROMIUM behaviour. I can write a patch for it if u want.

Also, mmoroz, do we have this report with msan origins enabled? 

We use byte arrays in GenSyncTokenCHROMIUM like all GL APIs, because it's a C API at the root.

re: early outs, GL commands are supposed to leave the input untouched if errors are returned. This was made consistent.

So... is the question why are we getting into an error path?

> GL commands are supposed to leave the input untouched if errors are returned.

Oh ok. Then I guess we need to understand why we're causing an error now.

1. sync_token can't be null since it's the array on the stack.
2. IsFenceSyncRelease() is false?

IsFenceSyncRelease() checks that |release| is < |next_fence_sync_release_|. |next_fence_sync_release_| is incremented by GenerateFenceSyncRelease() which returns the previous value of it, which becomes the |release| right after.

So I don't think that can fail.

3. IsFenceSyncFlushReceived() returns false if the context is lost. We can't rely on it not being lost here right? So we need to handle that case.

https://cs.chromium.org/chromium/src/gpu/ipc/client/command_buffer_proxy_impl.cc?rcl=1468855046&l=587

There's other reasons it can return false too, but maybe they would be logic errors.

What should we do with lost context here then?

This did start happening I guess because of https://chromium.googlesource.com/chromium/src/+/90fcc5eaa0a57e02c8be5f97eff8c1f29cad8a76 which changes the timing that we see lost context here I guess for this given fuzz.

Can we initialize syncToken[] to 0's before passing it to GenSyncTokenCHROMIUM ?
This is what we usually do with other GL calls that need to return values.

ok!

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

commit cf09bfc276420cce16b406380b6eeefb35ec8cbd
Author: danakj <danakj@chromium.org>
Date: Tue Jul 19 00:40:27 2016

webgl: Zero-initialize the SyncToken before calling GenSyncTokenCHROMIUM

The GenSyncTokenCHROMIUM function may fail if it generates an error, or
the context already has an error on it. At that point the sync token
will not be updated, so we should have it initialized to 0 for this
case.

R=kbr@chromium.org
BUG= 629008 

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

[modify] https://crrev.com/cf09bfc276420cce16b406380b6eeefb35ec8cbd/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp

ClusterFuzz has detected this issue as fixed in range 406033:406232.

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

Fuzzer: inferno_twister_custom_bundle
Job Type: linux_msan_chrome
Platform Id: linux

Crash Type: Use-of-uninitialized-value
Crash Address: 
Crash State:
  gpu::gles2::GLES2Implementation::WaitSyncTokenCHROMIUM
  blink::DrawingBuffer::copyToPlatformTexture
  blink::ImageBuffer::copyRenderingResultsFromDrawingBuffer
  
Recommended Security Severity: Low

Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_msan_chrome&range=390527:390610
Fixed: https://cluster-fuzz.appspot.com/revisions?job=linux_msan_chrome&range=406033:406232

Minimized Testcase (36.51 Kb): https://cluster-fuzz.appspot.com/download/AMIfv953Im11eTHep2hW-nkmGhe38dIs-Pvt_DqcovszsaEppEWW1FWs_Feji3DbjeNC_Tf6_uFcu1fm4wXwv2t278cTpKCOBzSUbeSw9AcvEHKqcqYjHqwZJO1mIICYZCbs8_CTGxTJz7msW4TSjOQI3K9UDiHjFbH3CUSWicBR31M5WJlg7YU?testcase_id=6037803205132288

Additional requirements: Requires Gestures

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.

Your change meets the bar and is auto-approved for M53 (branch: 2785)

Please try to merge you change to M53 branch 2785 ASAP latest by 5:00 PM PDT today (sooner the better to avoid compile failure and merge conflicts) so we can take it for tomorrow's M53 beta promotion. Thank you.

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

commit 206672866b790d414a38714e944873ca709b79ef
Author: danakj <danakj@chromium.org>
Date: Wed Jul 27 21:35:56 2016

webgl: Zero-initialize the SyncToken before calling GenSyncTokenCHROMIUM

The GenSyncTokenCHROMIUM function may fail if it generates an error, or
the context already has an error on it. At that point the sync token
will not be updated, so we should have it initialized to 0 for this
case.

R=kbr@chromium.org
BUG= 629008 

Review-Url: https://codereview.chromium.org/2162673002
Cr-Commit-Position: refs/heads/master@{#406162}
(cherry picked from commit cf09bfc276420cce16b406380b6eeefb35ec8cbd)

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

Cr-Commit-Position: refs/branch-heads/2785@{#377}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/206672866b790d414a38714e944873ca709b79ef/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp

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