Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2495783003:
Implement support for checking bad IPs aka MatchMalwareIP (Closed)
|
Created:
4 years, 1 month ago by vakh (use Gerrit instead) Modified:
4 years, 1 month ago CC:
chromium-reviews, grt+watch_chromium.org, vakh+watch_chromium.org, woz Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement support for checking bad IPs aka MatchMalwareIP
It works as follows:
1. Get an IP address as string.
2. Convert it to IPV6, if needed.
3. Convert the IPV6 address to packed string.
4. Generate SHA1 hash of the packed string.
5. Append 128 at the end because the server does so.
6. Compare it to the hashes sent by the server.
7. Do not send a full hash request. A local match is sufficient to determine
reputation.
BUG=543161
Committed: https://crrev.com/bb5c3a10199a3ef251b540deb6a53032f59245be
Cr-Commit-Position: refs/heads/master@{#432067}
Patch Set 1 #
Total comments: 11
Patch Set 2 : shess@ review and trying to fix Windows build error #
Total comments: 8
Patch Set 3 : shess@ and nparker@ review #
Total comments: 15
Patch Set 4 : nparker@ review #
Total comments: 2
Patch Set 5 : Add a comma after the last enum value #
Messages
Total messages: 40 (18 generated)
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vakh@chromium.org changed reviewers: + nparker@chromium.org, shess@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_local_database_manager.cc:249: std::set<FullHash> encoded_ips{encoded_ip}; I wonder if this should convert to string, then to FullHash, or just go direct to FullHash. Is there any other plausible use case? https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_local_database_manager.h:209: std::string* encoded_ip); Would this make sense in a util.h/cc of some sort? https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right): https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_local_database_manager_unittest.cc:42: if (stored_sahp.hash_prefix == full_hash.substr(0, prefix_size)) { Could you phrase this in terms of std::string::compare()? Otherwise it feels that there's a fresh temporary which may-or-may-not be optimized away. https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_local_database_manager_unittest.cc:43: store_and_hash_prefixes->push_back(stored_sahp); You should probably clear() things up front to replicate the previous code's assignment.
shess@ review and trying to fix Windows build error
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Thanks for the review. PTAL. https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_local_database_manager.cc:249: std::set<FullHash> encoded_ips{encoded_ip}; On 2016/11/12 23:11:34, Scott Hess wrote: > I wonder if this should convert to string, then to FullHash, or just go direct > to FullHash. Is there any other plausible use case? FullHash is a std::string so it's only one conversion. https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_local_database_manager.h:209: std::string* encoded_ip); On 2016/11/12 23:11:34, Scott Hess wrote: > Would this make sense in a util.h/cc of some sort? I am on the fence here myself. The code in that function is not generic IPV4 string to encoded IPV6 conversion. It adds that extra byte also at the end. That's why I did not put it in the v4_protocol_manager_util.h file -- that file is used across multiple classes. I don't mind moving it into v4_protocol_manager_util if you think it is better suited there. https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... File components/safe_browsing_db/v4_local_database_manager_unittest.cc (right): https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_local_database_manager_unittest.cc:42: if (stored_sahp.hash_prefix == full_hash.substr(0, prefix_size)) { On 2016/11/12 23:11:34, Scott Hess wrote: > Could you phrase this in terms of std::string::compare()? Otherwise it feels > that there's a fresh temporary which may-or-may-not be optimized away. Done. https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_local_database_manager_unittest.cc:43: store_and_hash_prefixes->push_back(stored_sahp); On 2016/11/12 23:11:34, Scott Hess wrote: > You should probably clear() things up front to replicate the previous code's > assignment. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
WRT the FullHash-vs-std::string, mostly I'm just looking for "This should be a string because ..." rather than having it just happen to be different types for no apparent reason :-). https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_local_database_manager.cc:249: std::set<FullHash> encoded_ips{encoded_ip}; On 2016/11/14 19:59:16, vakh (Varun Khaneja) wrote: > On 2016/11/12 23:11:34, Scott Hess wrote: > > I wonder if this should convert to string, then to FullHash, or just go direct > > to FullHash. Is there any other plausible use case? > > FullHash is a std::string so it's only one conversion. I'm not questioning efficiency or anything. It's just that you're introducing a new helper method with a std::string output with only one user who immediately converts it to FullHash. If IPAddressToEncodedIPV6() will only ever be used to create things that will be used as a FullHash, I think it would make sense to just have it create a FullHash directly. https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_local_database_manager.h:209: std::string* encoded_ip); On 2016/11/14 19:59:16, vakh (Varun Khaneja) wrote: > On 2016/11/12 23:11:34, Scott Hess wrote: > > Would this make sense in a util.h/cc of some sort? > > I am on the fence here myself. The code in that function is not generic IPV4 > string to encoded IPV6 conversion. It adds that extra byte also at the end. > That's why I did not put it in the v4_protocol_manager_util.h file -- that file > is used across multiple classes. > I don't mind moving it into v4_protocol_manager_util if you think it is better > suited there. I would make the decision based on whether that encoding is determined by the protocol, or by the database. If it has to be encoded that way to match the data coming from the service, then I think it's logically part of the protocol interface. If it is encoded that way as a convenience for storing it in the database, then I think it's part of the database interface. https://codereview.chromium.org/2495783003/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2495783003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:497: encoded_ip->resize(base::kSHA1Length + 1, '\x00'); Does the char matter? AFAICT everything will be overwritten. https://codereview.chromium.org/2495783003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:499: (*encoded_ip)[20] = static_cast<unsigned char>(128); I think you should consistently use hash.size() instead of base::kSHA1Length and the literal 20. OR you should consistently use base::kSHA1Length in all three places and ignore (or DCHECK) hash.size().
https://codereview.chromium.org/2495783003/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2495783003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:456: bool V4LocalDatabaseManager::HandleCheck(std::unique_ptr<PendingCheck> check, The flow is quite different if !sychronous_respose... How about making it a separate function? HandleCheckSynchronously(..) The duplicated code would be small, but it'd be easier to understand. And then anything that uses a whitelist (or we could call it a "fully-populated store" or "store of full-hashes) would call that function. https://codereview.chromium.org/2495783003/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2495783003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:206: const bool synchronous_response = false); I think this arg is confusing... I've commented in the .cc for a possible soln
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
shess@ and nparker@ review
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2495783003/diff/1/components/safe_browsing_db... components/safe_browsing_db/v4_local_database_manager.cc:249: std::set<FullHash> encoded_ips{encoded_ip}; On 2016/11/14 20:52:10, Scott Hess wrote: > On 2016/11/14 19:59:16, vakh (Varun Khaneja) wrote: > > On 2016/11/12 23:11:34, Scott Hess wrote: > > > I wonder if this should convert to string, then to FullHash, or just go > direct > > > to FullHash. Is there any other plausible use case? > > > > FullHash is a std::string so it's only one conversion. > > I'm not questioning efficiency or anything. It's just that you're introducing a > new helper method with a std::string output with only one user who immediately > converts it to FullHash. If IPAddressToEncodedIPV6() will only ever be used to > create things that will be used as a FullHash, I think it would make sense to > just have it create a FullHash directly. Done. https://codereview.chromium.org/2495783003/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2495783003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:456: bool V4LocalDatabaseManager::HandleCheck(std::unique_ptr<PendingCheck> check, On 2016/11/14 21:09:37, Nathan Parker wrote: > The flow is quite different if !sychronous_respose... How about making it a > separate function? > > HandleCheckSynchronously(..) > > The duplicated code would be small, but it'd be easier to understand. And then > anything that uses a whitelist (or we could call it a "fully-populated store" or > "store of full-hashes) would call that function. Done. https://codereview.chromium.org/2495783003/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2495783003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:206: const bool synchronous_response = false); On 2016/11/14 21:09:37, Nathan Parker wrote: > I think this arg is confusing... I've commented in the .cc for a possible soln Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2495783003/diff/20001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.cc (right): https://codereview.chromium.org/2495783003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:497: encoded_ip->resize(base::kSHA1Length + 1, '\x00'); On 2016/11/14 20:52:10, Scott Hess wrote: > Does the char matter? AFAICT everything will be overwritten. Done. https://codereview.chromium.org/2495783003/diff/20001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.cc:499: (*encoded_ip)[20] = static_cast<unsigned char>(128); On 2016/11/14 20:52:10, Scott Hess wrote: > I think you should consistently use hash.size() instead of base::kSHA1Length and > the literal 20. OR you should consistently use base::kSHA1Length in all three > places and ignore (or DCHECK) hash.size(). Done.
lgtm https://codereview.chromium.org/2495783003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/2495783003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:962: if (!V4ProtocolManagerUtil::GetIPV6AddressFromString(ip_address, &address)) { Is there value in updating this (legacy) code? https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:202: // |synchronous_response| is false, an asynchronous full hash request to Update comments, since |synchronous_response| isn't there anymore https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:208: // ready or if there's no match; false otherwise. Maybe add a comment to say what this is used for: For things where a full-hash check isn't needed https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_protocol_manager_util.cc (right): https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_protocol_manager_util.cc:562: const std::string hash = base::SHA1HashString(packed_ip); Maybe DCHECK that the size is 20, since that's what the server assumes. https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_protocol_manager_util.h (right): https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_protocol_manager_util.h:277: static bool GetIPV6AddressFromString(const std::string& ip_address, Is this used anywhere besides the function below? https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_protocol_manager_util_unittest.cc (right): https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_protocol_manager_util_unittest.cc:250: std::vector<std::tuple<bool, std::string, std::string>> test_cases = { Personally I think this would be simpler just written out w/o the for loop, but either way is fine.
Thanks for the review. https://codereview.chromium.org/2495783003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/2495783003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:962: if (!V4ProtocolManagerUtil::GetIPV6AddressFromString(ip_address, &address)) { On 2016/11/15 00:54:46, Nathan Parker wrote: > Is there value in updating this (legacy) code? Smaller binary size until we rip out PVer3 completely? https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:202: // |synchronous_response| is false, an asynchronous full hash request to On 2016/11/15 00:54:46, Nathan Parker wrote: > Update comments, since |synchronous_response| isn't there anymore Done. https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:208: // ready or if there's no match; false otherwise. On 2016/11/15 00:54:46, Nathan Parker wrote: > Maybe add a comment to say what this is used for: For things where a full-hash > check isn't needed Done. https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_protocol_manager_util.cc (right): https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_protocol_manager_util.cc:562: const std::string hash = base::SHA1HashString(packed_ip); On 2016/11/15 00:54:46, Nathan Parker wrote: > Maybe DCHECK that the size is 20, since that's what the server assumes. Done. https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_protocol_manager_util.h (right): https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_protocol_manager_util.h:277: static bool GetIPV6AddressFromString(const std::string& ip_address, On 2016/11/15 00:54:46, Nathan Parker wrote: > Is this used anywhere besides the function below? safe_browsing_database.cc https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... File components/safe_browsing_db/v4_protocol_manager_util_unittest.cc (right): https://codereview.chromium.org/2495783003/diff/40001/components/safe_browsin... components/safe_browsing_db/v4_protocol_manager_util_unittest.cc:250: std::vector<std::tuple<bool, std::string, std::string>> test_cases = { On 2016/11/15 00:54:46, Nathan Parker wrote: > Personally I think this would be simpler just written out w/o the for loop, but > either way is fine. Acknowledged.
nparker@ review
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2495783003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/2495783003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:962: if (!V4ProtocolManagerUtil::GetIPV6AddressFromString(ip_address, &address)) { On 2016/11/15 01:03:41, vakh (Varun Khaneja) wrote: > On 2016/11/15 00:54:46, Nathan Parker wrote: > > Is there value in updating this (legacy) code? > > Smaller binary size until we rip out PVer3 completely? Do you have binary-size metrics to back that up? I'm not saying it's _not_ smaller. Just that hand-wavy arguments don't carry much weight :-). https://codereview.chromium.org/2495783003/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2495783003/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:104: CHECK_MALWARE_IP = 4 Nit: I'd pop a comma, here, so that when you invariably add another element, you don't have to modify this line.
https://codereview.chromium.org/2495783003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/2495783003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:962: if (!V4ProtocolManagerUtil::GetIPV6AddressFromString(ip_address, &address)) { On 2016/11/15 01:21:10, Scott Hess wrote: > On 2016/11/15 01:03:41, vakh (Varun Khaneja) wrote: > > On 2016/11/15 00:54:46, Nathan Parker wrote: > > > Is there value in updating this (legacy) code? > > > > Smaller binary size until we rip out PVer3 completely? > > Do you have binary-size metrics to back that up? > No, but if we leave the same code in two places, isn't it going to be compiled into the binary twice? Please do correct me if I am wrong here. A smart compiler may realize that the code is exactly the same and try to optimize it, but why rely on the compiler when you can easily change that code and make it more readable in the process? > I'm not saying it's _not_ smaller. Just that hand-wavy arguments don't carry > much weight :-). I seriously didn't realize that this could be considered hand-wavy. Repeated code == higher binary size, in my mind.
https://codereview.chromium.org/2495783003/diff/40001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/safe_browsing_database.cc (right): https://codereview.chromium.org/2495783003/diff/40001/chrome/browser/safe_bro... chrome/browser/safe_browsing/safe_browsing_database.cc:962: if (!V4ProtocolManagerUtil::GetIPV6AddressFromString(ip_address, &address)) { On 2016/11/15 01:27:51, vakh (Varun Khaneja) wrote: > On 2016/11/15 01:21:10, Scott Hess wrote: > > On 2016/11/15 01:03:41, vakh (Varun Khaneja) wrote: > > > On 2016/11/15 00:54:46, Nathan Parker wrote: > > > > Is there value in updating this (legacy) code? > > > > > > Smaller binary size until we rip out PVer3 completely? > > > > Do you have binary-size metrics to back that up? > > > > No, but if we leave the same code in two places, isn't it going to be compiled > into the binary twice? Please do correct me if I am wrong here. > > A smart compiler may realize that the code is exactly the same and try to > optimize it, but why rely on the compiler when you can easily change that code > and make it more readable in the process? > > > I'm not saying it's _not_ smaller. Just that hand-wavy arguments don't carry > > much weight :-). > > I seriously didn't realize that this could be considered hand-wavy. Repeated > code == higher binary size, in my mind. For small functions, it depends on if the various code related to framing and aligning the new function outweighs the savings in calling the new function. Which in turn is dependent on things like inlining, which in turn depends on whether your compiler is crazy smart or not. PROBABLY since the functions being called are not available in ip_address.h, extracting the shared bit like this will be a savings. But I wouldn't bet more than like $1 on it, as a lark, because AFAICT the compiler specializes in destroying my assumptions.
Add a comma after the last enum value
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by vakh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org, shess@chromium.org Link to the patchset: https://codereview.chromium.org/2495783003/#ps80001 (title: "Add a comma after the last enum value")
https://codereview.chromium.org/2495783003/diff/60001/components/safe_browsin... File components/safe_browsing_db/v4_local_database_manager.h (right): https://codereview.chromium.org/2495783003/diff/60001/components/safe_browsin... components/safe_browsing_db/v4_local_database_manager.h:104: CHECK_MALWARE_IP = 4 On 2016/11/15 01:21:10, Scott Hess wrote: > Nit: I'd pop a comma, here, so that when you invariably add another element, you > don't have to modify this line. Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/11/15 01:34:24, Scott Hess wrote: > https://codereview.chromium.org/2495783003/diff/40001/chrome/browser/safe_bro... > File chrome/browser/safe_browsing/safe_browsing_database.cc (right): > > https://codereview.chromium.org/2495783003/diff/40001/chrome/browser/safe_bro... > chrome/browser/safe_browsing/safe_browsing_database.cc:962: if > (!V4ProtocolManagerUtil::GetIPV6AddressFromString(ip_address, &address)) { > On 2016/11/15 01:27:51, vakh (Varun Khaneja) wrote: > > On 2016/11/15 01:21:10, Scott Hess wrote: > > > On 2016/11/15 01:03:41, vakh (Varun Khaneja) wrote: > > > > On 2016/11/15 00:54:46, Nathan Parker wrote: > > > > > Is there value in updating this (legacy) code? > > > > > > > > Smaller binary size until we rip out PVer3 completely? > > > > > > Do you have binary-size metrics to back that up? > > > > > > > No, but if we leave the same code in two places, isn't it going to be compiled > > into the binary twice? Please do correct me if I am wrong here. > > > > A smart compiler may realize that the code is exactly the same and try to > > optimize it, but why rely on the compiler when you can easily change that code > > and make it more readable in the process? > > > > > I'm not saying it's _not_ smaller. Just that hand-wavy arguments don't > carry > > > much weight :-). > > > > I seriously didn't realize that this could be considered hand-wavy. Repeated > > code == higher binary size, in my mind. > > For small functions, it depends on if the various code related to framing and > aligning the new function outweighs the savings in calling the new function. > Which in turn is dependent on things like inlining, which in turn depends on > whether your compiler is crazy smart or not. > > PROBABLY since the functions being called are not available in ip_address.h, > extracting the shared bit like this will be a savings. But I wouldn't bet more > than like $1 on it, as a lark, because AFAICT the compiler specializes in > destroying my assumptions. :) I like to think of it this way: Calling the function instead of inlining it gives the compiler a better chance of keeping the binary size small, and at the same time reduces the maintenance overhead.
<joining-the-fray> While this discussion may be informative to future CLs where the size is significant, I think the cost of this discussion and the potential cost of it breaking legacy code far outweighs the potential benefit from smaller binary size. </joining-the-fray> On Mon, Nov 14, 2016 at 5:46 PM <vakh@chromium.org> wrote: > On 2016/11/15 01:34:24, Scott Hess wrote: > > > > https://codereview.chromium.org/2495783003/diff/40001/chrome/browser/safe_bro... > > File chrome/browser/safe_browsing/safe_browsing_database.cc (right): > > > > > > https://codereview.chromium.org/2495783003/diff/40001/chrome/browser/safe_bro... > > chrome/browser/safe_browsing/safe_browsing_database.cc:962: if > > (!V4ProtocolManagerUtil::GetIPV6AddressFromString(ip_address, &address)) > { > > On 2016/11/15 01:27:51, vakh (Varun Khaneja) wrote: > > > On 2016/11/15 01:21:10, Scott Hess wrote: > > > > On 2016/11/15 01:03:41, vakh (Varun Khaneja) wrote: > > > > > On 2016/11/15 00:54:46, Nathan Parker wrote: > > > > > > Is there value in updating this (legacy) code? > > > > > > > > > > Smaller binary size until we rip out PVer3 completely? > > > > > > > > Do you have binary-size metrics to back that up? > > > > > > > > > > No, but if we leave the same code in two places, isn't it going to be > compiled > > > into the binary twice? Please do correct me if I am wrong here. > > > > > > A smart compiler may realize that the code is exactly the same and try > to > > > optimize it, but why rely on the compiler when you can easily change > that > code > > > and make it more readable in the process? > > > > > > > I'm not saying it's _not_ smaller. Just that hand-wavy arguments > don't > > carry > > > > much weight :-). > > > > > > I seriously didn't realize that this could be considered hand-wavy. > Repeated > > > code == higher binary size, in my mind. > > > > For small functions, it depends on if the various code related to > framing and > > aligning the new function outweighs the savings in calling the new > function. > > Which in turn is dependent on things like inlining, which in turn > depends on > > whether your compiler is crazy smart or not. > > > > PROBABLY since the functions being called are not available in > ip_address.h, > > extracting the shared bit like this will be a savings. But I wouldn't bet > more > > than like $1 on it, as a lark, because AFAICT the compiler specializes in > > destroying my assumptions. > > :) > I like to think of it this way: Calling the function instead of inlining it > gives the compiler a better chance of keeping the binary size small, and > at the > same time reduces the maintenance overhead. > > https://codereview.chromium.org/2495783003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Implement support for checking bad IPs aka MatchMalwareIP It works as follows: 1. Get an IP address as string. 2. Convert it to IPV6, if needed. 3. Convert the IPV6 address to packed string. 4. Generate SHA1 hash of the packed string. 5. Append 128 at the end because the server does so. 6. Compare it to the hashes sent by the server. 7. Do not send a full hash request. A local match is sufficient to determine reputation. BUG=543161 ========== to ========== Implement support for checking bad IPs aka MatchMalwareIP It works as follows: 1. Get an IP address as string. 2. Convert it to IPV6, if needed. 3. Convert the IPV6 address to packed string. 4. Generate SHA1 hash of the packed string. 5. Append 128 at the end because the server does so. 6. Compare it to the hashes sent by the server. 7. Do not send a full hash request. A local match is sufficient to determine reputation. BUG=543161 Committed: https://crrev.com/bb5c3a10199a3ef251b540deb6a53032f59245be Cr-Commit-Position: refs/heads/master@{#432067} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bb5c3a10199a3ef251b540deb6a53032f59245be Cr-Commit-Position: refs/heads/master@{#432067} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
