Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
323953002:
Support for recording registered LSPs (Closed)
|
Created:
6 years, 6 months ago by pmonette Modified:
6 years, 5 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@grt Visibility:
Public. |
DescriptionInclude loaded modules in safe browsing client incident reports.
This includes the sanitized paths of all loaded modules and an
indication for those that host active LSPs.
BUG=386156
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278655
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=280066
Patch Set 1 #
Total comments: 31
Patch Set 2 : Modifications based on feedback received #
Total comments: 14
Patch Set 3 : Changes to csd.proto to fit protobuf guidelines. Base_address and length now use unit32. #Patch Set 4 : Final rebase #
Total comments: 15
Patch Set 5 : Addressing comments #
Total comments: 6
Patch Set 6 : Addressing more comments #
Total comments: 2
Patch Set 7 : Fixing last nit #Patch Set 8 : Updated trunk and fixed conflics #Patch Set 9 : Fixing the unit test that was failing on WinXP #
Total comments: 3
Patch Set 10 : Converting paths to lowercase instead of doing a case-insensitive comparison #Patch Set 11 : Sync and resolve conflic with trunk #Messages
Total messages: 48 (0 generated)
Please take a look.
looks good https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/environment_data_collection.h (right): https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. this file appears unchanged. are the line-endings different, or something (chromium uses unix-style line endings)? https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/environment_data_collection_unittest.cc (right): https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. it looks like this file is testing the functions in the _win.{cc,h} pair. if this is the case, please rename it to _win_unittest.cc so that it is only included on windows builds. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_unittest.cc:12: namespace safe_browsing { nit: it looks like if you remove the safe_browsing namespace here, lines 45 and 54 wouldn't need to span multiple lines. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/environment_data_collection_win.cc (right): https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:48: safe_browsing::ClientIncidentReport_EnvironmentData_Process* process) { "safe_browsing::" is not needed here https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:54: std::vector<std::wstring> lsp_paths(lsp_list.size()); use base::string16 rather than std::wstring https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:55: for (unsigned int i = 0; i < lsp_list.size(); ++i) { unsigned int -> size_t (use size_t for the size_type of all STL containers) https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:61: lsp_paths.erase(unique(lsp_paths.begin(), lsp_paths.end()), lsp_paths.end()); unique -> std::unique std::unique requires that lsp_paths be sorted. it's not obvious that it is. consider using a std::set for lsp_paths so you don't need to worry about the duplicate removal. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:64: for (unsigned int i = 0; i < lsp_paths.size(); ++i) { unsigned int -> size_t https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:64: for (unsigned int i = 0; i < lsp_paths.size(); ++i) { if you use a std::set for lsp_paths, you can make this O(MlogN) rather than O(MN) by making the outer loop over process->dlls and then check for lsp_paths.count(process->dlls(i).path()). when that's non-zero, you've found a match. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:69: } if there is no match, that means that the LSP was not present in the browser process. do we still want to report those LSPs somehow? perhaps this iteration should remove the LSPs from lsp_paths as they are logged in the protobuf, and then do something interesting with those that are left over. minimally, we could send the count up by UMA so that we know this is happening. let's discuss in person. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:80: wchar_t path_expanded[MAX_PATH + 1] = {0}; nit: {0} -> {} https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:81: ExpandEnvironmentStrings(path.c_str(), path_expanded, MAX_PATH); you need to check the return value here since there are failure modes. also, the paths could be longer than MAX_PATH. i suggest something like this: static const DWORD kMaxBuffer = 32 * 1024; // Max according to MSDN. base::string16 path_expanded; DWORD path_len = MAX_PATH; do { DWORD result = ExpandEnvironmentStrings( path.c_str(), WriteInto(&path_expanded, path_len), path_len); if (!result) { // Failed to expand variables. Return the original string. DPLOG(ERROR) << path; break; } if (result <= path_len) return path_expanded.substr(0, result - 1); path_len = result; } while (path_len < kMaxBuffer); return path; https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/environment_data_collection_win.h (right): https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.h:23: void VerifyLSP(ClientIncidentReport_EnvironmentData_Process* process); how about renaming this to "RecordLspFeatures"? https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.h:25: // Helper function for expanding all environment variables in |path|. move this into the unnamed namespace in the .cc file (and move the #include of string16 into the .cc file, too). https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.h:28: // Helper function that insert basic information about a dll into the i'm not sure that it's worth having this be a public helper. how bad would it be to inline it in the two places where it's used? https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/path_sanitizer.h (right): https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/path_sanitizer.h:22: void StripHomeDirectory(base::string16* file_path_str) const; in chromium, we generally don't overload like this. please distinguish the functions. maybe make this one StripHomeDirectoryFromString
Please update the CL description. The description ends up being the commit message, so it should describe the change (there's no need to refer to the other cl). Also, in the future, you can use "git cl issue NNN" to associate a branch with a rietveld issue. My apologies for not mentioning this yesterday.
So I cleaned up things a bit based on your feedback. Please take another look. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/environment_data_collection.h (right): https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/06/10 13:47:53, grt wrote: > this file appears unchanged. are the line-endings different, or something > (chromium uses unix-style line endings)? I still don't know what happened. My editor is set to unix-style line endings. Fixed by doing a checkout of your version. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/environment_data_collection_unittest.cc (right): https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/06/10 13:47:53, grt wrote: > it looks like this file is testing the functions in the _win.{cc,h} pair. if > this is the case, please rename it to _win_unittest.cc so that it is only > included on windows builds. Done. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_unittest.cc:12: namespace safe_browsing { On 2014/06/10 13:47:53, grt wrote: > nit: it looks like if you remove the safe_browsing namespace here, lines 45 and > 54 wouldn't need to span multiple lines. Done. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/environment_data_collection_win.cc (right): https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:48: safe_browsing::ClientIncidentReport_EnvironmentData_Process* process) { On 2014/06/10 13:47:53, grt wrote: > "safe_browsing::" is not needed here Done. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:54: std::vector<std::wstring> lsp_paths(lsp_list.size()); On 2014/06/10 13:47:53, grt wrote: > use base::string16 rather than std::wstring Done. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:55: for (unsigned int i = 0; i < lsp_list.size(); ++i) { On 2014/06/10 13:47:53, grt wrote: > unsigned int -> size_t (use size_t for the size_type of all STL containers) Done. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:61: lsp_paths.erase(unique(lsp_paths.begin(), lsp_paths.end()), lsp_paths.end()); On 2014/06/10 13:47:53, grt wrote: > unique -> std::unique > std::unique requires that lsp_paths be sorted. it's not obvious that it is. > consider using a std::set for lsp_paths so you don't need to worry about the > duplicate removal. Done. I just went for std::set. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:64: for (unsigned int i = 0; i < lsp_paths.size(); ++i) { On 2014/06/10 13:47:53, grt wrote: > if you use a std::set for lsp_paths, you can make this O(MlogN) rather than > O(MN) by making the outer loop over process->dlls and then check for > lsp_paths.count(process->dlls(i).path()). when that's non-zero, you've found a > match. Yes. My intent was to add a break in the if statement but somehow I forgot. Anyway, using count() makes the code way easier to read. Do you think that lsp_paths.find() != lsp_paths.end() would be even more descriptive? https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:69: } On 2014/06/10 13:47:53, grt wrote: > if there is no match, that means that the LSP was not present in the browser > process. do we still want to report those LSPs somehow? perhaps this iteration > should remove the LSPs from lsp_paths as they are logged in the protobuf, and > then do something interesting with those that are left over. minimally, we could > send the count up by UMA so that we know this is happening. let's discuss in > person. We talked about this. No match is an expected behavior when no tampering has been done. The "leftovers" are just LSPs that are registered in Windows but aren't loaded in Chrome. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:80: wchar_t path_expanded[MAX_PATH + 1] = {0}; On 2014/06/10 13:47:53, grt wrote: > nit: {0} -> {} Done. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.cc:81: ExpandEnvironmentStrings(path.c_str(), path_expanded, MAX_PATH); On 2014/06/10 13:47:53, grt wrote: > you need to check the return value here since there are failure modes. also, the > paths could be longer than MAX_PATH. i suggest something like this: > static const DWORD kMaxBuffer = 32 * 1024; // Max according to MSDN. > base::string16 path_expanded; > DWORD path_len = MAX_PATH; > do { > DWORD result = ExpandEnvironmentStrings( > path.c_str(), WriteInto(&path_expanded, path_len), path_len); > if (!result) { > // Failed to expand variables. Return the original string. > DPLOG(ERROR) << path; > break; > } > if (result <= path_len) > return path_expanded.substr(0, result - 1); > path_len = result; > } while (path_len < kMaxBuffer); > > return path; Done. WriteInto is handy! https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/environment_data_collection_win.h (right): https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.h:23: void VerifyLSP(ClientIncidentReport_EnvironmentData_Process* process); On 2014/06/10 13:47:54, grt wrote: > how about renaming this to "RecordLspFeatures"? Done. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.h:25: // Helper function for expanding all environment variables in |path|. On 2014/06/10 13:47:53, grt wrote: > move this into the unnamed namespace in the .cc file (and move the #include of > string16 into the .cc file, too). Done. https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/environment_data_collection_win.h:28: // Helper function that insert basic information about a dll into the On 2014/06/10 13:47:53, grt wrote: > i'm not sure that it's worth having this be a public helper. how bad would it be > to inline it in the two places where it's used? Sure. I just liked the way my code looked like with this ;) https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... File chrome/browser/safe_browsing/path_sanitizer.h (right): https://codereview.chromium.org/323953002/diff/1/chrome/browser/safe_browsing... chrome/browser/safe_browsing/path_sanitizer.h:22: void StripHomeDirectory(base::string16* file_path_str) const; On 2014/06/10 13:47:54, grt wrote: > in chromium, we generally don't overload like this. please distinguish the > functions. maybe make this one StripHomeDirectoryFromString Done.
looks really good https://codereview.chromium.org/323953002/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/environment_data_collection_win.cc (right): https://codereview.chromium.org/323953002/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/environment_data_collection_win.cc:86: ClientIncidentReport_EnvironmentData_Process_Dll_Feature_LSP); i think it looks a tad nicer to use the in-class constant in this case: ClientIncidentReport_EnvironmentData_Process_Dll::LSP https://codereview.chromium.org/323953002/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc (right): https://codereview.chromium.org/323953002/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:105: ClientIncidentReport_EnvironmentData_Process_Dll_Feature_LSP) ClientIncidentReport_EnvironmentData_Process_Dll::LSP https://codereview.chromium.org/323953002/diff/20001/chrome/common/safe_brows... File chrome/common/safe_browsing/csd.proto (left): https://codereview.chromium.org/323953002/diff/20001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:383: repeated string dlls = 2; rather than change field #2 from a repeated string to a repeated message, please put OBSOLETE_ in front of this field and make your new repeated field #9 (i'm adding 7 and 8 to my CL) and move it down so that it appears after #6. https://codereview.chromium.org/323953002/diff/20001/chrome/common/safe_brows... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/323953002/diff/20001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:389: optional int64 base_address = 2; i checked the underlying file format and confirmed that this is an unsigned 32-bit value even on 64-bit images, so uint32 here as well. thanks. https://codereview.chromium.org/323953002/diff/20001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:390: optional int32 length = 3; uint32 since this is unsigned https://codereview.chromium.org/323953002/diff/20001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:391: repeated Feature features = 4; i just noticed that the protobuf style guide says to use singular form for repeated fields, so please rename this from "features" to "feature". https://codereview.chromium.org/323953002/diff/20001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:393: repeated Dll dlls = 2; dlls -> dll
There you go. Please take another look. https://codereview.chromium.org/323953002/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/environment_data_collection_win.cc (right): https://codereview.chromium.org/323953002/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/environment_data_collection_win.cc:86: ClientIncidentReport_EnvironmentData_Process_Dll_Feature_LSP); On 2014/06/10 20:57:53, grt wrote: > i think it looks a tad nicer to use the in-class constant in this case: > ClientIncidentReport_EnvironmentData_Process_Dll::LSP Done. Cool! Wasn't aware that I could use the enum constant this way with protobuf. https://codereview.chromium.org/323953002/diff/20001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc (right): https://codereview.chromium.org/323953002/diff/20001/chrome/browser/safe_brow... chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:105: ClientIncidentReport_EnvironmentData_Process_Dll_Feature_LSP) On 2014/06/10 20:57:53, grt wrote: > ClientIncidentReport_EnvironmentData_Process_Dll::LSP Done. https://codereview.chromium.org/323953002/diff/20001/chrome/common/safe_brows... File chrome/common/safe_browsing/csd.proto (left): https://codereview.chromium.org/323953002/diff/20001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:383: repeated string dlls = 2; On 2014/06/10 20:57:53, grt wrote: > rather than change field #2 from a repeated string to a repeated message, please > put OBSOLETE_ in front of this field and make your new repeated field #9 (i'm > adding 7 and 8 to my CL) and move it down so that it appears after #6. Done. https://codereview.chromium.org/323953002/diff/20001/chrome/common/safe_brows... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/323953002/diff/20001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:389: optional int64 base_address = 2; On 2014/06/10 20:57:54, grt wrote: > i checked the underlying file format and confirmed that this is an unsigned > 32-bit value even on 64-bit images, so uint32 here as well. thanks. Done. https://codereview.chromium.org/323953002/diff/20001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:390: optional int32 length = 3; On 2014/06/10 20:57:54, grt wrote: > uint32 since this is unsigned Done. https://codereview.chromium.org/323953002/diff/20001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:391: repeated Feature features = 4; On 2014/06/10 20:57:54, grt wrote: > i just noticed that the protobuf style guide says to use singular form for > repeated fields, so please rename this from "features" to "feature". Done. https://codereview.chromium.org/323953002/diff/20001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:393: repeated Dll dlls = 2; On 2014/06/10 20:57:53, grt wrote: > dlls -> dll Done.
lgtm. please update the change description. how about: """ Include loaded modules in safe browsing client incident reports. This includes the sanitized paths of all loaded modules and an indication of which host active LSPs. """
On 2014/06/11 13:33:24, grt wrote: > lgtm. please update the change description. how about: > > """ > Include loaded modules in safe browsing client incident reports. > > This includes the sanitized paths of all loaded modules and an > indication of which host active LSPs. > """ hmm "of which" -> "for those that" ?
I just made a final rebase. Take a look
https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/path_sanitizer.cc (right): https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/path_sanitizer.cc:32: base::FilePath file_path(*file_path_str); looks like you'll either need to make PathSanitizer win-specific, or change this (see trybot failures). string16 != FilePath::StringType outside of windows. https://codereview.chromium.org/323953002/diff/60001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/323953002/diff/60001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:1920: 'browser/safe_browsing/incident_report_upload_result.h', remove this https://codereview.chromium.org/323953002/diff/60001/chrome/common/safe_brows... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/323953002/diff/60001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:410: repeated Dll dll = 9; note to matt: i've asked patrick to use 9 here since i'm adding 7 and 8 in another CL elsewhere.
https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc (right): https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:39: base::FilePath msvdc32_dll(L"msvidc32.dll"); Are these tests portable? It seems like the try run succeeded on win_chromium_x64_rel, which seems weird to me if they reference 32 bit libraries. https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/path_sanitizer_unittest.cc (right): https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/path_sanitizer_unittest.cc:33: FILE_PATH_LITERAL("\\path\\in\\home\\directory\\test.dll")); hm, why is the leading \ on the appended path ok? https://codereview.chromium.org/323953002/diff/60001/chrome/common/safe_brows... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/323953002/diff/60001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:406: optional uint32 base_address = 2; what about 64bit chrome?
https://codereview.chromium.org/323953002/diff/60001/chrome/common/safe_brows... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/323953002/diff/60001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:406: optional uint32 base_address = 2; On 2014/06/17 01:28:18, mattm wrote: > what about 64bit chrome? AddressOfEntryPoint (base_address) and SizeOfCode (length) are both DWORDs in the PE image headers for 32-bit and 64-bit images (see http://msdn.microsoft.com/en-us/gg463119.aspx).
https://codereview.chromium.org/323953002/diff/60001/chrome/common/safe_brows... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/323953002/diff/60001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:406: optional uint32 base_address = 2; On 2014/06/17 10:22:05, grt wrote: > On 2014/06/17 01:28:18, mattm wrote: > > what about 64bit chrome? > > AddressOfEntryPoint (base_address) and SizeOfCode (length) are both DWORDs in > the PE image headers for 32-bit and 64-bit images (see > http://msdn.microsoft.com/en-us/gg463119.aspx). Ah, but now I see that the base_address isn't AddressOfEntryPoint, but is the location in memory. This should be a uint64. Apologies for the misdirection, Patrick.
Thanks for the feedback. Take another look please. https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc (right): https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:39: base::FilePath msvdc32_dll(L"msvidc32.dll"); On 2014/06/17 01:28:18, mattm wrote: > Are these tests portable? It seems like the try run succeeded on > win_chromium_x64_rel, which seems weird to me if they reference 32 bit > libraries. We (Greg and I) investigated this and we found out that msvidc32.dll exists in both 32 bit and 64 bit versions. The name is confusing but the 32 bit version is located in C:\Windows\SysWOW64. The right version is loaded depending on the process. https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/path_sanitizer.cc (right): https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/path_sanitizer.cc:32: base::FilePath file_path(*file_path_str); On 2014/06/16 20:30:38, grt wrote: > looks like you'll either need to make PathSanitizer win-specific, or change this > (see trybot failures). string16 != FilePath::StringType outside of windows. Done. https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/path_sanitizer_unittest.cc (right): https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/path_sanitizer_unittest.cc:33: FILE_PATH_LITERAL("\\path\\in\\home\\directory\\test.dll")); On 2014/06/17 01:28:18, mattm wrote: > hm, why is the leading \ on the appended path ok? Removed. https://codereview.chromium.org/323953002/diff/60001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/323953002/diff/60001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:1920: 'browser/safe_browsing/incident_report_upload_result.h', On 2014/06/16 20:30:38, grt wrote: > remove this Done. https://codereview.chromium.org/323953002/diff/60001/chrome/common/safe_brows... File chrome/common/safe_browsing/csd.proto (right): https://codereview.chromium.org/323953002/diff/60001/chrome/common/safe_brows... chrome/common/safe_browsing/csd.proto:406: optional uint32 base_address = 2; On 2014/06/17 13:10:06, grt wrote: > On 2014/06/17 10:22:05, grt wrote: > > On 2014/06/17 01:28:18, mattm wrote: > > > what about 64bit chrome? > > > > AddressOfEntryPoint (base_address) and SizeOfCode (length) are both DWORDs in > > the PE image headers for 32-bit and 64-bit images (see > > http://msdn.microsoft.com/en-us/gg463119.aspx). > > Ah, but now I see that the base_address isn't AddressOfEntryPoint, but is the > location in memory. This should be a uint64. Apologies for the misdirection, > Patrick. Done.
https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc (right): https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:39: base::FilePath msvdc32_dll(L"msvidc32.dll"); On 2014/06/17 17:48:22, pmonette wrote: > On 2014/06/17 01:28:18, mattm wrote: > > Are these tests portable? It seems like the try run succeeded on > > win_chromium_x64_rel, which seems weird to me if they reference 32 bit > > libraries. > > We (Greg and I) investigated this and we found out that msvidc32.dll exists in > both 32 bit and 64 bit versions. The name is confusing but the 32 bit version is > located in C:\Windows\SysWOW64. The right version is loaded depending on the > process. Ah, thanks. Could you add a comment about it?. https://codereview.chromium.org/323953002/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/path_sanitizer.h (right): https://codereview.chromium.org/323953002/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/path_sanitizer.h:13: } // namespace FilePath*/ dead code https://codereview.chromium.org/323953002/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/path_sanitizer_unittest.cc (right): https://codereview.chromium.org/323953002/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/path_sanitizer_unittest.cc:20: FILE_PATH_LITERAL("C:\\path\\not\\in\\home\\directory\\test.dll")); These tests probably still need some work for non-win platforms
oh, and include BUG number in the description
https://codereview.chromium.org/323953002/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/environment_data_collection_win.cc (right): https://codereview.chromium.org/323953002/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/environment_data_collection_win.cc:58: path_sanitizer.StripHomeDirectoryFromString(&dll_path); how about making dll_path a FilePath here and using the non-FromString variation? i think you could get rid of FromString altogether without much difficulty.
On 2014/06/17 17:56:31, mattm wrote: > oh, and include BUG number in the description I've filed 386156 for you, Patrick. Please add: BUG=386156 to the end of the CL description.
Another round of changes. Can you take another look please? https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc (right): https://codereview.chromium.org/323953002/diff/60001/chrome/browser/safe_brow... chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:39: base::FilePath msvdc32_dll(L"msvidc32.dll"); On 2014/06/17 17:55:51, mattm wrote: > On 2014/06/17 17:48:22, pmonette wrote: > > On 2014/06/17 01:28:18, mattm wrote: > > > Are these tests portable? It seems like the try run succeeded on > > > win_chromium_x64_rel, which seems weird to me if they reference 32 bit > > > libraries. > > > > We (Greg and I) investigated this and we found out that msvidc32.dll exists in > > both 32 bit and 64 bit versions. The name is confusing but the 32 bit version > is > > located in C:\Windows\SysWOW64. The right version is loaded depending on the > > process. > > Ah, thanks. Could you add a comment about it?. Done. https://codereview.chromium.org/323953002/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/environment_data_collection_win.cc (right): https://codereview.chromium.org/323953002/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/environment_data_collection_win.cc:58: path_sanitizer.StripHomeDirectoryFromString(&dll_path); On 2014/06/17 18:05:07, grt wrote: > how about making dll_path a FilePath here and using the non-FromString > variation? i think you could get rid of FromString altogether without much > difficulty. Done. https://codereview.chromium.org/323953002/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/path_sanitizer.h (right): https://codereview.chromium.org/323953002/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/path_sanitizer.h:13: } // namespace FilePath*/ On 2014/06/17 17:55:51, mattm wrote: > dead code Removed https://codereview.chromium.org/323953002/diff/80001/chrome/browser/safe_brow... File chrome/browser/safe_browsing/path_sanitizer_unittest.cc (right): https://codereview.chromium.org/323953002/diff/80001/chrome/browser/safe_brow... chrome/browser/safe_browsing/path_sanitizer_unittest.cc:20: FILE_PATH_LITERAL("C:\\path\\not\\in\\home\\directory\\test.dll")); On 2014/06/17 17:55:52, mattm wrote: > These tests probably still need some work for non-win platforms I made them portable.
lgtm with a nit. please do "git cl try" to run the full battery of tryjobs. thanks. https://codereview.chromium.org/323953002/diff/100001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc (right): https://codereview.chromium.org/323953002/diff/100001/chrome/browser/safe_bro... chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:39: // msvidc32.dll exists in both 32 and 64 bit version. nit: version -> versions
Fixed https://codereview.chromium.org/323953002/diff/100001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc (right): https://codereview.chromium.org/323953002/diff/100001/chrome/browser/safe_bro... chrome/browser/safe_browsing/environment_data_collection_win_unittest.cc:39: // msvidc32.dll exists in both 32 and 64 bit version. On 2014/06/18 15:42:40, grt wrote: > nit: version -> versions Done.
lgtm
Hi Patrick, Please update past r278358 (and resolve the merge conflicts in the .proto file) and re-upload. We'll check the commit box and let it fly.
It is done Greg.
The CQ bit was checked by pmonette@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pmonette@google.com/323953002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
The CQ bit was checked by pmonette@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pmonette@google.com/323953002/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was checked by grt@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pmonette@google.com/323953002/140001
Message was sent while issue was closed.
Change committed as 278655
I've fixed the test that was failing on WinXP. It was a matter of case-sensitive comparison because on XP the %SystemRoot% variable expands to an all uppercase path. I also changed all base::string16 references to std::wstring because that's what base::FilePath uses anyway. Can you take a look?
https://codereview.chromium.org/323953002/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/environment_data_collection_win.cc (right): https://codereview.chromium.org/323953002/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/environment_data_collection_win.cc:84: for (std::set<std::wstring>::const_iterator iter = lsp_paths.begin(); How many entries are in these lists? Is it worth worrying about making this a O(n*m) loop? Could avoid by lowercasing the entries as you put them into the set.
https://codereview.chromium.org/323953002/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/environment_data_collection_win.cc (right): https://codereview.chromium.org/323953002/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/environment_data_collection_win.cc:84: for (std::set<std::wstring>::const_iterator iter = lsp_paths.begin(); On 2014/06/23 22:23:16, mattm wrote: > How many entries are in these lists? Is it worth worrying about making this a > O(n*m) loop? Could avoid by lowercasing the entries as you put them into the > set. I like Matt's suggestion: drop the case on lsp_path in the upper loop when inserting in the set and on dll_path on line 84 and switch back to a straight lookup in the set. base/i18n/case_conversion.h has helpers for operating on string16s (std::wstring and base::string16 are the same datatype on Windows).
Take a look please. https://codereview.chromium.org/323953002/diff/180001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/environment_data_collection_win.cc (right): https://codereview.chromium.org/323953002/diff/180001/chrome/browser/safe_bro... chrome/browser/safe_browsing/environment_data_collection_win.cc:84: for (std::set<std::wstring>::const_iterator iter = lsp_paths.begin(); On 2014/06/24 13:48:15, grt wrote: > On 2014/06/23 22:23:16, mattm wrote: > > How many entries are in these lists? Is it worth worrying about making this a > > O(n*m) loop? Could avoid by lowercasing the entries as you put them into the > > set. > > I like Matt's suggestion: drop the case on lsp_path in the upper loop when > inserting in the set and on dll_path on line 84 and switch back to a straight > lookup in the set. base/i18n/case_conversion.h has helpers for operating on > string16s (std::wstring and base::string16 are the same datatype on Windows). Done. But I figured that doing the case convertion for dll paths in CollectDlls is a better idea and will remove the burden of dealing with case-sensitive paths server-side.
lgtm. please sync to ToT (merge conflict with chrome_browser.gypi), re-upload, and re-run tryjobs. thanks.
lgtm
The CQ bit was checked by pmonette@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pmonette@google.com/323953002/220001
Message was sent while issue was closed.
Change committed as 280066
Message was sent while issue was closed.
Please take a look at my recent modification. RecordLspFeature unit test was failing on some windows try bots where mswsock.dll was already loaded into the process.
On 2014/06/27 15:50:39, pmonette wrote: > Please take a look at my recent modification. > > RecordLspFeature unit test was failing on some windows try bots where > mswsock.dll was already loaded into the process. So I have moved this to another CL since this one has already landed.
On 2014/06/27 15:50:39, pmonette wrote: > Please take a look at my recent modification. > > RecordLspFeature unit test was failing on some windows try bots where > mswsock.dll was already loaded into the process. So I have moved this to another CL since this one has already landed. |
