ipc: Update Message::FindNext to parse brokered attachments.

ipc: Update Message::FindNext to parse brokered attachments.

The method Message::FindNext now takes an output parameter NextMessageInfo which
contains additional metadata needed for generating brokered attachments.

BUG=493414
Committed: https://crrev.com/29e0f2f9ad9a9f377995804efaf72476cc987714
Cr-Commit-Position: refs/heads/master@{#348459}

[erikchen, 2015-08-28 20:21:33]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-28 20:21:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308613006/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308613006/1

[commit-bot: I haz the power, 2015-08-28 20:35:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-28 20:35:22]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[erikchen, 2015-08-28 21:05:29]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-28 21:05:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308613006/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308613006/20001

[commit-bot: I haz the power, 2015-08-28 21:35:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-28 21:35:53]

Dry run: Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[erikchen, 2015-08-28 21:36:46]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-28 21:37:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308613006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308613006/40001

[erikchen, 2015-08-28 22:53:19]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-28 22:54:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308613006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308613006/60001

[commit-bot: I haz the power, 2015-08-28 23:34:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-28 23:34:04]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[erikchen, 2015-08-28 23:36:40]

The CQ bit was checked by erikchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-08-28 23:37:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308613006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308613006/80001

[erikchen, 2015-08-29 00:46:08]

erikchen@chromium.org changed reviewers:
+ tsepez@chromium.org

[erikchen, 2015-08-29 00:46:08]

tsepez: Please review.

[commit-bot: I haz the power, 2015-08-29 01:31:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-29 01:31:40]

Dry run: This issue passed the CQ dry run.

[Tom Sepez, 2015-09-01 18:01:55]

https://codereview.chromium.org/1308613006/diff/80001/ipc/attachment_broker_p...
File ipc/attachment_broker_privileged_win_unittest.cc (right):

https://codereview.chromium.org/1308613006/diff/80001/ipc/attachment_broker_p...
ipc/attachment_broker_privileged_win_unittest.cc:21: // TODO(erikchen):
Temporarily disabled. http://crbug.com/493414.
Do not comment out disabled test code.  Instead, name your test
DISABLED_SendHandleWin and so on and the test harness will not run it.

https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.cc
File ipc/ipc_message.cc (right):

https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.cc#newc...
ipc/ipc_message.cc:149: if (pickle_end == nullptr)
nit: if (!pickle_end)

https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.cc#newc...
ipc/ipc_message.cc:162: if (num_attachments > 100) {
nit: this should be a constant kSomethingOrOther in a header file.

https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.cc#newc...
ipc/ipc_message.cc:175: pickle_end + i * (BrokerableAttachment::kNonceSize);
nit: unnecessary ()

https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.h
File ipc/ipc_message.h (right):

https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.h#newco...
ipc/ipc_message.h:170: // are aggregated into an instance of this struct.
nit: this may be complex enough to warrant being a class with protected members.
 Your call.

https://codereview.chromium.org/1308613006/diff/80001/tools/ipc_fuzzer/messag...
File tools/ipc_fuzzer/message_lib/message_file_reader.cc (right):

https://codereview.chromium.org/1308613006/diff/80001/tools/ipc_fuzzer/messag...
tools/ipc_fuzzer/message_lib/message_file_reader.cc:111:
IPC::Message::NextMessageInfo info = IPC::Message::FindNext(begin, end);
nit: the assignment may not be cheap; a copy of the entire attachment vector
will take place.

[erikchen, 2015-09-04 02:13:39]

tsepez: PTAL

https://codereview.chromium.org/1308613006/diff/80001/ipc/attachment_broker_p...
File ipc/attachment_broker_privileged_win_unittest.cc (right):

https://codereview.chromium.org/1308613006/diff/80001/ipc/attachment_broker_p...
ipc/attachment_broker_privileged_win_unittest.cc:21: // TODO(erikchen):
Temporarily disabled. http://crbug.com/493414.
On 2015/09/01 18:01:54, Tom Sepez wrote:
> Do not comment out disabled test code.  Instead, name your test
> DISABLED_SendHandleWin and so on and the test harness will not run it.

Not sure what I was thinking. I also moved this into a separate CL.

https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.cc
File ipc/ipc_message.cc (right):

https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.cc#newc...
ipc/ipc_message.cc:149: if (pickle_end == nullptr)
On 2015/09/01 18:01:54, Tom Sepez wrote:
> nit: if (!pickle_end)

Done.

https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.cc#newc...
ipc/ipc_message.cc:162: if (num_attachments > 100) {
On 2015/09/01 18:01:54, Tom Sepez wrote:
> nit: this should be a constant kSomethingOrOther in a header file.

The previous code didn't actually prevent overflows. I've updated the logic to
correctly check for overflows, which also doesn't rely on an arbitrary constant.

I also removed the field parsing_error in NextMessageInfo, since there's no
point in providing different behavior for certain overflows (too many
attachments) from the existing behavior for a message with an unreasonable
pickle size.

https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.cc#newc...
ipc/ipc_message.cc:175: pickle_end + i * (BrokerableAttachment::kNonceSize);
On 2015/09/01 18:01:54, Tom Sepez wrote:
> nit: unnecessary ()

removed

https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.h
File ipc/ipc_message.h (right):

https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.h#newco...
ipc/ipc_message.h:170: // are aggregated into an instance of this struct.
On 2015/09/01 18:01:54, Tom Sepez wrote:
> nit: this may be complex enough to warrant being a class with protected
members.
>  Your call.

This is just a wrapper around several different pieces of information being
returned from a function. Making it a class and adding accessors adds
unnecessary complexity.

https://codereview.chromium.org/1308613006/diff/80001/tools/ipc_fuzzer/messag...
File tools/ipc_fuzzer/message_lib/message_file_reader.cc (right):

https://codereview.chromium.org/1308613006/diff/80001/tools/ipc_fuzzer/messag...
tools/ipc_fuzzer/message_lib/message_file_reader.cc:111:
IPC::Message::NextMessageInfo info = IPC::Message::FindNext(begin, end);
On 2015/09/01 18:01:54, Tom Sepez wrote:
> nit: the assignment may not be cheap; a copy of the entire attachment vector
> will take place.

I changed the signature of FindNext to take a NextMessageInfo* as an output
parameter.

[erikchen, 2015-09-10 00:10:55]

On 2015/09/04 02:13:39, erikchen wrote:
> tsepez: PTAL
> 
>
https://codereview.chromium.org/1308613006/diff/80001/ipc/attachment_broker_p...
> File ipc/attachment_broker_privileged_win_unittest.cc (right):
> 
>
https://codereview.chromium.org/1308613006/diff/80001/ipc/attachment_broker_p...
> ipc/attachment_broker_privileged_win_unittest.cc:21: // TODO(erikchen):
> Temporarily disabled. http://crbug.com/493414.
> On 2015/09/01 18:01:54, Tom Sepez wrote:
> > Do not comment out disabled test code.  Instead, name your test
> > DISABLED_SendHandleWin and so on and the test harness will not run it.
> 
> Not sure what I was thinking. I also moved this into a separate CL.
> 
> https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.cc
> File ipc/ipc_message.cc (right):
> 
>
https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.cc#newc...
> ipc/ipc_message.cc:149: if (pickle_end == nullptr)
> On 2015/09/01 18:01:54, Tom Sepez wrote:
> > nit: if (!pickle_end)
> 
> Done.
> 
>
https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.cc#newc...
> ipc/ipc_message.cc:162: if (num_attachments > 100) {
> On 2015/09/01 18:01:54, Tom Sepez wrote:
> > nit: this should be a constant kSomethingOrOther in a header file.
> 
> The previous code didn't actually prevent overflows. I've updated the logic to
> correctly check for overflows, which also doesn't rely on an arbitrary
constant.
> 
> I also removed the field parsing_error in NextMessageInfo, since there's no
> point in providing different behavior for certain overflows (too many
> attachments) from the existing behavior for a message with an unreasonable
> pickle size.
> 
>
https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.cc#newc...
> ipc/ipc_message.cc:175: pickle_end + i * (BrokerableAttachment::kNonceSize);
> On 2015/09/01 18:01:54, Tom Sepez wrote:
> > nit: unnecessary ()
> 
> removed
> 
> https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.h
> File ipc/ipc_message.h (right):
> 
>
https://codereview.chromium.org/1308613006/diff/80001/ipc/ipc_message.h#newco...
> ipc/ipc_message.h:170: // are aggregated into an instance of this struct.
> On 2015/09/01 18:01:54, Tom Sepez wrote:
> > nit: this may be complex enough to warrant being a class with protected
> members.
> >  Your call.
> 
> This is just a wrapper around several different pieces of information being
> returned from a function. Making it a class and adding accessors adds
> unnecessary complexity.
> 
>
https://codereview.chromium.org/1308613006/diff/80001/tools/ipc_fuzzer/messag...
> File tools/ipc_fuzzer/message_lib/message_file_reader.cc (right):
> 
>
https://codereview.chromium.org/1308613006/diff/80001/tools/ipc_fuzzer/messag...
> tools/ipc_fuzzer/message_lib/message_file_reader.cc:111:
> IPC::Message::NextMessageInfo info = IPC::Message::FindNext(begin, end);
> On 2015/09/01 18:01:54, Tom Sepez wrote:
> > nit: the assignment may not be cheap; a copy of the entire attachment vector
> > will take place.
> 
> I changed the signature of FindNext to take a NextMessageInfo* as an output
> parameter.

tsepez: ping

[Tom Sepez, 2015-09-11 17:16:48]

lgtm

[erikchen, 2015-09-11 17:44:25]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2015-09-11 17:45:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1308613006/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1308613006/180001

[commit-bot: I haz the power, 2015-09-11 19:09:02]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-09-11 19:09:34]

Patchset 10 (id:??) landed as
https://crrev.com/29e0f2f9ad9a9f377995804efaf72476cc987714
Cr-Commit-Position: refs/heads/master@{#348459}

[commit-bot: I haz the power, 2015-09-23 12:25:10]

Patchset 10 (id:??) landed as
https://crrev.com/29e0f2f9ad9a9f377995804efaf72476cc987714
Cr-Commit-Position: refs/heads/master@{#348459}