Last Comment Bug 825153 - Add support for Windows Vista WMF and prepend the system32 directory path to dll names
: Add support for Windows Vista WMF and prepend the system32 directory path to ...
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: unspecified
: All Windows Vista
: -- normal with 3 votes (vote)
: mozilla22
Assigned To: Brian R. Bondy [:bbondy]
:
: Maire Reavy [:mreavy]
Mentors:
Depends on:
Blocks: 799315 799318
  Show dependency treegraph
 
Reported: 2012-12-27 23:26 PST by Masatoshi Kimura [:emk]
Modified: 2016-11-30 05:09 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.53 KB, patch)
2012-12-27 23:34 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
patch (3.59 KB, patch)
2012-12-27 23:44 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Patch v1 - WMF Vista support + load libraries from system32 directly (5.05 KB, patch)
2013-03-25 16:05 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch v1 - Is vista check (992 bytes, patch)
2013-03-25 16:07 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch v2 - WMF Vista support + load libraries from system32 directly (5.23 KB, patch)
2013-03-25 17:57 PDT, Brian R. Bondy [:bbondy]
padenot: review+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2012-12-27 23:26:37 PST

    
Comment 1 Masatoshi Kimura [:emk] 2012-12-27 23:34:13 PST
Created attachment 696235 [details] [diff] [review]
patch
Comment 2 Masatoshi Kimura [:emk] 2012-12-27 23:44:03 PST
Created attachment 696236 [details] [diff] [review]
patch
Comment 3 Paul Adenot (:padenot) 2012-12-28 09:02:00 PST
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
Comment 4 Masatoshi Kimura [:emk] 2012-12-28 16:35:09 PST
(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.
Comment 5 Masatoshi Kimura [:emk] 2012-12-31 06:45:38 PST
Comment on attachment 696236 [details] [diff] [review]
patch

Hm, this patch didn't work on Vista + IE9 system.
Comment 6 Chris Pearce (:cpearce) 2013-01-04 06:05:19 PST
(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.
Comment 7 Masatoshi Kimura [:emk] 2013-01-04 06:19:04 PST
(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.)
Comment 8 Masatoshi Kimura [:emk] 2013-01-04 06:27:29 PST
FYI, canPlayType reports that "video/mp4" is supported. So at least DLLs are loaded. But the actual playback fails.
Comment 9 Chris Pearce (:cpearce) 2013-01-15 17:43:42 PST
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.
Comment 10 Chris Pearce (:cpearce) 2013-01-15 18:40:47 PST
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.
Comment 11 Masatoshi Kimura [:emk] 2013-01-16 04:09:31 PST
(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.
Comment 12 Chris Pearce (:cpearce) 2013-01-16 12:30:39 PST
Ah of course, sorry I missed that on re-reading.
Comment 13 Brian R. Bondy [:bbondy] 2013-03-25 16:02:27 PDT
(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+.
Comment 14 Brian R. Bondy [:bbondy] 2013-03-25 16:05:35 PDT
Created attachment 729269 [details] [diff] [review]
Patch v1 - WMF Vista support + load libraries from system32 directly
Comment 15 Brian R. Bondy [:bbondy] 2013-03-25 16:07:00 PDT
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.
Comment 16 Jim Mathies [:jimm] 2013-03-25 17:38:24 PDT
(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.
Comment 17 Brian R. Bondy [:bbondy] 2013-03-25 17:40:06 PDT
ok nice, I'll update to that and in that case I won't need a review from you :D
Comment 18 Brian R. Bondy [:bbondy] 2013-03-25 17:57:40 PDT
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.
Comment 19 Paul Adenot (:padenot) 2013-03-26 03:34:45 PDT
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.
Comment 21 Ryan VanderMeulen [:RyanVM] 2013-03-27 10:01:16 PDT
https://hg.mozilla.org/mozilla-central/rev/ad472cef6e89
Comment 22 Jesper Kristensen 2013-03-28 17:13:38 PDT
I added Vista on https://developer.mozilla.org/en-US/docs/HTML/Supported_media_formats

Note You need to log in before you can comment on or make changes to this bug.