1312323 - Single encoded value (either prefix or removal index) is not handled well

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[Henry Chang [:hchang]]

According to [1], if there is only one value encoded, num_entries will be zero and rice_parameter will be missing. We need to take this special case into account.

Besides, we'd better bubble the decoding error up so that no invalid update would be attempted to apply.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/chromium/safebrowsing.proto#419

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug 1312323 - Consider the "single encoded value" case and bubble the decoding error up. .

Review commit: https://reviewboard.mozilla.org/r/87926/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/87926/

Comment 2

[Henry Chang [:hchang]]

Comment on attachment 8803775 [details]
Bug 1312323 - Consider the "single encoded value" case and bubble the decoding error up. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87926/diff/1-2/

Comment 3

[Henry Chang [:hchang]]

Comment on attachment 8803775 [details]
Bug 1312323 - Consider the "single encoded value" case and bubble the decoding error up. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87926/diff/2-3/

Comment 4

[Henry Chang [:hchang]]

BTW, this bug is the root cause of Bug 1309770:

"When any of the updates contains "single encoded value" (either addition or removal),
the update would fail because the single-value addition or removal will not be applied."

And the reason we bubble the decoding error up (changes in ProtocolParserProtobuf::ProcessOneResponse)
is to prevent the junk TableUpdate from being applied.

Note that the usage of RiceDeltaDecoder is slightly modified to accommodate the "single value"
case and make the code simpler and cleaner.

Comment 5

[Henry Chang [:hchang]]

Attachment: safebrowsing-updatewreck.zip

The attachment is an instance which contains a "single encoded value" update.

Comment 6

[François Marier [:francois]]

Comment on attachment 8803775 [details]
Bug 1312323 - Consider the "single encoded value" case and bubble the decoding error up. .

https://reviewboard.mozilla.org/r/87926/#review87388

Looks good. I only have a nit for a comment and a suggestion for a stronger check before you land this.

::: toolkit/components/url-classifier/ProtocolParser.cpp
(Diff revision 3)
>  DoRiceDeltaDecode(const RiceDeltaEncoding& aEncoding,
>                    nsTArray<uint32_t>& aDecoded)
>  {
> -  // Sanity check of the encoding info.
> +  if (!aEncoding.has_first_value()) {
> -  if (!aEncoding.has_first_value() ||
> -      !aEncoding.has_rice_parameter() ||

Maybe we should expand that check to ensure that either:

1. `!has_num_entries()`, or
2. `has_num_entries() && has_rice_parameters() && has_encoded_data()`

That way we would essentially assert on our understanding of the spec.

::: toolkit/components/url-classifier/tests/gtest/TestProtocolParser.cpp:96
(Diff revision 3)
> +
> +  auto& tus = p->GetTableUpdates();
> +  auto tuv4 = TableUpdate::Cast<TableUpdateV4>(tus[0]);
> +  auto& prefixMap = tuv4->Prefixes();
> +  for (auto iter = prefixMap.Iter(); !iter.Done(); iter.Next()) {
> +    // This prefix map should contain only 4-byte prefixes.

"only a single 4-byte prefix"

Comment 7

[Henry Chang [:hchang]]

Comment on attachment 8803775 [details]
Bug 1312323 - Consider the "single encoded value" case and bubble the decoding error up. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87926/diff/3-4/

Comment 8

[Henry Chang [:hchang]]

Comment on attachment 8803775 [details]
Bug 1312323 - Consider the "single encoded value" case and bubble the decoding error up. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87926/diff/4-5/

Comment 9

[Henry Chang [:hchang]]

Comment on attachment 8803775 [details]
Bug 1312323 - Consider the "single encoded value" case and bubble the decoding error up. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87926/diff/5-6/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d26ac63f1b81
Consider the "single encoded value" case and bubble the decoding error up. r=francois.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d26ac63f1b81