Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2488253003:
Removing 16MB size limit for Mojo buffers. (Closed)
|
Created:
4 years, 1 month ago by Jay Civelli Modified:
4 years, 1 month ago CC:
Aaron Boodman, abarth-chromium, chromium-reviews, darin (slow to review), gavinp+memory_chromium.org, mac-reviews_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoving the Mojo buffers 16MB size limit.
It is causing problems as we are migrating the current Chrome IPC messages
to mojo and are starting to use Mojo buffers to replace the existing
SharedMemory instances directly passed in IPC messages.
Since Mojo buffer use SharedMemory, get rid of the check at the mojo level
and rely on SharedMemory limit.
BUG=664239
Committed: https://crrev.com/f093c54710346e88480688d71b1116be793919ff
Cr-Commit-Position: refs/heads/master@{#431590}
Patch Set 1 #Patch Set 2 : Comment change. #Patch Set 3 : Addressing @rockot comment #Messages
Total messages: 27 (15 generated)
Description was changed from ========== Get rid of the buffer size limit for Mojo buffer. BUG= ========== to ========== Removing the Mojo buffers 16MB size limit. It is causing problems as we are migrating the current Chrome IPC messages to mojo and are starting to use Mojo buffers to replace the existing SharedMemory instance directly passed in IPC messages. The Mojo buffer size limit now matches the SharedMemory's. BUG=664239 ==========
The CQ bit was checked by jcivelli@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...
Description was changed from ========== Removing the Mojo buffers 16MB size limit. It is causing problems as we are migrating the current Chrome IPC messages to mojo and are starting to use Mojo buffers to replace the existing SharedMemory instance directly passed in IPC messages. The Mojo buffer size limit now matches the SharedMemory's. BUG=664239 ========== to ========== Removing the Mojo buffers 16MB size limit. It is causing problems as we are migrating the current Chrome IPC messages to mojo and are starting to use Mojo buffers to replace the existing SharedMemory instances directly passed in IPC messages. The Mojo buffer size limit now matches the SharedMemory's. BUG=664239 ==========
jcivelli@chromium.org changed reviewers: + dcheng@chromium.org, rockot@chromium.org
Actually, unless dcheng@ disagrees, because we just use base::SharedMemory for the internal allocation anyway, and there weren't actually limits previously imposed on child messages requesting buffer allocation, we should just completely eliminate the check in BrokerHost.
The CQ bit was checked by jcivelli@chromium.org to run a CQ dry run
On 2016/11/10 19:00:15, Ken Rockot wrote: > Actually, unless dcheng@ disagrees, because we just use base::SharedMemory for > the internal allocation anyway, and there weren't actually limits previously > imposed on child messages requesting buffer allocation, we should just > completely eliminate the check in BrokerHost. Very good point! Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Removing the Mojo buffers 16MB size limit. It is causing problems as we are migrating the current Chrome IPC messages to mojo and are starting to use Mojo buffers to replace the existing SharedMemory instances directly passed in IPC messages. The Mojo buffer size limit now matches the SharedMemory's. BUG=664239 ========== to ========== Removing the Mojo buffers 16MB size limit. It is causing problems as we are migrating the current Chrome IPC messages to mojo and are starting to use Mojo buffers to replace the existing SharedMemory instances directly passed in IPC messages. Since Mojo buffer use SharedMemory, get rid of the check at the mojo level and rely on SharedMemory limit. BUG=664239 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
I'm no longer an owner of anything in this CL, but sounds reasonable to me: I did a bit of exploring, and it looks like PlatformSharedBuffer::Create checks the return value of SharedMemory::Create() via Init(), and returns nullptr on failure.
Oh, I think we just wanted a security eye on this since it technically expands on what a sandboxed process is capable of doing via Mojo. Of course the Mojo brokering size restriction was arbitrary anyway, so maybe it never mattered. :) On Thu, Nov 10, 2016 at 2:38 PM, <dcheng@chromium.org> wrote: > I'm no longer an owner of anything in this CL, but sounds reasonable to > me: I > did a bit of exploring, and it looks like PlatformSharedBuffer::Create > checks > the return value of SharedMemory::Create() via Init(), and returns nullptr > on > failure. > > https://codereview.chromium.org/2488253003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM from a security perspective, since we couldn't really enforce limit with legacy IPC anyway.
The CQ bit was checked by jcivelli@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...)
Woops. Thought I already stamped it. LGTM
The CQ bit was checked by rockot@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.
Description was changed from ========== Removing the Mojo buffers 16MB size limit. It is causing problems as we are migrating the current Chrome IPC messages to mojo and are starting to use Mojo buffers to replace the existing SharedMemory instances directly passed in IPC messages. Since Mojo buffer use SharedMemory, get rid of the check at the mojo level and rely on SharedMemory limit. BUG=664239 ========== to ========== Removing the Mojo buffers 16MB size limit. It is causing problems as we are migrating the current Chrome IPC messages to mojo and are starting to use Mojo buffers to replace the existing SharedMemory instances directly passed in IPC messages. Since Mojo buffer use SharedMemory, get rid of the check at the mojo level and rely on SharedMemory limit. BUG=664239 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Removing the Mojo buffers 16MB size limit. It is causing problems as we are migrating the current Chrome IPC messages to mojo and are starting to use Mojo buffers to replace the existing SharedMemory instances directly passed in IPC messages. Since Mojo buffer use SharedMemory, get rid of the check at the mojo level and rely on SharedMemory limit. BUG=664239 ========== to ========== Removing the Mojo buffers 16MB size limit. It is causing problems as we are migrating the current Chrome IPC messages to mojo and are starting to use Mojo buffers to replace the existing SharedMemory instances directly passed in IPC messages. Since Mojo buffer use SharedMemory, get rid of the check at the mojo level and rely on SharedMemory limit. BUG=664239 Committed: https://crrev.com/f093c54710346e88480688d71b1116be793919ff Cr-Commit-Position: refs/heads/master@{#431590} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f093c54710346e88480688d71b1116be793919ff Cr-Commit-Position: refs/heads/master@{#431590} |
