Closed Bug 1188235 Opened 9 years ago Closed 9 years ago

[EME] Make GMPStorage insusceptible to hash collisions

Categories

(Core :: Audio/Video: Playback, defect, P1)

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox41 --- fixed
firefox42 --- fixed
firefox43 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Patch v2: Make GMPStorage immune to record name hash collisions
27.74 KB, patch
mozbugz
: review+
ritu
: approval-mozilla-aurora+
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review
Interdiff of patch v1 to v2
9.87 KB, patch
mozbugz
: feedback+
Details | Diff | Splinter Review 
GMPStorage stores records in a file which has name as the hash of the record name. This can suffer from hash collisions. We may be seeing this when GMPStorage is running at scale; Netflix has detected low rates of GMPStorage write failures in the field.
* I changed the way we choose filenames in which to store records, so that we resolve hash collisions by adding 1 to the hash code until we find an unused file. This means we can keep the existing record file format; so existing records in the field won't be invalid.
* Change way we read/write GMP disk storage files, so that the record name size is stored as a uint32_t, rather than a size_t. In 32bit Firefox, size_t is 32bit, but in 64bit Firefox, its 64bits. Making this change means that when people migrate from 32 to 64 bit Firefox, and they re-use the same Firefox profile, their GMP records won't be invalid. Note that Adobe currently EME only works in 32bit Firefox, so existing users aren't affected (yet).
* On startup, GMPDiskStorage enumerates files in its storage dir, and builds an index of the, records. It deletes invalid records from its storage dir. Building the index means that GetRecordNames() operations should be faster; Adobe's GMP does this operation quite a lot.
Attachment #8644050 - Flags: review?(gsquelart)
Comment on attachment 8644050 [details] [diff] [review]
Patch v1: Make GMPStorage immune to record name hash collisions

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

r+ with nits / minor worries.

::: dom/media/gmp/GMPStorageParent.cpp
@@ +85,5 @@
> +// Disked backed GMP storage. Records are stored in files on disk in
> +// the profile directory. The record name is a hash of the filename,
> +// and we resolve hash collisions by just adding 1 to the hash code.
> +// The format of records on disk is:
> +//   4 byte, uint32_t $recordNameLength, in host byte order,

Maybe a far-fetched scenario: Would a user ever want (and be allowed) to move these records between devices with different endiannesses?

@@ +100,5 @@
> +    // Close all open file handles.
> +    for (auto iter = mRecords.ConstIter(); !iter.Done(); iter.Next()) {
> +      if (iter.Data()->mFileDesc) {
> +        PR_Close(iter.Data()->mFileDesc);
> +        iter.Data()->mFileDesc = nullptr;

In ConstIter loops where you don't modify the pointer, you may use iter.UserData(), which returns T* instead in nsAutoPtr<T>&.

@@ +307,5 @@
> +
> +  GMPErr GetRecordNames(nsTArray<nsCString>& aOutRecordNames) override
> +  {
> +    for (auto iter = mRecords.ConstIter(); !iter.Done(); iter.Next()) {
> +      aOutRecordNames.AppendElement(iter.Data()->mRecordName);

UserData()

@@ +410,5 @@
> +    // the file. The record name is not null terminated. The remainder of the
> +    // file is the record's data.
> +
> +    uint32_t recordNameLength = 0;
> +    if (fileLength < sizeof(recordNameLength)) {

Comparison between int32_t and size_t, probably safe here, but doesn't the compiler complain?
And there's quite a bit of arithmetic below between i32, u32, and size_t; it feels dangerous!
But not too much, as all values should normally stay low (sizeof() is 4 or 8, others are checked against some maximum values first, etc.)
So I'm happy to let it go, with the disclaimer that I haven't spent the time to verify all calculations in extreme never-gonna-happen-in-real-life values.

@@ +414,5 @@
> +    if (fileLength < sizeof(recordNameLength)) {
> +      // Record file is empty, or doesn't even have enough contents to
> +      // store the record name length and/or record name. Report record
> +      // as empty.
> +      return NS_OK;

0-length files seem ok, but a file with 1-3 bytes only should not normally exist and therefore should be treated as an error, don't you think?

@@ +421,5 @@
> +    int32_t bytesRead = PR_Read(aFd, &recordNameLength, sizeof(recordNameLength));
> +    if (sizeof(recordNameLength) != bytesRead ||
> +      recordNameLength > fileLength - sizeof(recordNameLength)) {
> +      // Record file has invalid contents. Report record as empty.
> +      return NS_ERROR_FAILURE;

"Report record as empty" confuses me in this situation where you return an error; I'd assume that if you return NS_ERROR_FAILURE, you don't actually report anything, i.e., out parameters cannot be trusted.
Alternatively if you really want to report an empty record, you should reset aOutRecordLength and aOutRecordName, and return NS_OK -- more forgiving.

@@ +435,5 @@
> +    recordName.SetLength(recordNameLength);
> +    bytesRead = PR_Read(aFd, recordName.BeginWriting(), recordNameLength);
> +    if (bytesRead != (int32_t)recordNameLength) {
> +      // Record file has invalid contents. Report record as empty.
> +      return NS_ERROR_FAILURE;

"Report record as empty"? Same as above.
Attachment #8644050 - Flags: review?(gsquelart) → review+
Patch reworked to take into account review comments.

* More aggressive at declaring failure in ReadRecordMetadata(), but more permissive in handling it at the call sites, which basically just pretend records for which ReadRecordMetadata() failed are empty.
* Also added a PR_Sync() call to the Write() implementation, so that if we crash before the file descriptor has been closed, the record is less likely to be corrupted. I'm thinking about a separate patch to close the file descriptor immediately instead; would do that in another bug.

I'll attach an interdiff to help make re-reviewing this easier.
Attachment #8646063 - Flags: review?(gsquelart)
Attachment #8644050 - Attachment is obsolete: true
Comment on attachment 8646064 [details] [diff] [review]
Interdiff of patch v1 to v2

Great help, much appreciated. Review to follow.
Attachment #8646064 - Flags: feedback?(gsquelart) → feedback+
Comment on attachment 8646063 [details] [diff] [review]
Patch v2: Make GMPStorage immune to record name hash collisions

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

r+ with nits.

::: dom/media/gmp/GMPStorageParent.cpp
@@ +82,5 @@
>  
>    return NS_OK;
>  }
>  
> +// Disked backed GMP storage. Records are stored in files on disk in

"Disked backed" -> "Disk-backed"

@@ +285,5 @@
> +    int32_t bytesWritten = 0;
> +    char buf[sizeof(uint32_t)] = {0};
> +    LittleEndian::writeUint32(buf, aRecordName.Length());
> +    bytesWritten = PR_Write(record->mFileDesc, buf, MOZ_ARRAY_LENGTH(buf));
> +    if (bytesWritten < 0 || bytesWritten != MOZ_ARRAY_LENGTH(buf)) {

|bytesWritten < 0| is not necessary, as it is included in |bytesWritten != MOZ_ARRAY_LENGTH(buf)| -- Unless you want to put special emphasis on it; it's hopefully optimised away anyway.

@@ +415,5 @@
> +    // uint32_t (little endian byte order) followed by the record name at the
> +    // start of the file. The record name is not null terminated. The remainder
> +    // of the file is the record's data.
> +
> +    uint32_t recordNameLength = 0;

recordNameLength declaration can be moved down, when doing LittleEndian::readUint32().

@@ +438,5 @@
> +    nsCString recordName;
> +    recordName.SetLength(recordNameLength);
> +    bytesRead = PR_Read(aFd, recordName.BeginWriting(), recordNameLength);
> +    if (bytesRead < 0 ||
> +        (uint32_t)bytesRead != recordNameLength) {

|bytesRead < 0| is not necessary, as it is included in |bytesRead != recordNameLength| -- Unless you want to put special emphasis on it; it's hopefully optimised away anyway.

@@ +450,5 @@
> +    aOutRecordLength = recordLength;
> +    aOutRecordName = recordName;
> +
> +    // Read cursor should be positioned after the record name, before the record contents.
> +    MOZ_ASSERT(PR_Seek(aFd, 0, PR_SEEK_CUR) == sizeof(recordNameLength) + recordNameLength);

Seems a bit rude to assert on a system call that could fail for external reasons. I'd suggest just returning NS_ERROR_FAILURE if that fails.

::: dom/media/gmp/gmp-api/gmp-errors.h
@@ +47,5 @@
>    GMPCryptoErr = 10,
>    GMPEndOfEnumeration = 11,
>    GMPInvalidArgErr = 12,
>    GMPAbortedErr = 13,
> +  GMPRecordCorrupted = 14,

GMPRecordCorrupted is not used in this patch. Did you mean to replace a few GMPGenericErr's with it?
Attachment #8646063 - Flags: review?(gsquelart) → review+
https://hg.mozilla.org/mozilla-central/rev/2704f58f38d7
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Aurora 42 Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=550613675b45

Beta 41 Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41f1f314b4b9

(I had to remove the use of nsHashTable::ConstIter and use EnumerateRead for Beta 41, as ConstIter is not available there)
Comment on attachment 8646063 [details] [diff] [review]
Patch v2: Make GMPStorage immune to record name hash collisions

Approval Request Comment
[Feature/regressing bug #]: EME

[User impact if declined]: 
Playback of protected video on Netflix.

Netlix are seeing reports of failure to write EME plugin storage data. This patch changes the way we store data to remove the potential for a hash collision to cause these failures.

[Describe test coverage new/current, TreeHerder]: We have gtests to cover the EME plugin storage feature.

[Risks and why]: Low; our tests are pretty thorough, so I'm confident in this patch.

[String/UUID change made/needed]: None.

Aurora try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=550613675b45

Beta try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41f1f314b4b9
(Note: Try runs tests that are disabled on Beta runs, so there will be failing boxes there that aren't expected to pass; they can be ignored).
Attachment #8646063 - Flags: approval-mozilla-beta?
Attachment #8646063 - Flags: approval-mozilla-aurora?
Comment on attachment 8646063 [details] [diff] [review]
Patch v2: Make GMPStorage immune to record name hash collisions

Let's uplift the patch to Aurora and stabilize before uplifting to Beta.
Attachment #8646063 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8646063 [details] [diff] [review]
Patch v2: Make GMPStorage immune to record name hash collisions

Approved for uplift to Beta, it has been in Aurora for ~2 days. Beta try pushes look clean (the oranges and reds as Chris explained do not apply to Beta test runs).
Attachment #8646063 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: