The incident reporting service now calls VerifyModule.

The incident reporting service now calls VerifyModule.
The modules ntdll.dll, chrome.dll and chrome_elf.dll are checked, their
status (unknown, unmodified, modified) is added to the report, along with
any potentially modified exports.

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

[krstnmnlsn, 2014-08-05 14:38:05]

Aiming to land this after https://codereview.chromium.org/406043003/

Thanks!

[csharp, 2014-08-05 15:46:33]

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/module_integrity_verifier_win.cc (right):

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/module_integrity_verifier_win.cc:17: const wchar_t*
test_dll_names[kTestDllsMaxCount] = {
Same comment about test data (lines 17->28)

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/module_integrity_verifier_win.h (right):

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/module_integrity_verifier_win.h:21: // The max
number of test dlls in |test_dll_names|.
Lines 21->33 seem to be just for tests, can they be moved to a test file?

[grt (UTC plus 2), 2014-08-05 16:04:54]

very cool!

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/environment_data_collection_win.cc (right):

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/environment_data_collection_win.cc:110: for (int i
= 0; modules_to_verify[i] != NULL; ++i) {
i'm included to say int -> size_t here since the size of the array is size_t.
although another approach (which i prefer) is to change the signature to:

void RecordModuleVerificationData(
    const wchar_t* const* modules_to_verify,
    ...) {

and then:
  for (; *modules_to_verify; ++modules_to_verify) {
    // use *modules_to_verify in the body to get the string

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/environment_data_collection_win.cc:113:
safe_browsing::VerifyModule(modules_to_verify[i], &modified_exports);
nit: omit "safe_browsing::"

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/environment_data_collection_win.cc:134:
RecordModuleVerificationData(safe_browsing::modules_to_verify, process);
nit: omit "safe_browsing::"

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/module_integrity_verifier_win.h (right):

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/module_integrity_verifier_win.h:21: // The max
number of test dlls in |test_dll_names|.
On 2014/08/05 15:46:33, csharp wrote:
> Lines 21->33 seem to be just for tests, can they be moved to a test file?

yes, please move as much of this as possible into the _unittest.cc file(s) or a
module_integrity_unittest_util file(s) or something.

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/module_integrity_verifier_win.h:22: const int
kTestDllsMaxCount = 5;
i think this can be safely removed. there are ways to make the test not need a
value like this at compile time

https://codereview.chromium.org/440753002/diff/80001/chrome/common/safe_brows...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/440753002/diff/80001/chrome/common/safe_brows...
chrome/common/safe_browsing/csd.proto:416: optional uint32 modified_state = 2;
this is really an enum, right? could you make an enum here that mirrors the
values in the code? beware that 0 must be "UNKNOWN", so you may need a function
to map code values to proto values.

[krstnmnlsn, 2014-08-05 23:07:26]

Thanks chris and greg for all the feedback so far!

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/environment_data_collection_win.cc (right):

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/environment_data_collection_win.cc:110: for (int i
= 0; modules_to_verify[i] != NULL; ++i) {
On 2014/08/05 16:04:54, grt (no reviews Aug 9 - 27) wrote:
> i'm included to say int -> size_t here since the size of the array is size_t.
> although another approach (which i prefer) is to change the signature to:
> 
> void RecordModuleVerificationData(
>     const wchar_t* const* modules_to_verify,
>     ...) {
> 
> and then:
>   for (; *modules_to_verify; ++modules_to_verify) {
>     // use *modules_to_verify in the body to get the string

That is super neat.

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/environment_data_collection_win.cc:113:
safe_browsing::VerifyModule(modules_to_verify[i], &modified_exports);
On 2014/08/05 16:04:54, grt (no reviews Aug 9 - 27) wrote:
> nit: omit "safe_browsing::"

Done.

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/environment_data_collection_win.cc:134:
RecordModuleVerificationData(safe_browsing::modules_to_verify, process);
On 2014/08/05 16:04:54, grt (no reviews Aug 9 - 27) wrote:
> nit: omit "safe_browsing::"

Done.

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/module_integrity_verifier_win.cc (right):

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/module_integrity_verifier_win.cc:17: const wchar_t*
test_dll_names[kTestDllsMaxCount] = {
On 2014/08/05 15:46:33, csharp wrote:
> Same comment about test data (lines 17->28)

Done.

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/module_integrity_verifier_win.h (right):

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/module_integrity_verifier_win.h:21: // The max
number of test dlls in |test_dll_names|.
On 2014/08/05 16:04:54, grt (no reviews Aug 9 - 27) wrote:
> On 2014/08/05 15:46:33, csharp wrote:
> > Lines 21->33 seem to be just for tests, can they be moved to a test file?
> 
> yes, please move as much of this as possible into the _unittest.cc file(s) or
a
> module_integrity_unittest_util file(s) or something.

Okay cool.  _util made.

https://codereview.chromium.org/440753002/diff/80001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/module_integrity_verifier_win.h:22: const int
kTestDllsMaxCount = 5;
On 2014/08/05 16:04:54, grt (no reviews Aug 9 - 27) wrote:
> i think this can be safely removed. there are ways to make the test not need a
> value like this at compile time

Done.

https://codereview.chromium.org/440753002/diff/80001/chrome/common/safe_brows...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/440753002/diff/80001/chrome/common/safe_brows...
chrome/common/safe_browsing/csd.proto:416: optional uint32 modified_state = 2;
On 2014/08/05 16:04:54, grt (no reviews Aug 9 - 27) wrote:
> this is really an enum, right? could you make an enum here that mirrors the
> values in the code? beware that 0 must be "UNKNOWN", so you may need a
function
> to map code values to proto values.

I didn't notice we could do enums, that's great.  
The first/0 value of my enum is "MODULE_STATE_UNKNOWN", returned when
VerifyModule has to return early (the module didn't exist, or the headers were
malformed etc.), so would it be reasonable to let that translate into this
"UNKNOWN" value?

[grt (UTC plus 2), 2014-08-06 01:28:11]

looking great

https://codereview.chromium.org/440753002/diff/80001/chrome/common/safe_brows...
File chrome/common/safe_browsing/csd.proto (right):

https://codereview.chromium.org/440753002/diff/80001/chrome/common/safe_brows...
chrome/common/safe_browsing/csd.proto:416: optional uint32 modified_state = 2;
On 2014/08/05 23:07:25, krstnmnlsn wrote:
> On 2014/08/05 16:04:54, grt (no reviews Aug 9 - 27) wrote:
> > this is really an enum, right? could you make an enum here that mirrors the
> > values in the code? beware that 0 must be "UNKNOWN", so you may need a
> function
> > to map code values to proto values.
> 
> I didn't notice we could do enums, that's great.  
> The first/0 value of my enum is "MODULE_STATE_UNKNOWN", returned when
> VerifyModule has to return early (the module didn't exist, or the headers were
> malformed etc.), so would it be reasonable to let that translate into this
> "UNKNOWN" value?

It's best not to. When new values are added to the enum, old clients will see
the field as unset and the getter will return the first-declared value. So the
first value really should mean "I have no idea what this value is." You could
offset the C++ enum so that it starts at 1 rather than 0 and add a doc comment
to it explaining that it maps 1:1 to the protobuf enum.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win.cc (right):

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.cc:133:
RecordModuleVerificationData(modules_to_verify, process);
nit: Collect rather than Record. "LspFeature" is an exception since it modifies
existing data in the pb.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.cc:133:
RecordModuleVerificationData(modules_to_verify, process);
i think it makes sense to put:
const wchar_t* const kModulesToVerify[] = {
  ...
};
in the unnamed namespace in this file, and pass kModulesToVerify to
RecordModuleVerificationData here.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win.h (right):

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.h:22: // along with
any modified exports, in |process|.
please add that |modules_to_verify| must be terminated with a NULL pointer.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/module_integrity_unittest_util.cc (right):

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util.cc:7: #include
<stddef.h>
unused

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util.cc:11: const
wchar_t* test_dll_names[3] = {
[3] -> []

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util.cc:17: int
TestDllsCount() {
size_t kTestDllNamesCount = arraysize(kTestDllNames) - 1;
(and #include "base/macros.h")

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/module_integrity_unittest_util.h (right):

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util.h:13: const wchar_t*
test_dll_names[];
extern const wchar_t* const kTestDllNames[];

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util.h:16: int
TestDllsCount();
extern size_t kTestDllNamesCount;

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util.h:19: const char
kTestExportName[] = "DummyExport";
extern const char kTestExportName[];
and put the definition in the .cc file.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/module_integrity_verifier_win.h (right):

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_verifier_win.h:23: const int
kModulesToVerifyMaxCount = 5;
remove

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_verifier_win.h:26: const wchar_t*
modules_to_verify[];
move this to the unnamed namespace of environment_data_collection_win.cc as:
const wchar_t* const kModulesToVerify[] = {
  ...
};

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_verifier_win.h:29: int
ModulesToVerifyCount();
remove

https://codereview.chromium.org/440753002/diff/100001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/440753002/diff/100001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:1268:
'browser/safe_browsing/module_integrity_unittest_util.cc',
_win at the end of these so that they automagically only build for windows.

[robertshield, 2014-08-06 13:00:51]

couple of minor drive by nits. Looks great :-)

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win.h (right):

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.h:23: void
RecordModuleVerificationData(
Call me paranoid, but I greatly prefer exposed functions that take an array of
things to also be explicitly passed a length for the array. 'Tis a teeny bit
safer on the off chance bad data is ever passed in. arraysize() in base/macros.h
is a handy way for a caller to get the length.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc
(right):

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:204: //
 RecordModuleVerificationData should find the modified exported function
micro nit: one less space between // and R

[krstnmnlsn, 2014-08-06 21:55:12]

Thanks for all the comments guys.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win.cc (right):

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.cc:133:
RecordModuleVerificationData(modules_to_verify, process);
On 2014/08/06 01:28:10, grt (no reviews Aug 9 - 27) wrote:
> i think it makes sense to put:
> const wchar_t* const kModulesToVerify[] = {
>   ...
> };
> in the unnamed namespace in this file, and pass kModulesToVerify to
> RecordModuleVerificationData here.

Done.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.cc:133:
RecordModuleVerificationData(modules_to_verify, process);
On 2014/08/06 01:28:10, grt (no reviews Aug 9 - 27) wrote:
> nit: Collect rather than Record. "LspFeature" is an exception since it
modifies
> existing data in the pb.

Done.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win.h (right):

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.h:23: void
RecordModuleVerificationData(
On 2014/08/06 13:00:51, robertshield wrote:
> Call me paranoid, but I greatly prefer exposed functions that take an array of
> things to also be explicitly passed a length for the array. 'Tis a teeny bit
> safer on the off chance bad data is ever passed in. arraysize() in
base/macros.h
> is a handy way for a caller to get the length. 

Alright, switched back.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc
(right):

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:204: //
 RecordModuleVerificationData should find the modified exported function
On 2014/08/06 13:00:51, robertshield wrote:
> micro nit: one less space between // and R

I'm beginning to see why you can't handle justified text

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/module_integrity_unittest_util.cc (right):

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util.cc:7: #include
<stddef.h>
On 2014/08/06 01:28:10, grt (no reviews Aug 9 - 27) wrote:
> unused

Done.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util.cc:11: const
wchar_t* test_dll_names[3] = {
On 2014/08/06 01:28:10, grt (no reviews Aug 9 - 27) wrote:
> [3] -> []

Done.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util.cc:17: int
TestDllsCount() {
On 2014/08/06 01:28:10, grt (no reviews Aug 9 - 27) wrote:
> size_t kTestDllNamesCount = arraysize(kTestDllNames) - 1;
> (and #include "base/macros.h")

Done.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/module_integrity_unittest_util.h (right):

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util.h:13: const wchar_t*
test_dll_names[];
On 2014/08/06 01:28:10, grt (no reviews Aug 9 - 27) wrote:
> extern const wchar_t* const kTestDllNames[];

Done.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util.h:16: int
TestDllsCount();
On 2014/08/06 01:28:10, grt (no reviews Aug 9 - 27) wrote:
> extern size_t kTestDllNamesCount;

Done.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util.h:19: const char
kTestExportName[] = "DummyExport";
On 2014/08/06 01:28:10, grt (no reviews Aug 9 - 27) wrote:
> extern const char kTestExportName[];
> and put the definition in the .cc file.

Done.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/module_integrity_verifier_win.h (right):

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_verifier_win.h:23: const int
kModulesToVerifyMaxCount = 5;
On 2014/08/06 01:28:10, grt (no reviews Aug 9 - 27) wrote:
> remove

Done.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_verifier_win.h:26: const wchar_t*
modules_to_verify[];
On 2014/08/06 01:28:11, grt (no reviews Aug 9 - 27) wrote:
> move this to the unnamed namespace of environment_data_collection_win.cc as:
> const wchar_t* const kModulesToVerify[] = {
>   ...
> };

Done.

https://codereview.chromium.org/440753002/diff/100001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_verifier_win.h:29: int
ModulesToVerifyCount();
On 2014/08/06 01:28:10, grt (no reviews Aug 9 - 27) wrote:
> remove

Done.

https://codereview.chromium.org/440753002/diff/100001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/440753002/diff/100001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:1268:
'browser/safe_browsing/module_integrity_unittest_util.cc',
On 2014/08/06 01:28:11, grt (no reviews Aug 9 - 27) wrote:
> _win at the end of these so that they automagically only build for windows.

nice!

[csharp, 2014-08-07 17:22:12]

https://codereview.chromium.org/440753002/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc
(right):

https://codereview.chromium.org/440753002/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:209:
for (int i = 0; i < process_report.module_state_size(); ++i) {
Right now the order of the modules returned from CollectModuleVerificationData
is the same order they are listed in test_dlls, I think we should add a check to
ensure that condition holds.

And then you can move lines 212-218 outside of the forloop, and just start the
forloop at position 1 (then no more if statement inside the forloop).

https://codereview.chromium.org/440753002/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc (right):

https://codereview.chromium.org/440753002/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc:17: size_t
kTestDllNamesCount = arraysize(kTestDllNames) - 1;
Nit: Might be worth adding a short comment reminding readers that the -1 here is
because you mark the end of the array with a NULL value, which isn't a name we
want to examine

[krstnmnlsn, 2014-08-07 21:21:22]

Rebased + Chris' changes.

https://codereview.chromium.org/440753002/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc
(right):

https://codereview.chromium.org/440753002/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:209:
for (int i = 0; i < process_report.module_state_size(); ++i) {
On 2014/08/07 17:22:11, csharp wrote:
> Right now the order of the modules returned from CollectModuleVerificationData
> is the same order they are listed in test_dlls, I think we should add a check
to
> ensure that condition holds.
> 
> And then you can move lines 212-218 outside of the forloop, and just start the
> forloop at position 1 (then no more if statement inside the forloop).

Done.

https://codereview.chromium.org/440753002/diff/160001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc (right):

https://codereview.chromium.org/440753002/diff/160001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc:17: size_t
kTestDllNamesCount = arraysize(kTestDllNames) - 1;
On 2014/08/07 17:22:12, csharp wrote:
> Nit: Might be worth adding a short comment reminding readers that the -1 here
is
> because you mark the end of the array with a NULL value, which isn't a name we
> want to examine

Done.

[csharp, 2014-08-07 21:32:56]

lgtm

[grt (UTC plus 2), 2014-08-08 01:08:52]

https://codereview.chromium.org/440753002/diff/240001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win.cc (right):

https://codereview.chromium.org/440753002/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.cc:31: NULL,
remove NULL

https://codereview.chromium.org/440753002/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.cc:120: int
modified = VerifyModule(modules_to_verify[i], &modified_exports);
should this just continue when this returns MODULE_STATE_UNMODIFIED, or do we
get useful information in this case?

https://codereview.chromium.org/440753002/diff/240001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc (right):

https://codereview.chromium.org/440753002/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc:14: NULL,
remove NULL

https://codereview.chromium.org/440753002/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc:17: // Ignore
the last value when iterating over names because |kTestDllNames| is
don't NULL terminate

https://codereview.chromium.org/440753002/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc:19: size_t
kTestDllNamesCount = arraysize(kTestDllNames) - 1;
remove " - 1"

[krstnmnlsn, 2014-08-08 13:18:08]

Now only modified and 'unknown' modules are sent back.  
Thanks for the feedback greg!

https://codereview.chromium.org/440753002/diff/240001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/environment_data_collection_win.cc (right):

https://codereview.chromium.org/440753002/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.cc:31: NULL,
On 2014/08/08 01:08:51, grt (no reviews Aug 9 - 27) wrote:
> remove NULL

Done.

https://codereview.chromium.org/440753002/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/environment_data_collection_win.cc:120: int
modified = VerifyModule(modules_to_verify[i], &modified_exports);
On 2014/08/08 01:08:51, grt (no reviews Aug 9 - 27) wrote:
> should this just continue when this returns MODULE_STATE_UNMODIFIED, or do we
> get useful information in this case?

We don't need to send back the value no.

https://codereview.chromium.org/440753002/diff/240001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc (right):

https://codereview.chromium.org/440753002/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc:14: NULL,
On 2014/08/08 01:08:51, grt (no reviews Aug 9 - 27) wrote:
> remove NULL

Done.

https://codereview.chromium.org/440753002/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc:17: // Ignore
the last value when iterating over names because |kTestDllNames| is
On 2014/08/08 01:08:51, grt (no reviews Aug 9 - 27) wrote:
> don't NULL terminate

Done.

https://codereview.chromium.org/440753002/diff/240001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc:19: size_t
kTestDllNamesCount = arraysize(kTestDllNames) - 1;
On 2014/08/08 01:08:51, grt (no reviews Aug 9 - 27) wrote:
> remove " - 1"

Done.

[grt (UTC plus 2), 2014-08-08 13:55:00]

LGTM w/ const nit.

https://codereview.chromium.org/440753002/diff/260001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc (right):

https://codereview.chromium.org/440753002/diff/260001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc:16: size_t
kTestDllNamesCount = arraysize(kTestDllNames);
const

https://codereview.chromium.org/440753002/diff/260001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/module_integrity_unittest_util_win.h (right):

https://codereview.chromium.org/440753002/diff/260001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util_win.h:16: extern
size_t kTestDllNamesCount;
const

[noelutz, 2014-08-08 16:26:08]

lgtm

[krstnmnlsn, 2014-08-08 21:22:52]

The CQ bit was checked by krstnmnlsn@chromium.org

[krstnmnlsn, 2014-08-08 21:22:56]

The CQ bit was unchecked by krstnmnlsn@chromium.org

[krstnmnlsn, 2014-08-08 21:23:07]

Final touches!

https://codereview.chromium.org/440753002/diff/260001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc (right):

https://codereview.chromium.org/440753002/diff/260001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util_win.cc:16: size_t
kTestDllNamesCount = arraysize(kTestDllNames);
On 2014/08/08 13:55:00, grt (no reviews Aug 9 - 27) wrote:
> const

Done.

https://codereview.chromium.org/440753002/diff/260001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/module_integrity_unittest_util_win.h (right):

https://codereview.chromium.org/440753002/diff/260001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/module_integrity_unittest_util_win.h:16: extern
size_t kTestDllNamesCount;
On 2014/08/08 13:55:00, grt (no reviews Aug 9 - 27) wrote:
> const

Done.

[krstnmnlsn, 2014-08-08 21:23:19]

The CQ bit was checked by krstnmnlsn@chromium.org

[commit-bot: I haz the power, 2014-08-08 21:26:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/krstnmnlsn@chromium.org/440753002/340001

[commit-bot: I haz the power, 2014-08-09 05:47:57]

Change committed as 288517