Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2095493003:
[mojo-edk] Fix unchecked header sizes channel messages (Closed)
|
Created:
4 years, 6 months ago by Ken Rockot(use gerrit already) Modified:
4 years, 6 months ago Reviewers:
Oliver Chang CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[mojo-edk] Fix unchecked header sizes channel messages
BUG=622522
R=ochang@chromium.org
Committed: https://crrev.com/b794e408d837a01063d9c7f9f2dc549c635fd0fe
Cr-Commit-Position: refs/heads/master@{#401510}
Patch Set 1 #
Total comments: 5
Patch Set 2 : . #Messages
Total messages: 13 (4 generated)
https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc File mojo/edk/system/channel.cc (right): https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc#... mojo/edk/system/channel.cc:141: header->num_header_bytes < sizeof(Header)) { I made this condition explicit here, but we already know the following things are true already: data_num_bytes >= sizeof(Header) header->num_bytes == data_num_bytes header->num_header_bytes >= header->num_bytes How is it not guaranteed then that header->num_header_bytes >= sizeof(Header)?
https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc File mojo/edk/system/channel.cc (right): https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc#... mojo/edk/system/channel.cc:141: header->num_header_bytes < sizeof(Header)) { On 2016/06/23 at 00:26:37, Ken Rockot wrote: > I made this condition explicit here, but we already know the following things are true already: > > data_num_bytes >= sizeof(Header) > header->num_bytes == data_num_bytes > header->num_header_bytes >= header->num_bytes > > How is it not guaranteed then that header->num_header_bytes >= sizeof(Header)? Wait, nevermind. I did backwards things in my head. We only know that num_bytes >= num_header_bytes, which is uninteresting for this bug. :)
lgtm https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc File mojo/edk/system/channel.cc (right): https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc#... mojo/edk/system/channel.cc:141: header->num_header_bytes < sizeof(Header)) { On 2016/06/23 00:30:35, Ken Rockot wrote: > On 2016/06/23 at 00:26:37, Ken Rockot wrote: > > I made this condition explicit here, but we already know the following things > are true already: > > > > data_num_bytes >= sizeof(Header) > > header->num_bytes == data_num_bytes > > header->num_header_bytes >= header->num_bytes > > > > How is it not guaranteed then that header->num_header_bytes >= sizeof(Header)? > > Wait, nevermind. I did backwards things in my head. We only know that num_bytes > >= num_header_bytes, which is uninteresting for this bug. :) yep :) I missed this during the first review too https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc#... mojo/edk/system/channel.cc:156: uint32_t max_handles = (extra_header_size - sizeof(MachPortsExtraHeader)) / not sure if it's worth sanity checking max_handles, but up to you if it makes sense to.
On 2016/06/23 at 00:31:18, ochang wrote: > lgtm > > https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc > File mojo/edk/system/channel.cc (right): > > https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc#... > mojo/edk/system/channel.cc:141: header->num_header_bytes < sizeof(Header)) { > On 2016/06/23 00:30:35, Ken Rockot wrote: > > On 2016/06/23 at 00:26:37, Ken Rockot wrote: > > > I made this condition explicit here, but we already know the following things > > are true already: > > > > > > data_num_bytes >= sizeof(Header) > > > header->num_bytes == data_num_bytes > > > header->num_header_bytes >= header->num_bytes > > > > > > How is it not guaranteed then that header->num_header_bytes >= sizeof(Header)? > > > > Wait, nevermind. I did backwards things in my head. We only know that num_bytes > > >= num_header_bytes, which is uninteresting for this bug. :) > > yep :) I missed this during the first review too > > https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc#... > mojo/edk/system/channel.cc:156: uint32_t max_handles = (extra_header_size - sizeof(MachPortsExtraHeader)) / > not sure if it's worth sanity checking max_handles, but up to you if it makes sense to. Sanity checking against what? You mean just making sure it's not some largish value like >= uint16 max? Also regarding unification of some OnReadComplete and Deserialize logic: I'd like to do it, but I would prefer to wait until we can finally land https://codereview.chromium.org/1975073002 since waiting will make rebasing that patch a bit less painful.
On 2016/06/23 00:36:04, Ken Rockot wrote: > On 2016/06/23 at 00:31:18, ochang wrote: > > lgtm > > > > https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc > > File mojo/edk/system/channel.cc (right): > > > > > https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc#... > > mojo/edk/system/channel.cc:141: header->num_header_bytes < sizeof(Header)) { > > On 2016/06/23 00:30:35, Ken Rockot wrote: > > > On 2016/06/23 at 00:26:37, Ken Rockot wrote: > > > > I made this condition explicit here, but we already know the following > things > > > are true already: > > > > > > > > data_num_bytes >= sizeof(Header) > > > > header->num_bytes == data_num_bytes > > > > header->num_header_bytes >= header->num_bytes > > > > > > > > How is it not guaranteed then that header->num_header_bytes >= > sizeof(Header)? > > > > > > Wait, nevermind. I did backwards things in my head. We only know that > num_bytes > > > >= num_header_bytes, which is uninteresting for this bug. :) > > > > yep :) I missed this during the first review too > > > > > https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc#... > > mojo/edk/system/channel.cc:156: uint32_t max_handles = (extra_header_size - > sizeof(MachPortsExtraHeader)) / > > not sure if it's worth sanity checking max_handles, but up to you if it makes > sense to. > > Sanity checking against what? You mean just making sure it's not some largish > value like >= uint16 max? In Channel::Message's constructor, I see: DCHECK_LE(max_handles_, kMaxAttachedHandles); // kMaxAttachedHandles == 128. > > Also regarding unification of some OnReadComplete and Deserialize logic: I'd > like to do it, but I would prefer to wait until we can finally land > https://codereview.chromium.org/1975073002 since waiting will make rebasing that > patch a bit less painful. That would be great :)
https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc File mojo/edk/system/channel.cc (right): https://codereview.chromium.org/2095493003/diff/1/mojo/edk/system/channel.cc#... mojo/edk/system/channel.cc:156: uint32_t max_handles = (extra_header_size - sizeof(MachPortsExtraHeader)) / On 2016/06/23 at 00:31:17, Oliver Chang wrote: > not sure if it's worth sanity checking max_handles, but up to you if it makes sense to. oh right... done!
The CQ bit was checked by rockot@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ochang@chromium.org Link to the patchset: https://codereview.chromium.org/2095493003/#ps20001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2095493003/20001
Description was changed from ========== Fix unchecked header sizes in EDK channel messages BUG=622522 R=ochang@chromium.org ========== to ========== [mojo-edk] Fix unchecked header sizes channel messages BUG=622522 R=ochang@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [mojo-edk] Fix unchecked header sizes channel messages BUG=622522 R=ochang@chromium.org ========== to ========== [mojo-edk] Fix unchecked header sizes channel messages BUG=622522 R=ochang@chromium.org Committed: https://crrev.com/b794e408d837a01063d9c7f9f2dc549c635fd0fe Cr-Commit-Position: refs/heads/master@{#401510} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b794e408d837a01063d9c7f9f2dc549c635fd0fe Cr-Commit-Position: refs/heads/master@{#401510} |
