Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10946041:
Fix crash in NaClIPCAdapter where incomplete messages incorrectly (Closed)
|
Created:
8 years, 3 months ago by bbudge Modified:
8 years, 3 months ago CC:
chromium-reviews, native-client-reviews_googlegroups.com Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionFix crash in NaClIPCAdapter where incomplete messages incorrectly
return 8 null file descriptors.
BUG=116317
TEST=Robby Roto app works
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157828
Patch Set 1 : #
Total comments: 7
Patch Set 2 : #Messages
Total messages: 13 (0 generated)
lgtm https://chromiumcodereview.appspot.com/10946041/diff/2001/chrome/nacl/nacl_ip... File chrome/nacl/nacl_ipc_adapter.cc (right): https://chromiumcodereview.appspot.com/10946041/diff/2001/chrome/nacl/nacl_ip... chrome/nacl/nacl_ipc_adapter.cc:584: msg->ndesc_length = 0; Would it be good to either add a comment or move this to Read(), so that it's obvious why we need to set to 0? Here, it's less obvious that we only send the descriptors with the last fragment of the message.
http://codereview.chromium.org/10946041/diff/2001/chrome/nacl/nacl_ipc_adapte... File chrome/nacl/nacl_ipc_adapter.cc (right): http://codereview.chromium.org/10946041/diff/2001/chrome/nacl/nacl_ipc_adapte... chrome/nacl/nacl_ipc_adapter.cc:263: msg->ndesc_length = desc_count; It's kind of odd that your new ndesc_length assignment is far away from this one... http://codereview.chromium.org/10946041/diff/2001/chrome/nacl/nacl_ipc_adapte... chrome/nacl/nacl_ipc_adapter.cc:271: } ...is there a reason you don't do the following here: else { msg->ndesc_length = 0; } (I haven't looked at this code closely enough to know the answer.)
http://codereview.chromium.org/10946041/diff/2001/chrome/nacl/nacl_ipc_adapte... File chrome/nacl/nacl_ipc_adapter.cc (right): http://codereview.chromium.org/10946041/diff/2001/chrome/nacl/nacl_ipc_adapte... chrome/nacl/nacl_ipc_adapter.cc:263: msg->ndesc_length = desc_count; On 2012/09/19 23:25:14, Mark Seaborn wrote: > It's kind of odd that your new ndesc_length assignment is far away from this > one... Very odd. http://codereview.chromium.org/10946041/diff/2001/chrome/nacl/nacl_ipc_adapte... chrome/nacl/nacl_ipc_adapter.cc:271: } On 2012/09/19 23:25:14, Mark Seaborn wrote: > ...is there a reason you don't do the following here: > > else { > msg->ndesc_length = 0; > } > > (I haven't looked at this code closely enough to know the answer.) You're right, this is a much better place. Done. http://codereview.chromium.org/10946041/diff/2001/chrome/nacl/nacl_ipc_adapte... chrome/nacl/nacl_ipc_adapter.cc:584: msg->ndesc_length = 0; On 2012/09/19 22:47:48, dmichael wrote: > Would it be good to either add a comment or move this to Read(), so that it's > obvious why we need to set to 0? Here, it's less obvious that we only send the > descriptors with the last fragment of the message. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bbudge@chromium.org/10946041/2001
Presubmit check for 10946041-2001 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/nacl
http://codereview.chromium.org/10946041/diff/2001/chrome/nacl/nacl_ipc_adapte... File chrome/nacl/nacl_ipc_adapter.cc (right): http://codereview.chromium.org/10946041/diff/2001/chrome/nacl/nacl_ipc_adapte... chrome/nacl/nacl_ipc_adapter.cc:584: msg->ndesc_length = 0; On 2012/09/19 23:46:09, bbudge1 wrote: > On 2012/09/19 22:47:48, dmichael wrote: > > Would it be good to either add a comment or move this to Read(), > > so that it's obvious why we need to set to 0? Here, it's less > > obvious that we only send the descriptors with the last fragment > > of the message. > > Done. Did you mean to re-upload? I just see the original change.
On 2012/09/20 00:28:42, Mark Seaborn wrote: > http://codereview.chromium.org/10946041/diff/2001/chrome/nacl/nacl_ipc_adapte... > File chrome/nacl/nacl_ipc_adapter.cc (right): > > http://codereview.chromium.org/10946041/diff/2001/chrome/nacl/nacl_ipc_adapte... > chrome/nacl/nacl_ipc_adapter.cc:584: msg->ndesc_length = 0; > On 2012/09/19 23:46:09, bbudge1 wrote: > > On 2012/09/19 22:47:48, dmichael wrote: > > > Would it be good to either add a comment or move this to Read(), > > > so that it's obvious why we need to set to 0? Here, it's less > > > obvious that we only send the descriptors with the last fragment > > > of the message. > > > > Done. > > Did you mean to re-upload? I just see the original change. I'm not sure what happened. PTAL
PTAL
lgtm, that's better, thanks. I think you can TBR mseaborn if he's not around.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bbudge@chromium.org/10946041/9001
Change committed as 157828 |
