Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2009183002:
SafeBrowsing: Implement cache lookup for full hash checks (Closed)
|
Created:
4 years, 7 months ago by kcarattini Modified:
4 years, 6 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSafeBrowsing: Implement cache lookup for full hash checks
BUG=561867, 543161
Committed: https://crrev.com/ddfb2e3842cb671f3cb3dad712f5fe0a1f242f8e
Cr-Commit-Position: refs/heads/master@{#397366}
Patch Set 1 #Patch Set 2 : Cleanup #
Total comments: 18
Patch Set 3 : Review comments #Patch Set 4 : Review comments #
Total comments: 2
Patch Set 5 : Review Comments #Patch Set 6 : Typo #
Dependent Patchsets: Messages
Total messages: 30 (12 generated)
kcarattini@chromium.org changed reviewers: + awoz@google.com, nparker@chromium.org
Alex, please review for cache lookup logic. Thanks, Kendra
Some of this is Pver4 specific, and some is pver4-api-check specific. We should
name them as such, or even pull out the generic pcer4-caching into a separate
caching class. That would make the naming simpler (v4_cache->{nov4}Function..)
but wouldn't change the dependencies.
Varun: The V4LocalDatabaseManager will need to check the local DB before
invoking the logic Kendra has here (check the cache, make a network call). Will
it work to have this code in DatabaseManager? Actually seems like it will.
Kendra: What happens if there are different threat types requested at different
times (in addition to the API-checks), and they happen to hit the same hash
prefix? Will this handle that?
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/database_manager.cc (right):
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:103: std::vector<SBPrefix>
prefixes;
maybe "prefixes_needing_reqs"? Otherwise it looks like the prefixes of all the
full hashes.
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:117:
HandleGetHashesWithApisResults(check, full_hash_results, base::Time());
Shouldn't this be cached_results?
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:138: if (full_hashes.empty())
nit: This is duplicate of that up on line 94
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:144: // The cache operates as
follows:
Nice documentation!
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:179: for (; i <
cache_result.full_hashes.size(); ++i) {
Not sure if this is better, you decide. It's one less variable:
const SBFullHashResult* found_full_hash = nullptr;
for (const SBFullHashResult& hash_result : cache_result.full_hashes) {
if...() {
found_full_hash = &hash_result;
break;
}
if (found_full_hash) {
..
}
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:200: // Case i.
This is a little weird, but I see the point of the symmetry.
Caching logic LGTM https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin... File components/safe_browsing_db/database_manager.cc (right): https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin... components/safe_browsing_db/database_manager.cc:103: std::vector<SBPrefix> prefixes; On 2016/05/26 22:30:55, Nathan Parker wrote: > maybe "prefixes_needing_reqs"? Otherwise it looks like the prefixes of all the > full hashes. +1 https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin... components/safe_browsing_db/database_manager.cc:142: // https://devsite.googleplex.com/safe-browsing/v4/caching#about-caching Can you update this to the public URL? https://developers.google.com/safe-browsing/v4/caching#about-caching https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin... components/safe_browsing_db/database_manager.cc:144: // The cache operates as follows: On 2016/05/26 22:30:55, Nathan Parker wrote: > Nice documentation! +1! https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin... components/safe_browsing_db/database_manager.cc:166: // Individual full hash results can be removed from the prefix' s/prefix'/prefix's
message: On 2016/05/26 22:30:55, Nathan Parker wrote:
> Some of this is Pver4 specific, and some is pver4-api-check specific. We
should
> name them as such, or even pull out the generic pcer4-caching into a separate
> caching class. That would make the naming simpler
(v4_cache->{nov4}Function..)
> but wouldn't change the dependencies.
I've renamed some things to better reflect what's pver4 generic and API
specific.
Seems like it might be overkill to have a new cache class for the 1 method. What
else would go in there? I don't have a great idea of what other changes are
needed for non-api V4 full hash caching.
>
> Varun: The V4LocalDatabaseManager will need to check the local DB before
> invoking the logic Kendra has here (check the cache, make a network call).
Will
> it work to have this code in DatabaseManager? Actually seems like it will.
>
> Kendra: What happens if there are different threat types requested at
different
> times (in addition to the API-checks), and they happen to hit the same hash
> prefix? Will this handle that?
Alex: Will a full hash request with API threat type also return results for
other threat types such that they can use the same cached value?
If so, then seems like it will, since the entire SBFullHashResult is saved in
the cache, not just the api information. GetFullHashCachedResults should be
called before those requests are sent too, and handled appropriately in the
respective callback that processes the results. Since this function is a
protected member of the base class, Local/Remote DB should have access to it.
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
File components/safe_browsing_db/database_manager.cc (right):
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:103: std::vector<SBPrefix>
prefixes;
On 2016/05/27 16:54:36, awoz wrote:
> On 2016/05/26 22:30:55, Nathan Parker wrote:
> > maybe "prefixes_needing_reqs"? Otherwise it looks like the prefixes of all
> the
> > full hashes.
>
> +1
Done.
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:117:
HandleGetHashesWithApisResults(check, full_hash_results, base::Time());
On 2016/05/26 22:30:55, Nathan Parker wrote:
> Shouldn't this be cached_results?
The cached_results are stored in the check (passed as the first argument).
full_hash_results represents the results from the SB server (which are empty
because there was no request). Can I make this more clear somehow? I've updated
the comment.
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:138: if (full_hashes.empty())
On 2016/05/26 22:30:55, Nathan Parker wrote:
> nit: This is duplicate of that up on line 94
Removed.
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:142: //
https://devsite.googleplex.com/safe-browsing/v4/caching#about-caching
On 2016/05/27 16:54:36, awoz wrote:
> Can you update this to the public URL?
> https://developers.google.com/safe-browsing/v4/caching#about-caching
Done.
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:144: // The cache operates as
follows:
On 2016/05/27 16:54:36, awoz wrote:
> On 2016/05/26 22:30:55, Nathan Parker wrote:
> > Nice documentation!
>
> +1!
Thanks!
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:166: // Individual full hash
results can be removed from the prefix'
On 2016/05/27 16:54:36, awoz wrote:
> s/prefix'/prefix's
Done.
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:179: for (; i <
cache_result.full_hashes.size(); ++i) {
On 2016/05/26 22:30:55, Nathan Parker wrote:
> Not sure if this is better, you decide. It's one less variable:
>
>
> const SBFullHashResult* found_full_hash = nullptr;
> for (const SBFullHashResult& hash_result : cache_result.full_hashes) {
> if...() {
> found_full_hash = &hash_result;
> break;
> }
>
> if (found_full_hash) {
> ..
> }
Done.
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
components/safe_browsing_db/database_manager.cc:200: // Case i.
On 2016/05/26 22:30:55, Nathan Parker wrote:
> This is a little weird, but I see the point of the symmetry.
Acknowledged.
On 2016/05/30 05:22:51, kcarattini wrote:
> message: On 2016/05/26 22:30:55, Nathan Parker wrote:
> > Some of this is Pver4 specific, and some is pver4-api-check specific. We
> should
> > name them as such, or even pull out the generic pcer4-caching into a
separate
> > caching class. That would make the naming simpler
> (v4_cache->{nov4}Function..)
> > but wouldn't change the dependencies.
>
> I've renamed some things to better reflect what's pver4 generic and API
> specific.
> Seems like it might be overkill to have a new cache class for the 1 method.
What
> else would go in there? I don't have a great idea of what other changes are
> needed for non-api V4 full hash caching.
>
> >
> > Varun: The V4LocalDatabaseManager will need to check the local DB before
> > invoking the logic Kendra has here (check the cache, make a network call).
> Will
> > it work to have this code in DatabaseManager? Actually seems like it will.
> >
> > Kendra: What happens if there are different threat types requested at
> different
> > times (in addition to the API-checks), and they happen to hit the same hash
> > prefix? Will this handle that?
>
> Alex: Will a full hash request with API threat type also return results for
> other threat types such that they can use the same cached value?
It will only return threat types specified in the original request. We currently
don't expose a way to ask for "all."
>
> If so, then seems like it will, since the entire SBFullHashResult is saved in
> the cache, not just the api information. GetFullHashCachedResults should be
> called before those requests are sent too, and handled appropriately in the
> respective callback that processes the results. Since this function is a
> protected member of the base class, Local/Remote DB should have access to it.
>
>
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
> File components/safe_browsing_db/database_manager.cc (right):
>
>
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
> components/safe_browsing_db/database_manager.cc:103: std::vector<SBPrefix>
> prefixes;
> On 2016/05/27 16:54:36, awoz wrote:
> > On 2016/05/26 22:30:55, Nathan Parker wrote:
> > > maybe "prefixes_needing_reqs"? Otherwise it looks like the prefixes of
all
> > the
> > > full hashes.
> >
> > +1
>
> Done.
>
>
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
> components/safe_browsing_db/database_manager.cc:117:
> HandleGetHashesWithApisResults(check, full_hash_results, base::Time());
> On 2016/05/26 22:30:55, Nathan Parker wrote:
> > Shouldn't this be cached_results?
>
> The cached_results are stored in the check (passed as the first argument).
> full_hash_results represents the results from the SB server (which are empty
> because there was no request). Can I make this more clear somehow? I've
updated
> the comment.
>
>
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
> components/safe_browsing_db/database_manager.cc:138: if (full_hashes.empty())
> On 2016/05/26 22:30:55, Nathan Parker wrote:
> > nit: This is duplicate of that up on line 94
>
> Removed.
>
>
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
> components/safe_browsing_db/database_manager.cc:142: //
> https://devsite.googleplex.com/safe-browsing/v4/caching#about-caching
> On 2016/05/27 16:54:36, awoz wrote:
> > Can you update this to the public URL?
> > https://developers.google.com/safe-browsing/v4/caching#about-caching
>
> Done.
>
>
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
> components/safe_browsing_db/database_manager.cc:144: // The cache operates as
> follows:
> On 2016/05/27 16:54:36, awoz wrote:
> > On 2016/05/26 22:30:55, Nathan Parker wrote:
> > > Nice documentation!
> >
> > +1!
>
> Thanks!
>
>
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
> components/safe_browsing_db/database_manager.cc:166: // Individual full hash
> results can be removed from the prefix'
> On 2016/05/27 16:54:36, awoz wrote:
> > s/prefix'/prefix's
>
> Done.
>
>
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
> components/safe_browsing_db/database_manager.cc:179: for (; i <
> cache_result.full_hashes.size(); ++i) {
> On 2016/05/26 22:30:55, Nathan Parker wrote:
> > Not sure if this is better, you decide. It's one less variable:
> >
> >
> > const SBFullHashResult* found_full_hash = nullptr;
> > for (const SBFullHashResult& hash_result : cache_result.full_hashes) {
> > if...() {
> > found_full_hash = &hash_result;
> > break;
> > }
> >
> > if (found_full_hash) {
> > ..
> > }
>
> Done.
>
>
https://codereview.chromium.org/2009183002/diff/20001/components/safe_browsin...
> components/safe_browsing_db/database_manager.cc:200: // Case i.
> On 2016/05/26 22:30:55, Nathan Parker wrote:
> > This is a little weird, but I see the point of the symmetry.
>
> Acknowledged.
> > > Kendra: What happens if there are different threat types requested at > > different > > > times (in addition to the API-checks), and they happen to hit the same hash > > > prefix? Will this handle that? > > > > Alex: Will a full hash request with API threat type also return results for > > other threat types such that they can use the same cached value? > > It will only return threat types specified in the original request. We currently > don't expose a way to ask for "all." So then I think we need to segment the cached results by the requested threat type. Something like this: typedef std::map<SBPrefix, SBCachedFullHashResult> PrefixToFullHashResultsMap; typedef std::map<ThreatType, PrefixToFullHashResultsMap> ThreatTypeResultsMap; ThreatTypeResultsMap full_hash_cache_; This way, for example, full hash results for API_ABUSE won't affect results for UNWANTED_SOFTWARE. I think the caching doesn't need to be exposed to the db-manager, and could be contained wholly in the V4GetHashProtocolManager (Or a separate class owned by the protocol manager, if it's sufficiently complex).
On 2016/05/31 20:41:57, Nathan Parker wrote: > > > > Kendra: What happens if there are different threat types requested at > > > different > > > > times (in addition to the API-checks), and they happen to hit the same > hash > > > > prefix? Will this handle that? > > > > > > Alex: Will a full hash request with API threat type also return results for > > > other threat types such that they can use the same cached value? > > > > It will only return threat types specified in the original request. We > currently > > don't expose a way to ask for "all." > > > So then I think we need to segment the cached results by the requested threat > type. Something like this: > > typedef std::map<SBPrefix, SBCachedFullHashResult> PrefixToFullHashResultsMap; > typedef std::map<ThreatType, PrefixToFullHashResultsMap> ThreatTypeResultsMap; > > ThreatTypeResultsMap full_hash_cache_; > > This way, for example, full hash results for API_ABUSE won't affect results for > UNWANTED_SOFTWARE. > > I think the caching doesn't need to be exposed to the db-manager, and could be > contained wholly in the V4GetHashProtocolManager (Or a separate class owned by > the protocol manager, if it's sufficiently complex). I've updated the cache to track by SBThreatType and added a TODO to look into moving this to protocol manager. PTAL.
lgtm Can you file a bug to remember about moving the caching out of database manager? Thanks. https://codereview.chromium.org/2009183002/diff/60001/components/safe_browsin... File components/safe_browsing_db/database_manager_unittest.cc (right): https://codereview.chromium.org/2009183002/diff/60001/components/safe_browsin... components/safe_browsing_db/database_manager_unittest.cc:65: base::TimeDelta::FromMinutes(negative_cache_duration_mins) : now; Does this ternary conditional do anything? If the argument is zero, you can just add zero.
Description was changed from ========== SafeBrowsing: Implement cache lookup for full hash checks BUG=561867 ========== to ========== SafeBrowsing: Implement cache lookup for full hash checks BUG=561867,543161 ==========
Bug Filed: https://bugs.chromium.org/p/chromium/issues/detail?id=616647 Thanks! https://codereview.chromium.org/2009183002/diff/60001/components/safe_browsin... File components/safe_browsing_db/database_manager_unittest.cc (right): https://codereview.chromium.org/2009183002/diff/60001/components/safe_browsin... components/safe_browsing_db/database_manager_unittest.cc:65: base::TimeDelta::FromMinutes(negative_cache_duration_mins) : now; On 2016/06/01 17:56:01, Nathan Parker wrote: > Does this ternary conditional do anything? If the argument is zero, you can > just add zero. This was my reaction to a comment in time.h that said "Adding or subtracting the maximum time delta to a time or another time delta has an undefined result." Here we have a Time object, as opposed to TimeDelta, and it's not clear what adding TimeDeltas to it does, so I was just trying to avoid undefined behaviour. I changed it up and added a comment to make this more clear.
The CQ bit was checked by kcarattini@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from awoz@google.com, nparker@chromium.org Link to the patchset: https://codereview.chromium.org/2009183002/#ps100001 (title: "Typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2009183002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2009183002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by kcarattini@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2009183002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2009183002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kcarattini@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2009183002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2009183002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kcarattini@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2009183002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2009183002/100001
Message was sent while issue was closed.
Description was changed from ========== SafeBrowsing: Implement cache lookup for full hash checks BUG=561867,543161 ========== to ========== SafeBrowsing: Implement cache lookup for full hash checks BUG=561867,543161 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== SafeBrowsing: Implement cache lookup for full hash checks BUG=561867,543161 ========== to ========== SafeBrowsing: Implement cache lookup for full hash checks BUG=561867,543161 Committed: https://crrev.com/ddfb2e3842cb671f3cb3dad712f5fe0a1f242f8e Cr-Commit-Position: refs/heads/master@{#397366} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/ddfb2e3842cb671f3cb3dad712f5fe0a1f242f8e Cr-Commit-Position: refs/heads/master@{#397366} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
