IPC fuzzer: create message_lib library.

IPC fuzzer: create message_lib library.

Currently it can:
 - read / write IPC message files
 - translate message type <-> name

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

[aedla, 2013-12-04 18:28:26]

tsepez@, could you please take a look?

Quite a big CL. I broke ipc_fuzzer_main.cc into replay.cc + replay_process.cc +
replay_process.h. Shall I do this in another CL, so that this CL can show the
diff of ipc_fuzzer_main.cc?

[Tom Sepez, 2013-12-04 18:47:24]

> Quite a big CL. I broke ipc_fuzzer_main.cc into replay.cc + replay_process.cc
+
> replay_process.h. Shall I do this in another CL, so that this CL can show the
> diff of ipc_fuzzer_main.cc?

Yes, please.  Refactor first in one CL and then make changes on top of it.

[aedla, 2013-12-06 18:01:15]

On 2013/12/04 18:47:24, Tom Sepez wrote:
> > Quite a big CL. I broke ipc_fuzzer_main.cc into replay.cc +
replay_process.cc
> +
> > replay_process.h. Shall I do this in another CL, so that this CL can show
the
> > diff of ipc_fuzzer_main.cc?
> 
> Yes, please.  Refactor first in one CL and then make changes on top of it.

Okay, a bit more sane. PTAL.

[Tom Sepez, 2013-12-06 18:57:15]

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
File tools/ipc_fuzzer/message_lib/message_file_format.h (right):

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_format.h:9: 
You'll want a comment here describing the format of this file.  ASCII art?

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_format.h:13: uint32 message_count;
I'd suggests having a magic number here just to be sure what you're opening is a
message file.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_format.h:15: };
Hard to detect corruption if the counts are wrong ...

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
File tools/ipc_fuzzer/message_lib/message_file_reader.cc (right):

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_reader.cc:105: 
nit: (micro-nit, actually) no blank line here.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_reader.cc:113: if (!name_count)
I'd consider this an error, since all the fixups below will fail.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_reader.cc:149: for (size_t i = 0; i <
messages_->size(); ++i) {
Probably a comment about why this is necessary, and that it is only a partial
fix since although any trivial edit causes the IDs float around for the same
message, there's no guarantee that someone hasn't changed the arguments or
structure layouts.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_reader.cc:154: return false;
You can't bail here.  Someone might have removed the message, in which case,
you'll want to simply delete the message from the vector. Otherwise, your corpus
will rapidly become stale and flunk this test. Still, you'll want a warning.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
File tools/ipc_fuzzer/message_lib/message_file_writer.cc (right):

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_writer.cc:94: return false;
Symptomatic of a missing message file in all_messages.  This may happen from
time to time as folks add messages.  Do you want to just warn and drop the
message to continue working in face of this?

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
File tools/ipc_fuzzer/message_lib/message_names.cc (right):

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_names.cc:19: MessageNames*
MessageNames::all_names_;
Explicit init to NULL
nit: prior line comment as // static

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_names.cc:27: MessageNames&
MessageNames::Get() {
nit: prior line comment as // static

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
File tools/ipc_fuzzer/message_lib/message_names.h (right):

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_names.h:18: static MessageNames& Get();
nit: Usually called GetInstance() in chromium code base, and usually returns
Type* rather than Type&.

[aedla, 2013-12-09 18:08:32]

Thanks for the comments. PTAL.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
File tools/ipc_fuzzer/message_lib/message_file_format.h (right):

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_format.h:9: 
On 2013/12/06 18:57:16, Tom Sepez wrote:
> You'll want a comment here describing the format of this file.  ASCII art?

Done.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_format.h:13: uint32 message_count;
On 2013/12/06 18:57:16, Tom Sepez wrote:
> I'd suggests having a magic number here just to be sure what you're opening is
a
> message file.

Added magic and version.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_format.h:15: };
On 2013/12/06 18:57:16, Tom Sepez wrote:
> Hard to detect corruption if the counts are wrong ...

I think it's unlikely that the magic is correct and counts are wrong. If it
happens, I would think that further parsing will likely fail.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
File tools/ipc_fuzzer/message_lib/message_file_reader.cc (right):

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_reader.cc:105: 
On 2013/12/06 18:57:16, Tom Sepez wrote:
> nit: (micro-nit, actually) no blank line here.

Done.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_reader.cc:113: if (!name_count)
On 2013/12/06 18:57:16, Tom Sepez wrote:
> I'd consider this an error, since all the fixups below will fail.

I'd like to support empty testcases, which have message_count == name_count ==
0.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_reader.cc:149: for (size_t i = 0; i <
messages_->size(); ++i) {
On 2013/12/06 18:57:16, Tom Sepez wrote:
> Probably a comment about why this is necessary, and that it is only a partial
> fix since although any trivial edit causes the IDs float around for the same
> message, there's no guarantee that someone hasn't changed the arguments or
> structure layouts.

Done.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_reader.cc:154: return false;
On 2013/12/06 18:57:16, Tom Sepez wrote:
> You can't bail here.  Someone might have removed the message, in which case,
> you'll want to simply delete the message from the vector. Otherwise, your
corpus
> will rapidly become stale and flunk this test. Still, you'll want a warning.

Ah, this particular case happens when the file doesn't contain a name for a
message. It shouldn't happen for valid message files. But yeah, I wasn't
handling unknown messages correctly. Fixed that and added sanity checks to
TypeToName and NameToType.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
File tools/ipc_fuzzer/message_lib/message_file_writer.cc (right):

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_file_writer.cc:94: return false;
On 2013/12/06 18:57:16, Tom Sepez wrote:
> Symptomatic of a missing message file in all_messages.  This may happen from
> time to time as folks add messages.  Do you want to just warn and drop the
> message to continue working in face of this?

I actually do this check in the logging code. On failure it prints a warning and
doesn't add the message to the vector.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
File tools/ipc_fuzzer/message_lib/message_names.cc (right):

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_names.cc:19: MessageNames*
MessageNames::all_names_;
On 2013/12/06 18:57:16, Tom Sepez wrote:
> Explicit init to NULL
> nit: prior line comment as // static

Done.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_names.cc:27: MessageNames&
MessageNames::Get() {
On 2013/12/06 18:57:16, Tom Sepez wrote:
> nit: prior line comment as // static

Done.

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
File tools/ipc_fuzzer/message_lib/message_names.h (right):

https://codereview.chromium.org/105083002/diff/40001/tools/ipc_fuzzer/message...
tools/ipc_fuzzer/message_lib/message_names.h:18: static MessageNames& Get();
On 2013/12/06 18:57:16, Tom Sepez wrote:
> nit: Usually called GetInstance() in chromium code base, and usually returns
> Type* rather than Type&.

Done.

[Tom Sepez, 2013-12-09 18:35:56]

lgtm

[commit-bot: I haz the power, 2013-12-10 08:54:51]

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

[commit-bot: I haz the power, 2013-12-10 09:10:52]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[aedla, 2013-12-10 10:36:43]

jochen@, could you please review the dependencies of message_lib?

message_lib/all_messages.h collects all messages that RenderProcessHost can
receive. For now I use it to generate message type <-> name mapping in
message_names.cc. In the future I would also like to include all_messages.h from
ipc_fuzzer/mutate/ to generate mutators for each message.

Thanks.

[jochen (gone - plz use gerrit), 2013-12-10 10:45:12]

+jam

John, can you look at the include of content/common/all_messages.h in
all_messages.h ?

Generally, including non-public content files from outside of content is a
no-go. For chrome, we added some API entry points to work around this.

Given that the IPC fuzzer is a bit orthogonal, I'm not sure what the answer here
is

[aedla, 2013-12-10 15:27:05]

On 2013/12/10 10:45:12, jochen wrote:
> +jam
> 
> John, can you look at the include of content/common/all_messages.h in
> all_messages.h ?
> 
> Generally, including non-public content files from outside of content is a
> no-go. For chrome, we added some API entry points to work around this.
> 
> Given that the IPC fuzzer is a bit orthogonal, I'm not sure what the answer
here
> is

Looks like ipcfuzz/ipclist got an exception here:
https://codereview.chromium.org/11347012/.

Even with an exception though, I might run into further problems while
implementing FuzzTraits.

[jam, 2013-12-10 15:53:51]

On 2013/12/10 10:45:12, jochen wrote:
> +jam
> 
> John, can you look at the include of content/common/all_messages.h in
> all_messages.h ?
> 
> Generally, including non-public content files from outside of content is a
> no-go. For chrome, we added some API entry points to work around this.
> 
> Given that the IPC fuzzer is a bit orthogonal, I'm not sure what the answer
here
> is

this is fine since it's an external debugging tool, it's not really an embedder
of content

[jochen (gone - plz use gerrit), 2013-12-10 15:56:51]

lgtm

[commit-bot: I haz the power, 2013-12-10 15:58:06]

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

[commit-bot: I haz the power, 2013-12-10 20:02:15]

Change committed as 239831