Closed Bug 1114867 Opened 5 years ago Closed 5 years ago

[EME] Clear stack memory after device binding in GMPLoader

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

In bug 1101308 I discovered that the code to zero the memory on the stack down to the guard page was corrupting parts of the stack we were using and causing GMPs to crash randomly. I will remove that code in bug 1101308, but we need to reinstate a fixed copy of this code to ensure that no user-identifiable state is left behind after we've performed device binding.
This patch just reverts the prior backout. The actual fix is in the next patch, for easier review.
Attachment #8543724 - Flags: review?(dmajor)
This basically inline code equivalent to the x86 case of _RtlSecureZeroMemory from winnt.h on my machine (Visual Studio 2013 Pro).

I didn't inline the x64 case using __stosb. I haven't tested x64.

I tested using __stosb in my 32bit build, and it seemed to work fine. I don't know why the MS forceinline definition of RtlSecureZeroMemory didn't use __stosb in the x86 case too, I fear there's a good reason, so I didn't want to risk using it myself.

I checked the assembly in an opt build on my machine, and the volatile loop was not optimized out.
Attachment #8543734 - Flags: review?(dmajor)
Attachment #8543724 - Flags: review?(dmajor) → review+
Attachment #8543734 - Flags: review?(dmajor) → review+
It would be nice to have a test for this, to make sure some future compiler doesn't mess with the loop... but I'm guessing it wouldn't be easy.
Presumably, gtests will start crashing again. I never saw them reported failing on our test machines, only locally however...
Mass update firefox-status to track EME uplift.
You need to log in before you can comment on or make changes to this bug.