1134885 - ChunkSets should be fallible

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

Underlying issue of https://bugzilla.mozilla.org/show_bug.cgi?id=1134858
is that ChunkSets are not currently fallible. In theory this isn't needed because they're small and the server is trusted.

In practice, *cough*.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

ni dougt for prioritization.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

ChunkSets should be fallible, and it shouldn't even try to allocate insane amount of memory.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

https://tbpl.mozilla.org/?tree=Try&rev=9b145ed462ec

Comment 4

[Gian-Carlo Pascutto [:gcp]]

Attachment: ChunkSets should be fallible

Make ChunkSet use FallibleTArray and add some basic sanity checks.

I think ChunkSets were hit by this because they use a range,
so only a few bytes are needed to make the update huge in terms of RAM. 
The other data like prefixes etc probably needs to push a load of data
from server to client to run into the same problem so it's less of a worry.

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8566857 [details] [diff] [review]
ChunkSets should be fallible

Review of attachment 8566857 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, gcp.

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

We should do something sane here for ranges. The chunkset that was being pushed was an adddel range.

Comment 7

[Gian-Carlo Pascutto [:gcp]]

The patch includes a sanity check on the range.

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

What I mean is, we should be able to handle an adddel range that just tries to wipe out all previous chunks (1-whatever) without allocating a ton of memory.

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Attachment: ChunkSets should be fallible

This ain't going to do much good if the callers don't check the result:
we'll end up with an inconsistent DB which is very bad as well.

Propagate the errors upwards.

I took a look and many of the other arrays are fallible already,
but nothing is checking the return values. Sigh. I'm going to file
bugs against nsTArray because god knows how many other bugs like
this we have.

Comment 10

[Gian-Carlo Pascutto [:gcp]]

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #8)
> What I mean is, we should be able to handle an adddel range that just tries
> to wipe out all previous chunks (1-whatever) without allocating a ton of
> memory.

Changing ChunkSet so it stores ranges efficiently is an XXX in it's header, but it's also a nice programming exercise to get it right. I think it's suitable for a mentored bug.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Filed bug 1135022 for that optimization.

Comment 12

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8567012 [details] [diff] [review]
ChunkSets should be fallible

Review of attachment 8567012 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/url-classifier/HashStore.h
@@ -38,5 @@
>    }
>  
>    // Throughout, uint32_t aChunk refers only to the chunk number. Chunk data is
>    // stored in the Prefix structures.
> -  void NewAddChunk(uint32_t aChunk) { mAddChunks.Set(aChunk); }

yikes :(

::: toolkit/components/url-classifier/ProtocolParser.cpp
@@ +26,5 @@
>  namespace safebrowsing {
>  
>  // Updates will fail if fed chunks larger than this
>  const uint32_t MAX_CHUNK_SIZE = (1024 * 1024);
> +// Updates will fail if the total number of tocuhed chunks is larger than this

touched

Comment 13

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc7ca0aabb24

Comment 14

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7d9e802627f7 for causing a bustage like:

https://treeherder.mozilla.org/logviewer.html#?job_id=6955039&repo=mozilla-inbound

Comment 15

[Gian-Carlo Pascutto [:gcp]]

https://tbpl.mozilla.org/?tree=Try&rev=b35dfde16657

Comment 16

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/964d7a3a8afc

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/964d7a3a8afc