IPC fuzzer child process component

IPC fuzzer child process component

Fuzzer child process takes messages from a testcase file specified by --ipc-fuzzer-testcase and sends them across IPC.

Renderer process is replaced by the fuzzer process using --renderer-cmd-prefix, which is only supported under POSIX.

BUG=260848
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237795

[aedla, 2013-07-12 16:34:46]

Tom, could you please review this?

[Tom Sepez, 2013-07-12 18:47:19]

My overwhelming concern is that generating the message file is going to be
difficult. Tell me about your plans for doing so.  thanks

https://codereview.chromium.org/18254010/diff/3001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/18254010/diff/3001/build/common.gypi#newcode735
build/common.gypi:735: 'enable_ipc_fuzzer%': 0,
Not sure this is the right way to include this feature. Someone other than
myself (like an OWNER) should review this.

https://codereview.chromium.org/18254010/diff/3001/chrome/app/chrome_main_del...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/18254010/diff/3001/chrome/app/chrome_main_del...
chrome/app/chrome_main_delegate.cc:109: extern int IpcFuzzerMain(const
content::MainFunctionParams&);
#if defined(ENABLE_IPC_FUZZER) around this prototype.

https://codereview.chromium.org/18254010/diff/3001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/18254010/diff/3001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:102: #include
"chrome/common/ipc_fuzzer_messages.h"
#if defined(EABLE_IPC_FUZZER) and move down lower.

https://codereview.chromium.org/18254010/diff/3001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/18254010/diff/3001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.h:250: virtual bool
ShouldKillChildOnBadMessage() OVERRIDE;
nit: maybe ShouldKillChildProcessOnBadMessage.

https://codereview.chromium.org/18254010/diff/3001/chrome/browser/ipc_fuzzer_...
File chrome/browser/ipc_fuzzer_host.cc (right):

https://codereview.chromium.org/18254010/diff/3001/chrome/browser/ipc_fuzzer_...
chrome/browser/ipc_fuzzer_host.cc:16: namespace chrome {
ditto here for ifdef

https://codereview.chromium.org/18254010/diff/3001/chrome/browser/ipc_fuzzer_...
File chrome/browser/ipc_fuzzer_host.h (right):

https://codereview.chromium.org/18254010/diff/3001/chrome/browser/ipc_fuzzer_...
chrome/browser/ipc_fuzzer_host.h:10: namespace chrome {
Does this whole thing get an ifdef?

https://codereview.chromium.org/18254010/diff/3001/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/18254010/diff/3001/content/public/browser/con...
content/public/browser/content_browser_client.h:580: // if it sends a bad IPC
message.
Comment about "apart from testing, it is an incredibly bad idea from as securiyt
perspective to allow a child process to survive after sending a bad message."

[aedla, 2013-07-15 16:12:07]

On 2013/07/12 18:47:19, Tom Sepez wrote:
> My overwhelming concern is that generating the message file is going to be
> difficult. Tell me about your plans for doing so.  thanks

I added a message logger as the first filter to RenderProcessHostImpl, that
dumps all the messages to a file and then ran all browser tests. It logged 254
message types, total ~100MB of data. Next, I'll write a bitflipper, that mutates
messages from a random log file to create a testcase. I think this will be good
enough at first. Then I'd like to start writing mutators for each type to avoid
deserialize failures.

https://codereview.chromium.org/18254010/diff/3001/chrome/app/chrome_main_del...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/18254010/diff/3001/chrome/app/chrome_main_del...
chrome/app/chrome_main_delegate.cc:109: extern int IpcFuzzerMain(const
content::MainFunctionParams&);
On 2013/07/12 18:47:20, Tom Sepez wrote:
> #if defined(ENABLE_IPC_FUZZER) around this prototype.

Done.

https://codereview.chromium.org/18254010/diff/3001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/18254010/diff/3001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.cc:102: #include
"chrome/common/ipc_fuzzer_messages.h"
On 2013/07/12 18:47:20, Tom Sepez wrote:
> #if defined(EABLE_IPC_FUZZER) and move down lower.

Oh, the include wasn't necessary actually.

https://codereview.chromium.org/18254010/diff/3001/chrome/browser/chrome_cont...
File chrome/browser/chrome_content_browser_client.h (right):

https://codereview.chromium.org/18254010/diff/3001/chrome/browser/chrome_cont...
chrome/browser/chrome_content_browser_client.h:250: virtual bool
ShouldKillChildOnBadMessage() OVERRIDE;
On 2013/07/12 18:47:20, Tom Sepez wrote:
> nit: maybe ShouldKillChildProcessOnBadMessage.

Done.

https://codereview.chromium.org/18254010/diff/3001/chrome/browser/ipc_fuzzer_...
File chrome/browser/ipc_fuzzer_host.h (right):

https://codereview.chromium.org/18254010/diff/3001/chrome/browser/ipc_fuzzer_...
chrome/browser/ipc_fuzzer_host.h:10: namespace chrome {
On 2013/07/12 18:47:20, Tom Sepez wrote:
> Does this whole thing get an ifdef?  

I actually put this file to ipc_fuzzer.gypi, so it is not compiled in if the
fuzzer is disabled.

https://codereview.chromium.org/18254010/diff/3001/content/public/browser/con...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/18254010/diff/3001/content/public/browser/con...
content/public/browser/content_browser_client.h:580: // if it sends a bad IPC
message.
On 2013/07/12 18:47:20, Tom Sepez wrote:
> Comment about "apart from testing, it is an incredibly bad idea from as
securiyt
> perspective to allow a child process to survive after sending a bad message."

Done.

[aedla, 2013-07-15 16:14:32]

rsleevi, could you please take a look at gyp file changes?

[aedla, 2013-07-15 16:19:46]

On 2013/07/15 16:14:32, aedla wrote:
> rsleevi, could you please take a look at gyp file changes?

Ah sorry, I guess I need an LGTM from an owner for chrome.gyp, I'll ask someone
else then.

[jam, 2013-07-15 16:24:39]

driveby: try to do this in a way that avoids adding test-only code to production
files (even if it does get ifdef'd out).

[aedla, 2013-07-15 16:25:02]

jochen@, could you please take a look at gyp file changes?

[aedla, 2013-07-15 17:24:49]

On 2013/07/15 16:24:39, jam wrote:
> driveby: try to do this in a way that avoids adding test-only code to
production
> files (even if it does get ifdef'd out).

I tried to keep changes to production files minimal and avoid changes to
content.

I did some more thinking now, to see if I could reduce the production file
changes. There is basically 3 changes:

1) Child process finds out that it is the fuzzer and runs IpcFuzzerMain.

I guess I could move the fuzzer code out of the chrome binary and execute it
with --renderer-cmd-prefix.

2) Browser sends the sandboxed fuzzer process the testcase.

If the fuzzer would be in a separate binary and run unsandboxed, it could open
its own testcase.

3) Browser won't kill child if it sends a bad message.

I don't think this can be avoided. By definition, fuzzer will send bad messages
and fuzzing is difficult if the fuzzer gets killed often.

tsepez, what do you think about the idea of moving the fuzzer to a separate
binary?

[Tom Sepez, 2013-07-15 17:30:28]

On 2013/07/15 17:24:49, aedla wrote:
> On 2013/07/15 16:24:39, jam wrote:
> > driveby: try to do this in a way that avoids adding test-only code to
> production
> > files (even if it does get ifdef'd out).
> 
> I tried to keep changes to production files minimal and avoid changes to
> content.
> 
> I did some more thinking now, to see if I could reduce the production file
> changes. There is basically 3 changes:
> 
> 1) Child process finds out that it is the fuzzer and runs IpcFuzzerMain.
> 
> I guess I could move the fuzzer code out of the chrome binary and execute it
> with --renderer-cmd-prefix.
> 
> 2) Browser sends the sandboxed fuzzer process the testcase.
> 
> If the fuzzer would be in a separate binary and run unsandboxed, it could open
> its own testcase.
> 
> 3) Browser won't kill child if it sends a bad message.
> 
> I don't think this can be avoided. By definition, fuzzer will send bad
messages
> and fuzzing is difficult if the fuzzer gets killed often.
> 
> tsepez, what do you think about the idea of moving the fuzzer to a separate
> binary?
Making a separate binary is OK for me; the last time I did this I put the
fuzzing stuff in a separate shared library.  That might be another approach.

Also, note that processes get killed when the format of the message is wrong;
eg. indiscriminate bit flipping. We've done indiscriminate bit flipping in the
past, and it doesn not reveal much.  The most important thing is to focus on
understanding the messages; once you do this you'll find that the process
doesn't get killed all that often when you fuzz the payload rather than the
container.

[jochen (gone - plz use gerrit), 2013-07-16 09:07:58]

some comments

can you please file a bug and reference it from the cl description?

won't the majority of IPCs the fuzzer sends just be ignored because they come
have routing IDs the browser knows nothing about?

https://codereview.chromium.org/18254010/diff/26001/chrome/browser/ipc_fuzzer...
File chrome/browser/ipc_fuzzer_host.cc (right):

https://codereview.chromium.org/18254010/diff/26001/chrome/browser/ipc_fuzzer...
chrome/browser/ipc_fuzzer_host.cc:44: host->GetHandle(),
if I'm not mistaken, this won't work on Linux. You invoke this method when the
RPH is created. This is, however, before the process actually exists, and so
host->GetHandle() is not the actual handle which in turn is required on Windows
to create a FileForTransit

https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
File chrome/ipc_fuzzer/ipc_fuzzer_main.cc (right):

https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
chrome/ipc_fuzzer/ipc_fuzzer_main.cc:26: class IpcTestcaseRunner : public
IPC::Listener {
this entire class can go into an anonymous namespace, no?

https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
chrome/ipc_fuzzer/ipc_fuzzer_main.cc:28: IpcTestcaseRunner();
should have a virtual destructor

https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
chrome/ipc_fuzzer/ipc_fuzzer_main.cc:49: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
chrome/ipc_fuzzer/ipc_fuzzer_main.cc:82: if (len > INT_MAX)
how can this happen?

https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
chrome/ipc_fuzzer/ipc_fuzzer_main.cc:120: printf("Sending message %lu/%lu\n",
current_message_, messages_.size());
Why not LOG(INFO) ?

https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
chrome/ipc_fuzzer/ipc_fuzzer_main.cc:124: current_message_++;
++current_message_;

[Tom Sepez, 2013-07-16 18:21:43]

Jochen is right, I remember now that the routing ID issue was why I tried to do
this "in-process" of a real renderer the last time I took a stab at it ...

[aedla, 2013-07-16 20:42:47]

On 2013/07/16 09:07:58, jochen wrote:
> won't the majority of IPCs the fuzzer sends just be ignored because they come
> have routing IDs the browser knows nothing about?

In theory we can have messages in the testcase, that create a RenderView. In
practice we already do, because testcases come from valid logged traffic. From
the fuzzing I've done on my machine, it seemed that routed messages are mostly
handled fine. I'll have to double check this though, to know I'm not being
fooled.

https://codereview.chromium.org/18254010/diff/26001/chrome/browser/ipc_fuzzer...
File chrome/browser/ipc_fuzzer_host.cc (right):

https://codereview.chromium.org/18254010/diff/26001/chrome/browser/ipc_fuzzer...
chrome/browser/ipc_fuzzer_host.cc:44: host->GetHandle(),
On 2013/07/16 09:07:58, jochen wrote:
> if I'm not mistaken, this won't work on Linux. You invoke this method when the
> RPH is created. This is, however, before the process actually exists, and so
> host->GetHandle() is not the actual handle which in turn is required on
Windows
> to create a FileForTransit

Did you mean this won't work on Windows. That could be true, I haven't tested it
on Windows. I won't fix it though, because most likely I'll rewrite the CL so
that the child process opens the testcase itself.

https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
File chrome/ipc_fuzzer/ipc_fuzzer_main.cc (right):

https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
chrome/ipc_fuzzer/ipc_fuzzer_main.cc:26: class IpcTestcaseRunner : public
IPC::Listener {
On 2013/07/16 09:07:58, jochen wrote:
> this entire class can go into an anonymous namespace, no?

Done.

https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
chrome/ipc_fuzzer/ipc_fuzzer_main.cc:28: IpcTestcaseRunner();
On 2013/07/16 09:07:58, jochen wrote:
> should have a virtual destructor

Done.

https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
chrome/ipc_fuzzer/ipc_fuzzer_main.cc:49: };
On 2013/07/16 09:07:58, jochen wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
chrome/ipc_fuzzer/ipc_fuzzer_main.cc:82: if (len > INT_MAX)
On 2013/07/16 09:07:58, jochen wrote:
> how can this happen?

By having a >= 2GB message in the testcase file, I think.

https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
chrome/ipc_fuzzer/ipc_fuzzer_main.cc:120: printf("Sending message %lu/%lu\n",
current_message_, messages_.size());
On 2013/07/16 09:07:58, jochen wrote:
> Why not LOG(INFO) ?

Done.

https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
chrome/ipc_fuzzer/ipc_fuzzer_main.cc:124: current_message_++;
On 2013/07/16 09:07:58, jochen wrote:
> ++current_message_;

Done.

[jochen (gone - plz use gerrit), 2013-07-18 07:00:42]

On 2013/07/16 20:42:47, aedla wrote:
> On 2013/07/16 09:07:58, jochen wrote:
> > won't the majority of IPCs the fuzzer sends just be ignored because they
come
> > have routing IDs the browser knows nothing about?
> 
> In theory we can have messages in the testcase, that create a RenderView. In
> practice we already do, because testcases come from valid logged traffic. From
> the fuzzing I've done on my machine, it seemed that routed messages are mostly
> handled fine. I'll have to double check this though, to know I'm not being
> fooled.
> 
>
https://codereview.chromium.org/18254010/diff/26001/chrome/browser/ipc_fuzzer...
> File chrome/browser/ipc_fuzzer_host.cc (right):
> 
>
https://codereview.chromium.org/18254010/diff/26001/chrome/browser/ipc_fuzzer...
> chrome/browser/ipc_fuzzer_host.cc:44: host->GetHandle(),
> On 2013/07/16 09:07:58, jochen wrote:
> > if I'm not mistaken, this won't work on Linux. You invoke this method when
the
> > RPH is created. This is, however, before the process actually exists, and so
> > host->GetHandle() is not the actual handle which in turn is required on
> Windows
> > to create a FileForTransit
> 
> Did you mean this won't work on Windows. That could be true, I haven't tested
it
> on Windows. I won't fix it though, because most likely I'll rewrite the CL so
> that the child process opens the testcase itself.

Ok, that means you'll abandon this CL?


> 
>
https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
> File chrome/ipc_fuzzer/ipc_fuzzer_main.cc (right):
> 
>
https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
> chrome/ipc_fuzzer/ipc_fuzzer_main.cc:26: class IpcTestcaseRunner : public
> IPC::Listener {
> On 2013/07/16 09:07:58, jochen wrote:
> > this entire class can go into an anonymous namespace, no?
> 
> Done.
> 
>
https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
> chrome/ipc_fuzzer/ipc_fuzzer_main.cc:28: IpcTestcaseRunner();
> On 2013/07/16 09:07:58, jochen wrote:
> > should have a virtual destructor
> 
> Done.
> 
>
https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
> chrome/ipc_fuzzer/ipc_fuzzer_main.cc:49: };
> On 2013/07/16 09:07:58, jochen wrote:
> > DISALLOW_COPY_AND_ASSIGN
> 
> Done.
> 
>
https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
> chrome/ipc_fuzzer/ipc_fuzzer_main.cc:82: if (len > INT_MAX)
> On 2013/07/16 09:07:58, jochen wrote:
> > how can this happen?
> 
> By having a >= 2GB message in the testcase file, I think.
> 
>
https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
> chrome/ipc_fuzzer/ipc_fuzzer_main.cc:120: printf("Sending message %lu/%lu\n",
> current_message_, messages_.size());
> On 2013/07/16 09:07:58, jochen wrote:
> > Why not LOG(INFO) ?
> 
> Done.
> 
>
https://codereview.chromium.org/18254010/diff/26001/chrome/ipc_fuzzer/ipc_fuz...
> chrome/ipc_fuzzer/ipc_fuzzer_main.cc:124: current_message_++;
> On 2013/07/16 09:07:58, jochen wrote:
> > ++current_message_;
> 
> Done.

[aedla, 2013-07-18 10:24:04]

On 2013/07/18 07:00:42, jochen wrote:
> On 2013/07/16 20:42:47, aedla wrote:
> > On 2013/07/16 09:07:58, jochen wrote:
> > > won't the majority of IPCs the fuzzer sends just be ignored because they
> come
> > > have routing IDs the browser knows nothing about?
> > 
> > In theory we can have messages in the testcase, that create a RenderView. In
> > practice we already do, because testcases come from valid logged traffic.
From
> > the fuzzing I've done on my machine, it seemed that routed messages are
mostly
> > handled fine. I'll have to double check this though, to know I'm not being
> > fooled.
> > 
> >
>
https://codereview.chromium.org/18254010/diff/26001/chrome/browser/ipc_fuzzer...
> > File chrome/browser/ipc_fuzzer_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/18254010/diff/26001/chrome/browser/ipc_fuzzer...
> > chrome/browser/ipc_fuzzer_host.cc:44: host->GetHandle(),
> > On 2013/07/16 09:07:58, jochen wrote:
> > > if I'm not mistaken, this won't work on Linux. You invoke this method when
> the
> > > RPH is created. This is, however, before the process actually exists, and
so
> > > host->GetHandle() is not the actual handle which in turn is required on
> > Windows
> > > to create a FileForTransit
> > 
> > Did you mean this won't work on Windows. That could be true, I haven't
tested
> it
> > on Windows. I won't fix it though, because most likely I'll rewrite the CL
so
> > that the child process opens the testcase itself.
> 
> Ok, that means you'll abandon this CL?

I think I'll just upload another patch set.

[aedla, 2013-08-27 17:14:26]

Ok, the fuzzer is now in a separate binary. Added a convenience python script,
that replaces the child processes with the fuzzer and specifies
--ipc-fuzzer-testcase. I won't support Windows for now, so Windows builds won't
be able to replay testcases.

jochen, could you PTAL at chrome/* and maybe check that the gyp files are sane?

[aedla, 2013-08-27 17:18:27]

brettw, could you please take a look at changes in base/ and content/ ?

[jam, 2013-08-29 21:43:16]

it seems that content/chrome changes would be simplified if we added a new
command line flag to content_switches like kDisableKillAfterBadIPC (or something
similar).

[aedla, 2013-08-30 09:54:55]

On 2013/08/29 21:43:16, jam wrote:
> it seems that content/chrome changes would be simplified if we added a new
> command line flag to content_switches like kDisableKillAfterBadIPC (or
something
> similar).

I agree, done.

[aedla, 2013-08-30 12:03:20]

I notice now that jam is in content OWNERS, maybe you'll review content changes,
as you've already looked at them.

So to be clear, the reviews I'm asking are:

base             - brettw
build, chrome    - jochen
content          - jam
tools/ipc_fuzzer - jochen, maybe you could take another look at this once you're
back from OOO

Thanks!

[jam, 2013-08-30 15:05:17]

no changes to chrome are necessary now...

[aedla, 2013-08-30 16:04:36]

On 2013/08/30 15:05:17, jam wrote:
> no changes to chrome are necessary now...

I still need to pass kIpcFuzzerTestcase flag on to the fuzzer.

[jam, 2013-08-30 20:59:30]

On 2013/08/30 16:04:36, aedla wrote:
> On 2013/08/30 15:05:17, jam wrote:
> > no changes to chrome are necessary now...
> 
> I still need to pass kIpcFuzzerTestcase flag on to the fuzzer.

i see, i didn't know how this worked until i clicked on the bug. it would be
nice to add that to the cl comment for this checkin.

we shouldn't duplicate kIpcFuzzerTestcase... just make
tools/ipc_fuzzer/ipc_fuzzer_main.cc include chrome_switches.cc

https://codereview.chromium.org/18254010/diff/78001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/18254010/diff/78001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:911: const char kIpcFuzzerTestcase[]        =
"ipc-fuzzer-testcase";
nit: please follow the indentation for the equal sign

[aedla, 2013-09-03 16:30:09]

On 2013/08/30 20:59:30, jam wrote:

> we shouldn't duplicate kIpcFuzzerTestcase... just make
> tools/ipc_fuzzer/ipc_fuzzer_main.cc include chrome_switches.cc

At first I didn't want to add chrome common dependency for just a flag. But I
need it later anyway, so I gave it a try. git cl upload ran into:

** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  tools/ipc_fuzzer/ipc_fuzzer_main.cc
      Illegal include: "chrome/common/chrome_switches.h"
    Because of no rule applying.

I should figure this out, I will need to include "chrome/common/all_messages.h"
later on.

https://codereview.chromium.org/18254010/diff/78001/chrome/common/chrome_swit...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/18254010/diff/78001/chrome/common/chrome_swit...
chrome/common/chrome_switches.cc:911: const char kIpcFuzzerTestcase[]        =
"ipc-fuzzer-testcase";
On 2013/08/30 20:59:30, jam wrote:
> nit: please follow the indentation for the equal sign

Done.

[brettw, 2013-09-03 20:21:18]

I'd prefer not to do the changes in base. The existing function is not difficult
at all to use, and this class is so large I think we're well beyond the point at
which adding helper functions actually makes the class more difficult to
understand as a whole.

https://codereview.chromium.org/18254010/diff/55001/base/command_line.h
File base/command_line.h (right):

https://codereview.chromium.org/18254010/diff/55001/base/command_line.h#newco...
base/command_line.h:132: void CopySwitchesFrom(const CommandLine& source,
I don't think this is a very commonly used function and the existing one isn't
*that* hard to call with only one argument, so my preference would be to keep
the API smaller and just have the one caller of the new one use the existing
version.

[brettw, 2013-09-03 20:21:36]

Whoops, got a comment twice.

[jam, 2013-09-03 20:35:54]

On 2013/09/03 16:30:09, aedla wrote:
> On 2013/08/30 20:59:30, jam wrote:
> 
> > we shouldn't duplicate kIpcFuzzerTestcase... just make
> > tools/ipc_fuzzer/ipc_fuzzer_main.cc include chrome_switches.cc
> 
> At first I didn't want to add chrome common dependency for just a flag. But I
> need it later anyway, so I gave it a try. git cl upload ran into:
> 
> ** Presubmit ERRORS **
> You added one or more #includes that violate checkdeps rules.
>   tools/ipc_fuzzer/ipc_fuzzer_main.cc
>       Illegal include: "chrome/common/chrome_switches.h"
>     Because of no rule applying.
> 
> I should figure this out, I will need to include
"chrome/common/all_messages.h"
> later on.

add a tools/ipc_fuzzer/DEPS and add a +chrome to it. code in tools is ok to
include chrome

[aedla, 2013-09-09 11:46:50]

On 2013/09/03 20:21:18, brettw wrote:
> I'd prefer not to do the changes in base. The existing function is not
difficult
> at all to use, and this class is so large I think we're well beyond the point
at
> which adding helper functions actually makes the class more difficult to
> understand as a whole.
> 
> https://codereview.chromium.org/18254010/diff/55001/base/command_line.h
> File base/command_line.h (right):
> 
>
https://codereview.chromium.org/18254010/diff/55001/base/command_line.h#newco...
> base/command_line.h:132: void CopySwitchesFrom(const CommandLine& source,
> I don't think this is a very commonly used function and the existing one isn't
> *that* hard to call with only one argument, so my preference would be to keep
> the API smaller and just have the one caller of the new one use the existing
> version.

I would argue that the simplification of callers outweighs an added function.
Grepping returned around 5 places that pass an array with one flag to
CopySwitchesFrom. But yeah, it's a matter of preference. And callers might
actually prefer CopySwitchesFrom to make it easy to add new flags. Removed
changes to base.

On 2013/09/03 20:35:54, jam wrote:
> On 2013/09/03 16:30:09, aedla wrote:
> > On 2013/08/30 20:59:30, jam wrote:
> > 
> > > we shouldn't duplicate kIpcFuzzerTestcase... just make
> > > tools/ipc_fuzzer/ipc_fuzzer_main.cc include chrome_switches.cc
> > 
> > At first I didn't want to add chrome common dependency for just a flag. But
I
> > need it later anyway, so I gave it a try. git cl upload ran into:
> > 
> > ** Presubmit ERRORS **
> > You added one or more #includes that violate checkdeps rules.
> >   tools/ipc_fuzzer/ipc_fuzzer_main.cc
> >       Illegal include: "chrome/common/chrome_switches.h"
> >     Because of no rule applying.
> > 
> > I should figure this out, I will need to include
> "chrome/common/all_messages.h"
> > later on.
> 
> add a tools/ipc_fuzzer/DEPS and add a +chrome to it. code in tools is ok to
> include chrome

Thanks, done.

[brettw, 2013-09-16 17:47:29]

LGTM on integration, I didn't check the fuzzer itself.

[aedla, 2013-10-29 20:42:49]

jochen@, you looked at the fuzzer code a while back when it was still inside the
chrome binary. I've rewritten it since to be in a separate binary. Could you
please take another look at the fuzzer code? It is now in tools/ipc_fuzzer.

[jochen (gone - plz use gerrit), 2013-10-31 10:46:59]

some comments

Can you add an OWNERS file to the ipc fuzzer dir?

Maybe somebody from the security team can chime in. I don't really know a lot
about fuzzers

https://codereview.chromium.org/18254010/diff/91001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/18254010/diff/91001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:1342:
arraysize(kIpcFuzzerSwitches));
for just one switch, using a static array seems to be overkill

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/DEPS
File tools/ipc_fuzzer/DEPS (right):

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/DEPS#new...
tools/ipc_fuzzer/DEPS:2: "+chrome",
it's kinda ugly that you depend on chrome just for one command line switch.
maybe you can stuff the switch into base or ipc or something to avoid this
dependency?

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/ipc_fuzz...
File tools/ipc_fuzzer/ipc_fuzzer_main.cc (right):

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/ipc_fuzz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:33: void Initialize(int argc, const char
**argv);
please add comments explaining what the methods do

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/ipc_fuzz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:38: virtual bool OnMessageReceived(const
IPC::Message& message) OVERRIDE;
nit. add // IPC::Listener implementation. comment before this line

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/ipc_fuzz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:48: base::Thread io_thread_;
during shutdown, the io thread will die before that channel..

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/ipc_fuzz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:157:
static_cast<uint16*>(const_cast<void*>(message->data()))[8] = 0;
what if data is too small?

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/ipc_fuzz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:176: base::TimeDelta::FromMilliseconds(1),
nit. indentation

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/play_tes...
File tools/ipc_fuzzer/play_testcase.py (right):

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/play_tes...
tools/ipc_fuzzer/play_testcase.py:22: sys.exit(1)
return 1

[aedla, 2013-11-26 17:09:50]

jochen@, thanks for the comments.

On 2013/10/31 10:46:59, jochen wrote:
> some comments
> 
> Can you add an OWNERS file to the ipc fuzzer dir?

Added.

> 
> Maybe somebody from the security team can chime in. I don't really know a lot
> about fuzzers

There's not much fuzzing specific code here. It is really a process that
extracts messages from a file and sends them across IPC. But I'll ask tsepez if
he is up for taking another look.

https://codereview.chromium.org/18254010/diff/91001/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/18254010/diff/91001/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:1342:
arraysize(kIpcFuzzerSwitches));
On 2013/10/31 10:47:00, jochen wrote:
> for just one switch, using a static array seems to be overkill

I don't like it either. Other callers of CopySwitchesFrom() use this pattern
even for a single switch and I'd like to stay consistent. In an earlier patch
set I introduced CommandLine::CopySwitchFrom(), but I removed it after a debate
with brettw. I guess an argument for using an array is that adding new switches
is easy.

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/DEPS
File tools/ipc_fuzzer/DEPS (right):

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/DEPS#new...
tools/ipc_fuzzer/DEPS:2: "+chrome",
On 2013/10/31 10:47:00, jochen wrote:
> it's kinda ugly that you depend on chrome just for one command line switch.
> maybe you can stuff the switch into base or ipc or something to avoid this
> dependency?

I'd rather keep this dependency because the next CL will need all the IPC
messages from chrome and content.

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/ipc_fuzz...
File tools/ipc_fuzzer/ipc_fuzzer_main.cc (right):

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/ipc_fuzz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:33: void Initialize(int argc, const char
**argv);
On 2013/10/31 10:47:00, jochen wrote:
> please add comments explaining what the methods do

Done.

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/ipc_fuzz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:38: virtual bool OnMessageReceived(const
IPC::Message& message) OVERRIDE;
On 2013/10/31 10:47:00, jochen wrote:
> nit. add // IPC::Listener implementation. comment before this line

Done.

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/ipc_fuzz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:48: base::Thread io_thread_;
On 2013/10/31 10:47:00, jochen wrote:
> during shutdown, the io thread will die before that channel..

Nice catch. I added channel_.reset() to the destructor.

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/ipc_fuzz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:157:
static_cast<uint16*>(const_cast<void*>(message->data()))[8] = 0;
On 2013/10/31 10:47:00, jochen wrote:
> what if data is too small?

It will actually be large enough, IPC::Message::FindNext() requires data length
to be >= header length. Header is 10 * uint16 on POSIX, which is the only
supported platform. But yeah, it is difficult to verify and ugly. Also the
header could change. I removed it. It is now up to the testcase generator to
clear the num_fds field.

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/ipc_fuzz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:176: base::TimeDelta::FromMilliseconds(1),
On 2013/10/31 10:47:00, jochen wrote:
> nit. indentation

Done.

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/play_tes...
File tools/ipc_fuzzer/play_testcase.py (right):

https://codereview.chromium.org/18254010/diff/91001/tools/ipc_fuzzer/play_tes...
tools/ipc_fuzzer/play_testcase.py:22: sys.exit(1)
On 2013/10/31 10:47:00, jochen wrote:
> return 1

Done.

[aedla, 2013-11-26 17:40:26]

So the current review status is:

build, content   - LGTM from brettw
chrome           - I think I need LGTM from jochen or jam
tools/ipc_fuzzer - jochen looked at it, would be nice if someone from the
security team can take a look as well

tsepez@, maybe you would also be up for looking at tools/ipc_fuzzer? Last time
it was still inside the chrome binary, it is now a separate binary. Also, would
you mind being in tools/ipc_fuzzer/OWNERS?

Thanks.

[Tom Sepez, 2013-11-26 18:25:04]

Nice.  I'm fine with being an OWNER.  LGTM.

https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/ipc_fuz...
File tools/ipc_fuzzer/ipc_fuzzer_main.cc (right):

https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/ipc_fuz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:117: break;
nit: do you want to do something more extreme then just breaking here?  Like
fail hard with an error explaining why?

https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/ipc_fuz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:162: IPC::Message* message = new
IPC::Message(*const_message);
nit: might be nice if we didn't have to duplicate messages here.

[Tom Sepez, 2013-11-26 18:28:55]

On 2013/11/26 18:25:04, Tom Sepez wrote:
> Nice.  I'm fine with being an OWNER.  LGTM.
> 
>
https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/ipc_fuz...
> File tools/ipc_fuzzer/ipc_fuzzer_main.cc (right):
> 
>
https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/ipc_fuz...
> tools/ipc_fuzzer/ipc_fuzzer_main.cc:117: break;
> nit: do you want to do something more extreme then just breaking here?  Like
> fail hard with an error explaining why?
> 
>
https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/ipc_fuz...
> tools/ipc_fuzzer/ipc_fuzzer_main.cc:162: IPC::Message* message = new
> IPC::Message(*const_message);
> nit: might be nice if we didn't have to duplicate messages here.

One other suggestion: you might split out the code that reads/writes messages
to/from message files into a separate library, since whatever is doing the
fuzzing is going to need this to perform file to file manipulations.

[Tom Sepez, 2013-11-26 21:30:43]

https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/ipc_fuz...
File tools/ipc_fuzzer/ipc_fuzzer_main.cc (right):

https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/ipc_fuz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:61: std::vector<scoped_ptr<IPC::Message>>
messages_;
nit: > >

[jochen (gone - plz use gerrit), 2013-11-27 09:19:49]

lgtm with comments addressed

https://codereview.chromium.org/18254010/diff/313001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/18254010/diff/313001/build/common.gypi#newcod...
build/common.gypi:848: 'enable_ipc_fuzzer%': 0,
you don't need to duplicate this here, it's enough to define the
enable_ipc_fuzzer%: 0 below

https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/DEPS
File tools/ipc_fuzzer/DEPS (right):

https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/DEPS#ne...
tools/ipc_fuzzer/DEPS:2: "+chrome",
chrome/common

[aedla, 2013-11-27 19:24:50]

On 2013/11/26 18:28:55, Tom Sepez wrote:
> One other suggestion: you might split out the code that reads/writes messages
> to/from message files into a separate library, since whatever is doing the
> fuzzing is going to need this to perform file to file manipulations.

Yeah, I like that. I'll keep in mind for the next CL that adds message names to
the testcase file.

https://codereview.chromium.org/18254010/diff/313001/build/common.gypi
File build/common.gypi (right):

https://codereview.chromium.org/18254010/diff/313001/build/common.gypi#newcod...
build/common.gypi:848: 'enable_ipc_fuzzer%': 0,
On 2013/11/27 09:19:49, jochen wrote:
> you don't need to duplicate this here, it's enough to define the
> enable_ipc_fuzzer%: 0 below

Done.

https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/DEPS
File tools/ipc_fuzzer/DEPS (right):

https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/DEPS#ne...
tools/ipc_fuzzer/DEPS:2: "+chrome",
On 2013/11/27 09:19:49, jochen wrote:
> chrome/common

Done.

https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/ipc_fuz...
File tools/ipc_fuzzer/ipc_fuzzer_main.cc (right):

https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/ipc_fuz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:61: std::vector<scoped_ptr<IPC::Message>>
messages_;
On 2013/11/26 21:30:44, Tom Sepez wrote:
> nit: > >

Moved to std::list<IPC::Message*>.

https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/ipc_fuz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:117: break;
On 2013/11/26 18:25:05, Tom Sepez wrote:
> nit: do you want to do something more extreme then just breaking here?  Like
> fail hard with an error explaining why?

Yeah, added an error message here. It previously failed hard below because data
< end, but the message was off.

https://codereview.chromium.org/18254010/diff/313001/tools/ipc_fuzzer/ipc_fuz...
tools/ipc_fuzzer/ipc_fuzzer_main.cc:162: IPC::Message* message = new
IPC::Message(*const_message);
On 2013/11/26 18:25:05, Tom Sepez wrote:
> nit: might be nice if we didn't have to duplicate messages here.

Done.

[commit-bot: I haz the power, 2013-11-28 13:16:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aedla@chromium.org/18254010/333001

[commit-bot: I haz the power, 2013-11-28 16:05:17]

Change committed as 237795