Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2251693005:
services/ui: Implement MojoGpuMemoryBufferManager::CreateGpuMemoryBufferFromHandle (Closed)
|
Created:
4 years, 4 months ago by Peng Modified:
4 years, 4 months ago CC:
chromium-reviews, rjkroege, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, Aaron Boodman, darin (slow to review), ben+mojo_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionservices/ui: Implement MojoGpuMemoryBufferManager::CreateGpuMemoryBufferFromHandle
BUG=None
Committed: https://crrev.com/fc56b0062cb12b75c7465710e96fe51f86355913
Cr-Commit-Position: refs/heads/master@{#412965}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix review issue. #
Total comments: 4
Messages
Total messages: 31 (14 generated)
penghuang@chromium.org changed reviewers: + piman@chromium.org
Hi Antoine, PTAL. Thanks.
The CQ bit was checked by penghuang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by penghuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
penghuang@chromium.org changed reviewers: + dcheng@chromium.org
Hi Daniel, PTAL. Thanks.
https://codereview.chromium.org/2251693005/diff/1/services/ui/public/cpp/mojo... File services/ui/public/cpp/mojo_gpu_memory_buffer.cc (right): https://codereview.chromium.org/2251693005/diff/1/services/ui/public/cpp/mojo... services/ui/public/cpp/mojo_gpu_memory_buffer.cc:68: return base::WrapUnique<gfx::GpuMemoryBuffer>( Nit: Omit the template parameter here (WrapUnique autodeduces)
https://codereview.chromium.org/2251693005/diff/1/services/ui/public/cpp/mojo... File services/ui/public/cpp/mojo_gpu_memory_buffer.cc (right): https://codereview.chromium.org/2251693005/diff/1/services/ui/public/cpp/mojo... services/ui/public/cpp/mojo_gpu_memory_buffer.cc:68: return base::WrapUnique<gfx::GpuMemoryBuffer>( On 2016/08/17 22:59:00, dcheng wrote: > Nit: Omit the template parameter here (WrapUnique autodeduces) Done.
https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... File services/ui/public/cpp/mojo_gpu_memory_buffer.cc (right): https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... services/ui/public/cpp/mojo_gpu_memory_buffer.cc:83: if (!shared_memory_->Map(gfx::BufferSizeForBufferFormat(size_, format_))) Shouldn't this use the checked variant? As we discussed, the parameters to create the MojoGpuMemoryBuffer from the GpuMemoryBufferHandle can come from an untrusted process, and the resulting size + format can be big enough to overflow size_t. https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... services/ui/public/cpp/mojo_gpu_memory_buffer.cc:93: gfx::BufferOffsetForBufferFormat(size_, format_, plane); I'm also not sure that it's safe to do this: if size is 2^31 - 1 x 2^31 - 1, I think the result can overflow: 4*((2^31 - 1) / 2 + (2^31 - 1) / 2) > 2^32 - 1. Although I guess at this point, we have the invariant that this was already mapped correctly, so maybe this is OK. https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... services/ui/public/cpp/mojo_gpu_memory_buffer.cc:104: return base::checked_cast<int>(gfx::RowSizeForBufferFormat( Why does this use checked_cast and GetHandle() uses static_cast? FWIW, I'm not sure it's OK to checked_cast here either: we don't want the GPU process dying on bad data from a less privileged process. So maybe we need to check these conditions proactively in Map(). https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... services/ui/public/cpp/mojo_gpu_memory_buffer.cc:114: gfx::RowSizeForBufferFormat(size_.width(), format_, 0)); Can this overflow an int32_t?
On 2016/08/18 02:46:35, dcheng wrote: > https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... > File services/ui/public/cpp/mojo_gpu_memory_buffer.cc (right): > > https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... > services/ui/public/cpp/mojo_gpu_memory_buffer.cc:83: if > (!shared_memory_->Map(gfx::BufferSizeForBufferFormat(size_, format_))) > Shouldn't this use the checked variant? As we discussed, the parameters to > create the MojoGpuMemoryBuffer from the GpuMemoryBufferHandle can come from an > untrusted process, and the resulting size + format can be big enough to overflow > size_t. > > https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... > services/ui/public/cpp/mojo_gpu_memory_buffer.cc:93: > gfx::BufferOffsetForBufferFormat(size_, format_, plane); > I'm also not sure that it's safe to do this: if size is 2^31 - 1 x 2^31 - 1, I > think the result can overflow: 4*((2^31 - 1) / 2 + (2^31 - 1) / 2) > 2^32 - 1. > > Although I guess at this point, we have the invariant that this was already > mapped correctly, so maybe this is OK. > > https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... > services/ui/public/cpp/mojo_gpu_memory_buffer.cc:104: return > base::checked_cast<int>(gfx::RowSizeForBufferFormat( > Why does this use checked_cast and GetHandle() uses static_cast? > > FWIW, I'm not sure it's OK to checked_cast here either: we don't want the GPU > process dying on bad data from a less privileged process. So maybe we need to > check these conditions proactively in Map(). > > https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... > services/ui/public/cpp/mojo_gpu_memory_buffer.cc:114: > gfx::RowSizeForBufferFormat(size_.width(), format_, 0)); > Can this overflow an int32_t? Sorry for my mistake. I just realized the GpuMemoryBuffer related code is used by GPU clients (untrusted process), the GMBHandle should be created by the privileged process and then sent to GPU clients. So GMBHandle should be valid, and overflow and crash shouldn't happen.
On 2016/08/18 14:14:20, Peng wrote: > On 2016/08/18 02:46:35, dcheng wrote: > > > https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... > > File services/ui/public/cpp/mojo_gpu_memory_buffer.cc (right): > > > > > https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... > > services/ui/public/cpp/mojo_gpu_memory_buffer.cc:83: if > > (!shared_memory_->Map(gfx::BufferSizeForBufferFormat(size_, format_))) > > Shouldn't this use the checked variant? As we discussed, the parameters to > > create the MojoGpuMemoryBuffer from the GpuMemoryBufferHandle can come from an > > untrusted process, and the resulting size + format can be big enough to > overflow > > size_t. > > > > > https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... > > services/ui/public/cpp/mojo_gpu_memory_buffer.cc:93: > > gfx::BufferOffsetForBufferFormat(size_, format_, plane); > > I'm also not sure that it's safe to do this: if size is 2^31 - 1 x 2^31 - 1, I > > think the result can overflow: 4*((2^31 - 1) / 2 + (2^31 - 1) / 2) > 2^32 - 1. > > > > Although I guess at this point, we have the invariant that this was already > > mapped correctly, so maybe this is OK. > > > > > https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... > > services/ui/public/cpp/mojo_gpu_memory_buffer.cc:104: return > > base::checked_cast<int>(gfx::RowSizeForBufferFormat( > > Why does this use checked_cast and GetHandle() uses static_cast? > > > > FWIW, I'm not sure it's OK to checked_cast here either: we don't want the GPU > > process dying on bad data from a less privileged process. So maybe we need to > > check these conditions proactively in Map(). > > > > > https://codereview.chromium.org/2251693005/diff/20001/services/ui/public/cpp/... > > services/ui/public/cpp/mojo_gpu_memory_buffer.cc:114: > > gfx::RowSizeForBufferFormat(size_.width(), format_, 0)); > > Can this overflow an int32_t? > > Sorry for my mistake. I just realized the GpuMemoryBuffer related code is used > by GPU clients (untrusted process), > the GMBHandle should be created by the privileged process and then sent to GPU > clients. > So GMBHandle should be valid, and overflow and crash shouldn't happen. OK, LGTM: if GMBHandle is created by the privileged process and sent to the less privileged process, there's no problem. Thanks.
The CQ bit was checked by penghuang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org Link to the patchset: https://codereview.chromium.org/2251693005/#ps20001 (title: "Fix review issue.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
penghuang@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org: PTAL. Thanks.
LGTM
The CQ bit was checked by penghuang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== services/ui: Implement MojoGpuMemoryBufferManager::CreateGpuMemoryBufferFromHandle BUG=None ========== to ========== services/ui: Implement MojoGpuMemoryBufferManager::CreateGpuMemoryBufferFromHandle BUG=None Committed: https://crrev.com/fc56b0062cb12b75c7465710e96fe51f86355913 Cr-Commit-Position: refs/heads/master@{#412965} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/fc56b0062cb12b75c7465710e96fe51f86355913 Cr-Commit-Position: refs/heads/master@{#412965} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
