407759 - support sub chunks that remove adds that haven't arrived yet

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P3)

Description

[Dave Camp (:dcamp)]

In version 2.1 of the safebrowsing protocol, clients are going to need to deal with sub chunks that remove urls that are added by add chunks that haven't arrived yet.  So if a subtracted url doesn't match an add in the database, it needs to be stored and checked against all incoming adds.

What I'm planning to do to support this is add a second table with the same schema as moz_classifier, which will store pending subs.  This table will be updated when a sub can't find a matching add, and will be checked on all new adds.

Comment 1

[Dave Camp (:dcamp)]

Attachment: part 1: rearrange access to moz_classifier

This first patch lays some groundwork for the second patch.  I'm submitting it separately for easier reviewing - this patch shouldn't change actual behavior at all, just refactor it a bit.

This adds an object that mediates access to the moz_classifier table.  The idea is that the pending subs table will add a new nsUrlClassifierStore object to the worker with a new pair of database tables.

Comment 2

[Dave Camp (:dcamp)]

Attachment: part 2: deal with out-of-order subs

... and this patch does the actual work.

There are a few testing-related additions bundled in here too.  resetDatabase() is needed to fit a lot of standalone unit tests in to one test run, and GetChunkEntries() has been modified to pack same-domain fragments into one entry in the same way the server should be doing it (which was needed to test some corner cases related to that).

test_addsub.js is a bit nicer than test_dbservice.js, and I should probably port test_dbservice.js over to use some of the scaffolding that test_addsub.js uses.

Comment 3

[Dave Camp (:dcamp)]

(In reply to comment #1)
> Created an attachment (id=292434) [details]
> This adds an object that mediates access to the moz_classifier table.  The idea
> is that the pending subs table will add a new nsUrlClassifierStore object to
> the worker with a new pair of database tables.

Forgot to mention:  To make the diff easier to read and review, I left some nsUrlClassifierStore methods mixed in with nsUrlClassifierWorker methods.  After part 1 is reviewed and finalized I'll put together a patch to move those methods up next to nsUrlClassifierStore.

Comment 4

[Tony Chang (Google)]

Comment on attachment 292434 [details] [diff] [review]
part 1: rearrange access to moz_classifier

Looks good.

This file is pretty big.  Maybe we should move nsUrlClassifierStore into a separate file.  Your call.

Comment 5

[Tony Chang (Google)]

Comment on attachment 292436 [details] [diff] [review]
part 2: deal with out-of-order subs

>diff -r 03248a31ec68 toolkit/components/url-classifier/public/nsIUrlClassifierDBService.idl
>+  /**
>+   * Reset the malware database.  Mostly intended for use in unit tests.
>+   */
>+  void resetDatabase();

By reset, we mean delete the file/db, so maybe the comment should say that.

>+    if (!thisEntry.IsEmpty()) {
>+      // There are leftover subtracts in this entry.  Save them in the
>+      // pending subtraction store.
>+      existingEntry.Clear();
>+      mPendingSubStore.ReadEntry(thisEntry.mKey, tableId, existingEntry);
>+      existingEntry.Merge(thisEntry);
>+      mPendingSubStore.WriteEntry(existingEntry);

Should there be COM rv checks here?

>diff -r 03248a31ec68 toolkit/components/url-classifier/tests/unit/test_addsub.js
>+function updateError()
>+{
>+  do_throw(arg);
>+}

What's arg?

Comment 6

[Dave Camp (:dcamp)]

Attachment: part 2 v2

Comment 7

[Dave Camp (:dcamp)]

(In reply to comment #4)
> This file is pretty big.  Maybe we should move nsUrlClassifierStore into a
> separate file.  Your call.

I'd prefer to keep this file intact until we've finished some of the other bigger changes planned.  We can figure out the best way to split it up then.

Comment 8

[Dave Camp (:dcamp)]

Checking in public/nsIUrlClassifierDBService.idl;
/cvsroot/mozilla/toolkit/components/url-classifier/public/nsIUrlClassifierDBService.idl,v  <--  nsIUrlClassifierDBService.idl
new revision: 1.13; previous revision: 1.12
done
Checking in src/nsUrlClassifierDBService.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp,v  <--  nsUrlClassifierDBService.cpp
new revision: 1.47; previous revision: 1.46
done
Checking in tests/unit/head_urlclassifier.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/head_urlclassifier.js,v  <--  head_urlclassifier.js
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_addsub.js,v
done
Checking in tests/unit/test_addsub.js;
/cvsroot/mozilla/toolkit/components/url-classifier/tests/unit/test_addsub.js,v  <--  test_addsub.js
initial revision: 1.1
done