Closed Bug 1190252 Opened 5 years ago Closed 5 years ago

[EME] Loading GMPs can fail due to use of std:string and std:ifstream in GMP child process

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 --- wontfix
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

We should not mix std strings and I/O where possible into Gecko code. We've had Adobe's GMP fail to load when the in the path to the GMP dir has non-ASCII characters in it due to the characters being mangled. We're still seeing loading Adobe's GMP failing, so I'm suspecting it's because we're using std::ifstream in GMPChild::PreLoadLibraries(), PreLoadPluginVoucher() and PreLoadSandboxVoucher(). I assume this problem was fixed years ago in NSPR file handling, so removing std string and ifstream from GMPChild will eliminate this as a possibility.
Remove std::ifstream usage. We can't use nsILineInputStream etc for I/O as that requires XPCOM to be initialized. So I had to roll my own split functions.
Attachment #8642249 - Flags: review?(edwin)
Should uplift to 41.
Flags: needinfo?(cpearce)
Summary: [EME] GMPChild should not use std:string and std:ifstream → [EME] Loading GMPs can fail due to use of std:string and std:ifstream in GMP child process
Comment on attachment 8642249 [details] [diff] [review]
Patch 2: Use nsIFile and NSPR I/O instead of std::io

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

::: dom/media/gmp/GMPChild.cpp
@@ +334,5 @@
> +{
> +  nsTArray<uint8_t> buf;
> +  bool rv = ReadIntoArray(aFile, buf, aMaxLength);
> +  if (rv) {
> +    aOutDst = nsDependentCString((const char*)buf.Elements(), buf.Length());

nsDependentCString is throwing because I'm passing a non-null terminated buffer here.

So insert:

  buf.AppendElement(0); // Append null terminator, required by nsC*String.

@@ +400,4 @@
>      }
> +    // Line starts with "libraries:".
> +    nsTArray<nsCString> libs = SplitAt(Tokenizer::Token::Char(','),
> +                                       Substring(line, offset));

That Substring needs to be:

  Substring(line, offset + strlen("libraries:"))

So that the first whiteListedLib below is "dxva2.dll" and not "libraries: dxva2.dll" when parsing clearkey.info.
Chris - just so I understand a bit better -- is it a problem if the GMP plugin itself is using std:string? We should not be using std:ifstream AFAIK, but std:string is being used.
(In reply to Joe Steele from comment #10)
> Chris - just so I understand a bit better -- is it a problem if the GMP
> plugin itself is using std:string? We should not be using std:ifstream
> AFAIK, but std:string is being used.

I don't believe your GMP using std::string would be a problem.

I believe the problems we have are caused by non-ASCII characters being in the paths to files (your DLL, voucher, and the plugin-container voucher) and the use of std::string somehow munging the characters, and/or std::ifstream internally fails to load paths with those characters in them. Since your GMP doesn't do any of that, and indeed probably won't ever be exposed to any input which could non 8-bit bytes/characters, I think you don't need to worry about it.
https://hg.mozilla.org/mozilla-central/rev/064871541f3d
https://hg.mozilla.org/mozilla-central/rev/ad6eae12a195
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
(In reply to Chris Pearce (:cpearce) from comment #13)
> 41 (Aurora ) Try push
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f457452dbf36

This push failed to build because mozilla/Tokenizer.h is not available on Aurora yet...
Switch from the only-in-Nightly mozilla/Tokenizer to NS_strtok. Note NS_strtok doesn't rely on static storage, so it's thread safe.

Also included a gtest for my line-break code.

Edwin's probably still sitting on a plane, so r?gerald.
Attachment #8645513 - Flags: review?(gsquelart)
Attachment #8645513 - Flags: review?(gsquelart) → review+
Backed out because clang MacOSX doesn't like my std::vector<std::string> initializer list, so I've removed them:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28fa781f757c
Just to clarify the situation here, there was two landings; the main fix, and a follow up to make it build in 41. The follow up to make it build in 41 landed after the uplift.

These changes are in 42:
https://hg.mozilla.org/integration/mozilla-inbound/rev/064871541f3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6eae12a195

This change is in 43:
https://hg.mozilla.org/mozilla-central/rev/b38760515fda

Therefore...

I need to uplift to 42: b38760515fda

I need to uplift to 41: 064871541f3d, ad6eae12a195, and b38760515fda
(In reply to Chris Pearce (:cpearce) from comment #22)
> I need to uplift to 42: b38760515fda

Auror 42 Try push of b38760515fda:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa6c51bf0941
(Green)

> I need to uplift to 41: 064871541f3d, ad6eae12a195, and b38760515fda

Beta 41 Try push of 064871541f3d, ad6eae12a195, and b38760515fda
https://treeherder.mozilla.org/#/jobs?repo=try&revision=318b2c34a8f3
(Only jobs that are not run on our actual beta branches are failing here, i.e. we run boxes on Try that we don't run on Beta, and those extra boxes are failing).
Comment on attachment 8642248 [details] [diff] [review]
Patch 1: Remove std::string from GMPChild and friends

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

[User impact if declined]:
Users who have installed Firefox into a directory with non-ASCII characters in the path may fail to start up playback of EME protected videos.

We're seeing some EME video playback failures due to failing to load some files out of the Firefox install directory, and I think this patch is likely to fix them.

[Describe test coverage new/current, TreeHerder]: We have lots of tests covering the area of code touched (GMP loading).

[Risks and why]: Low. This patch just changes how we store some file paths in memory, so use APIs that we've used for years.

[String/UUID change made/needed]: None.
Attachment #8642248 - Flags: approval-mozilla-beta?
Comment on attachment 8642249 [details] [diff] [review]
Patch 2: Use nsIFile and NSPR I/O instead of std::io

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

[User impact if declined]:
Users who have installed Firefox into a directory with non-ASCII characters in the path may fail to start up playback of EME protected videos.

We're seeing some EME video playback failures due to failing to load some files out of the Firefox install directory, and I think this patch is likely to fix them.

[Describe test coverage new/current, TreeHerder]: We have lots of tests covering the area of code touched (GMP loading).

[Risks and why]: Low. This patch just changes how we store some read files out of the Firefox install dir, to use APIs that we've used for years. See comment 23 for Green try pushes.

[String/UUID change made/needed]: None.
Attachment #8642249 - Flags: approval-mozilla-beta?
Comment on attachment 8645513 [details] [diff] [review]
Patch 3: Switch from mozilla/Tokenizer to NS_strtok

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

[User impact if declined]:
Users who have installed Firefox into a directory with non-ASCII characters in the path may fail to start up playback of EME protected videos.

We're seeing some EME video playback failures due to failing to load some files out of the Firefox install directory, and I think this patch is likely to fix them.

[Describe test coverage new/current, TreeHerder]: We have lots of tests covering the area of code touched (GMP loading).

[Risks and why]: Low. This patch just changes how we store some read the EME plugin metadata file to use APIs that are not Gecko42-and-later only; i.e. this patch makes the earlier patches build on Firefox41. See comment 23 for Green try pushes.

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


(Note this patch is needed on 42 and 41, whereas the other two uplift requests only need to be uplifted to 41 since they're already in 42)
Attachment #8645513 - Flags: approval-mozilla-beta?
Attachment #8645513 - Flags: approval-mozilla-aurora?
Comment on attachment 8645513 [details] [diff] [review]
Patch 3: Switch from mozilla/Tokenizer to NS_strtok

This patch is pretty big, let's uplift to Aurora first and then wait a day or two before we uplift to Beta. Fix has baked in m-c for a week, seems safe to land on Aurora.
Attachment #8645513 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8642248 [details] [diff] [review]
Patch 1: Remove std::string from GMPChild and friends

Approved for uplift to Beta. We want this for EME/Netflix testing in FF41 Beta2. Try pushes look clean.
Attachment #8642248 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8642249 [details] [diff] [review]
Patch 2: Use nsIFile and NSPR I/O instead of std::io

Approved.
Attachment #8642249 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8645513 [details] [diff] [review]
Patch 3: Switch from mozilla/Tokenizer to NS_strtok

Approved for Beta as this code has been in Aurora for ~2 days. We want this fix in 41.0 Beta1 for EME testing by Netflix.
Attachment #8645513 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.