Add support for Windows Vista WMF and prepend the system32 directory path to dll names

RESOLVED FIXED in mozilla22

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
8 months ago

People

(Reporter: emk, Assigned: bbondy)

Tracking

({dev-doc-needed})

unspecified
mozilla22
All
Windows Vista
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
(Reporter)

Comment 1

5 years ago
Created attachment 696235 [details] [diff] [review]
patch
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #696235 - Flags: review?(paul)
(Reporter)

Comment 2

5 years ago
Created attachment 696236 [details] [diff] [review]
patch
Attachment #696235 - Attachment is obsolete: true
Attachment #696235 - Flags: review?(paul)
Attachment #696236 - Flags: review?(paul)
Comment on attachment 696236 [details] [diff] [review]
patch

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

::: content/media/wmf/WMFDecoder.cpp
@@ -98,5 @@
> -  //       Win 7     = 6.1
> -  //       Win 8     = 6.2
> -  return versionInfo.dwMajorVersion > 6 ||
> -        (versionInfo.dwMajorVersion == 6 && versionInfo.dwMinorVersion >= 1);
> -}

Have you properly tested this feature on Windows Vista? Chris Pearce could not test this feature on a  Vista machine to make sure everything worked properly, hence this check, supposedly temporary. I don't have a Vista machine as well.

::: content/media/wmf/WMFUtils.cpp
@@ +234,5 @@
> +  uint32_t system32DirLength =
> +    GetSystemDirectoryW(system32Dir.BeginWriting(), system32Dir.Length());
> +  if (!system32DirLength) {
> +    sFailedToLoadDlls = true;
> +    return E_FAIL;

Maybe you can use |GetLastError()| so we can actually know what happens.

@@ +244,2 @@
>    for (uint32_t i = 0; i < dllLength; i++) {
> +    // Prepend the system32 directory path to prevent DLL preload attacks.

Not sure if this is necessary, since the DLL we care about are in the KnownDll list, a DLL dropped in the Firefox application directory would not be loaded. Or at least this is what [1] says. Maybe you are thinking of a different kind of attack, though, please explain.

Also, I don't really know the difference between Vista and 7 (and 8) when it comes to system directory architecture. Could you elaborate on why this is needed for Vista, but apparently not for 7 or 8, as the commit message seem to imply?

[1]: http://blogs.msdn.com/b/larryosterman/archive/2004/07/19/187752.aspx
(Reporter)

Comment 4

5 years ago
(In reply to Paul Adenot (:padenot) [away from 29-12 to 05-01] from comment #3)
> Have you properly tested this feature on Windows Vista? Chris Pearce could
> not test this feature on a  Vista machine to make sure everything worked
> properly, hence this check, supposedly temporary. I don't have a Vista
> machine as well.
Will do.

> @@ +244,2 @@
> >    for (uint32_t i = 0; i < dllLength; i++) {
> > +    // Prepend the system32 directory path to prevent DLL preload attacks.
> 
> Not sure if this is necessary, since the DLL we care about are in the
> KnownDll list, a DLL dropped in the Firefox application directory would not
> be loaded. Or at least this is what [1] says. Maybe you are thinking of a
> different kind of attack, though, please explain.
> 
> Also, I don't really know the difference between Vista and 7 (and 8) when it
> comes to system directory architecture. Could you elaborate on why this is
> needed for Vista, but apparently not for 7 or 8, as the commit message seem
> to imply?

mfreadwrite.dll wasn't present on Vista until Platform Update Supplement is developed. So it's impossible for Vista systems without Platform Update Supplement to know about mfreadwrite.dll.
(Reporter)

Comment 5

5 years ago
Comment on attachment 696236 [details] [diff] [review]
patch

Hm, this patch didn't work on Vista + IE9 system.
Attachment #696236 - Flags: review?(paul)
(Reporter)

Updated

5 years ago
Attachment #696236 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Assignee: VYV03354 → nobody
Status: ASSIGNED → NEW
(In reply to Masatoshi Kimura [:emk] from comment #5)
> Comment on attachment 696236 [details] [diff] [review]
> patch
> 
> Hm, this patch didn't work on Vista + IE9 system.

Do you have the "Platform update for Windows Vista" installed?

Even if you have the platform update installed, I'm not sure it'll work on Vista, I think it should though.
(Reporter)

Comment 7

5 years ago
(In reply to Chris Pearce (:cpearce, away 20 Dec until 10 Jan) from comment #6)
> (In reply to Masatoshi Kimura [:emk] from comment #5)
> > Comment on attachment 696236 [details] [diff] [review]
> > patch
> > 
> > Hm, this patch didn't work on Vista + IE9 system.
> 
> Do you have the "Platform update for Windows Vista" installed?

Platform update is a prerequisite of IE9. I was not able to install "Platform update" alone if IE9 is already installed.
FWIW, the attached patch had a bug. (SetCapacity should be SetLength.) But even when I fixed the bug locally, H.264 playback still failed. (It succeeded on Windows 8 using the same binary.)
(Reporter)

Comment 8

5 years ago
FYI, canPlayType reports that "video/mp4" is supported. So at least DLLs are loaded. But the actual playback fails.
Comment on attachment 696236 [details] [diff] [review]
patch

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

::: content/media/wmf/WMFUtils.cpp
@@ +231,5 @@
> +  // Prepare the system32 directory path.
> +  nsAutoString system32Dir;
> +  system32Dir.SetCapacity(MAX_PATH);
> +  uint32_t system32DirLength =
> +    GetSystemDirectoryW(system32Dir.BeginWriting(), system32Dir.Length());

Passing system32Dir.Length() here won't work, as it returns 0, it's the capacity that you want to pass. So you need to pass MAX_PATH instead of the result of Length(), otherwise GetSystemDirectoryW thinks it's input buffer is 0 length.
I had a go at getting the WMF backend to work on Vista, here's the results of my attempts.

I'm building on Windows 8 with Visual Studio 2012. This uses Windows SDK version 8.0.

I installed Vista in a VM and fully patched it with Windows Update. It definitely has the Platform Update for Windows Vista installed. IE9 can play H.264 HTML5 video.

I built Firefox with the emk's patch. When running on Vista the call to MFStartup() fails, it returns MF_E_BAD_STARTUP_VERSION.

Fair enough, in WMF.h I've defined:
#define WINVER _WIN32_WINNT_WIN7
http://mxr.mozilla.org/mozilla-central/source/content/media/wmf/WMF.h#20

If I change WINVER to _WIN32_WINNT_VISTA, I get compile errors:

error C2027: use of undefined type 'IMFSourceReader' [...]
C:\Program Files (x86)\Windows Kits\8.0\include\um\mfreadwrite.h(52) : see declaration of 'IMFSourceReader'

It turns out the definition of IMFSourceReader is inside an #if (WINVER >= _WIN32_WINNT_WIN7) block.

But according to MSDN IMFSourceReader is supposed to be available on Windows Vista. :(

(Note the mfreadwrite.h header in the Windows 7.0 SDK has the same _WIN32_WINNT_WIN7 guard).

If I change the header (mfreadwrite.h) so that #if is (WINVER >= _WIN32_WINNT_VISTA) I can compile successfully.

In this build still the MFStartup() call still fails on Vista, it returns MF_E_BAD_STARTUP_VERSION.

According to this page:
http://msdn.microsoft.com/en-us/library/windows/desktop/ee663600%28v=vs.85%29.aspx
I should be statically linking against mf_vista.lib and mfplat_vista.lib to access. I tried that, and MFStartup still returns MF_E_BAD_STARTUP_VERSION on Vista.

My googling hasn't been productive, I've found several posts by people with the same problem, but no answers.

I'm going to try building with earlier SDK versions, but I don't have much hope for that.
(Reporter)

Comment 11

5 years ago
(In reply to Chris Pearce (:cpearce) from comment #9)
> Passing system32Dir.Length() here won't work, as it returns 0, it's the
> capacity that you want to pass. So you need to pass MAX_PATH instead of the
> result of Length(), otherwise GetSystemDirectoryW thinks it's input buffer
> is 0 length.

Yeah, it was I said "Fixed it locally but didn't still work" bug in comment #7.
Ah of course, sorry I missed that on re-reading.
(Assignee)

Updated

5 years ago
Assignee: nobody → netzen
(Assignee)

Updated

5 years ago
Summary: Prepend the system32 directory path to dll names to load wmf dlls safely on Vista → Add support for Windows Vista WMF and prepend the system32 directory path to dll names
(Assignee)

Comment 13

5 years ago
(In reply to Chris Pearce (:cpearce) from comment #10)
> I had a go at getting the WMF backend to work on Vista, here's the results
> of my attempts.
> 
> I'm building on Windows 8 with Visual Studio 2012. This uses Windows SDK
> version 8.0.
> 
> I installed Vista in a VM and fully patched it with Windows Update. It
> definitely has the Platform Update for Windows Vista installed. IE9 can play
> H.264 HTML5 video.
> 
> I built Firefox with the emk's patch. When running on Vista the call to
> MFStartup() fails, it returns MF_E_BAD_STARTUP_VERSION.
> 
> Fair enough, in WMF.h I've defined:
> #define WINVER _WIN32_WINNT_WIN7
> http://mxr.mozilla.org/mozilla-central/source/content/media/wmf/WMF.h#20
> 
> If I change WINVER to _WIN32_WINNT_VISTA, I get compile errors:
> 
> error C2027: use of undefined type 'IMFSourceReader' [...]
> C:\Program Files (x86)\Windows Kits\8.0\include\um\mfreadwrite.h(52) : see
> declaration of 'IMFSourceReader'
> 
> It turns out the definition of IMFSourceReader is inside an #if (WINVER >=
> _WIN32_WINNT_WIN7) block.
> 
> But according to MSDN IMFSourceReader is supposed to be available on Windows
> Vista. :(
> 
> (Note the mfreadwrite.h header in the Windows 7.0 SDK has the same
> _WIN32_WINNT_WIN7 guard).
> 
> If I change the header (mfreadwrite.h) so that #if is (WINVER >=
> _WIN32_WINNT_VISTA) I can compile successfully.
> 
> In this build still the MFStartup() call still fails on Vista, it returns
> MF_E_BAD_STARTUP_VERSION.
> 
> According to this page:
> http://msdn.microsoft.com/en-us/library/windows/desktop/ee663600%28v=vs.
> 85%29.aspx
> I should be statically linking against mf_vista.lib and mfplat_vista.lib to
> access. I tried that, and MFStartup still returns MF_E_BAD_STARTUP_VERSION
> on Vista.
> 
> My googling hasn't been productive, I've found several posts by people with
> the same problem, but no answers.
> 
> I'm going to try building with earlier SDK versions, but I don't have much
> hope for that.

This confusion is really due to a header #fail on Microsoft's part. Just because you're using a certain SDK doesn't mean you are only targeting that associate Windows version.  They should have made 2 defines for vista and win7 plus and allowed you to set it, or even better just detected themselves within the function :(

Anyway fix being attached now which works in both Vista and Win7+.
(Assignee)

Comment 14

5 years ago
Created attachment 729269 [details] [diff] [review]
Patch v1 - WMF Vista support + load libraries from system32 directly
Attachment #729269 - Flags: review?(paul)
(Assignee)

Comment 15

5 years ago
Created attachment 729270 [details] [diff] [review]
Patch v1 - Is vista check

I'd like to eventually move the widget helper OS checks here and cleanup the code throughout the tree, but for now just adding a small helper similar to the current IsVistaOrLater.
Attachment #729270 - Flags: review?(jmathies)

Comment 16

5 years ago
(In reply to Brian R. Bondy [:bbondy] from comment #15)
> Created attachment 729270 [details] [diff] [review]
> Patch v1 - Is vista check
> 
> I'd like to eventually move the widget helper OS checks here and cleanup the
> code throughout the tree, but for now just adding a small helper similar to
> the current IsVistaOrLater.

I actually prefer the WinUtils version checks over functions like this. In fact, you can use those now and not add anything to this file since WinUtils.h is now exported.
(Assignee)

Comment 17

5 years ago
ok nice, I'll update to that and in that case I won't need a review from you :D
(Assignee)

Updated

5 years ago
Attachment #729270 - Flags: review?(jmathies)
(Assignee)

Comment 18

5 years ago
Created attachment 729339 [details] [diff] [review]
Patch v2 - WMF Vista support + load libraries from system32 directly

Updated as per Jim's suggestion for OS detection.
Attachment #729269 - Attachment is obsolete: true
Attachment #729270 - Attachment is obsolete: true
Attachment #729269 - Flags: review?(paul)
Attachment #729339 - Flags: review?(paul)
Comment on attachment 729339 [details] [diff] [review]
Patch v2 - WMF Vista support + load libraries from system32 directly

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

::: content/media/wmf/WMFUtils.cpp
@@ +273,5 @@
>  {
>    RefPtr<IMFPluginControl> pluginControl;
>    HRESULT hr = wmf::MFGetPluginControl(byRef(pluginControl));
> +  if (SUCCEEDED(hr) && pluginControl) {
> +    NS_ENSURE_TRUE(SUCCEEDED(hr), hr);

This |NS_ENSURE_TRUE| is now useless, since we go in this if only if |hr| is 0.
Attachment #729339 - Flags: review?(paul) → review+
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad472cef6e89
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/ad472cef6e89
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed

Comment 22

5 years ago
I added Vista on https://developer.mozilla.org/en-US/docs/HTML/Supported_media_formats

Updated

4 years ago
Blocks: 799318
Depends on: 1324183
You need to log in before you can comment on or make changes to this bug.