nsVideoFrame::BuildLayer reads uninitialised stack allocated memory




Audio/Video: Playback
8 years ago
3 years ago


(Reporter: jseward, Unassigned)




Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



8 years ago
m-c, checkout as of today, x86_64-linux build,

layout/generic/nsVideoFrame.cpp, function nsVideoFrame::BuildLayer
uses uninitialised memory from its own frame.  Valgrind output is


199:    CairoImage::Data cairoData;

cairoData.mSize is uninitialised.  Package it up somehow or other
into a gfxASurface:

200:    nsRefPtr<gfxASurface> imageSurface;
204:      imageSurface = container->GetCurrentAsSurface(&cairoData.mSize);

I'm not exactly sure how the data flows around after that.
Nevertheless the uninitialised cairoData.mSize soon turns up again,

229:  if (frameSize.width == 0 || frameSize.height == 0) {

It's easy to demonstrate the analysis correct.  At line 199, 
set cairoData.mSize to bogus values:

199:    CairoImage::Data cairoData; cairoData.mSize.width = 123; cairoData.mSize.height = 456;

and print frameSize.width just before line 229:

229:  fprintf(stderr, "XXXX got %d %d\n", frameSize.width, frameSize.height);

The initialisation shuts Valgrind up (as expected), and the bogus values
duly show up when running content/media/test/test_playback.html:

XXXX got 144 112
XXXX got 123 456
XXXX got 144 112
XXXX got 123 456
XXXX got 426 240
XXXX got 426 240

Raw vg output
Conditional jump or move depends on uninitialised value(s)
   at 0x57C107D: nsVideoFrame::BuildLayer (nsVideoFrame.cpp:229)
   by 0x57C14EF: nsDisplayVideo::BuildLayer (nsVideoFrame.cpp:376)
   by 0x56E7F84: mozilla::FrameLayerBuilder::AddThebesDisplayItem
   by 0x56E8972: mozilla::(anonymous namespace)::ContainerState::
                 ProcessDisplayItems (FrameLayerBuilder.cpp:944)
   by 0x56E820E: mozilla::(anonymous namespace)::ContainerState::
                 ProcessDisplayItems (FrameLayerBuilder.cpp:853)
   by 0x56E9356: mozilla::FrameLayerBuilder::BuildContainerLayerFor
   by 0x57179E8: nsDisplayList::PaintForFrame
   by 0x5728D70: nsLayoutUtils::PaintFrame
   by 0x57376EA: PresShell::Paint (nsPresShell.cpp:5907)
   by 0x5AABA37: nsViewManager::RenderViews (nsViewManager.cpp:448)
   by 0x5AABBAA: nsViewManager::Refresh (nsViewManager.cpp:414)
   by 0x5AAD4DD: nsViewManager::DispatchEvent (nsViewManager.cpp:843)

 Uninitialised value was created by a stack allocation
   at 0x57C0E70: nsVideoFrame::BuildLayer (nsVideoFrame.cpp:181)

Comment 1

8 years ago
Created attachment 460694 [details] [diff] [review]
trivial fix (may just be hiding a deeper problem, but ..)

Trivial fix.  Surely just hides the symptoms, but at least shuts
up valgrind and doesn't cause the test to fail.  Presumably someone
familiar with the code can come up with a proper fix.


8 years ago
Attachment #460694 - Flags: review?(roc)
GetCurrentAsSurface is supposed to initialize cairoData.mSize as an out-parameter. But if it fails, it returns null and mSize is uninitialized. So the correct fix is to null-check imageSurface after the call and bail out by returning nsnull.
Component: Layout → Video/Audio
QA Contact: layout → video.audio

Comment 3

8 years ago
Created attachment 460805 [details] [diff] [review]
revised patch as per comment 2

Tests ok with content/media/test/test_playback.html and with valgrind.
Attachment #460694 - Attachment is obsolete: true
Attachment #460805 - Flags: review?(roc)
Attachment #460694 - Flags: review?(roc)
Comment on attachment 460805 [details] [diff] [review]
revised patch as per comment 2

Thank you!
Attachment #460805 - Flags: review?(roc) → review+


8 years ago
Keywords: checkin-needed, valgrind
patch is duplicated by part 3 in bug 574481
Depends on: 574481
Keywords: checkin-needed
And does comment 5 mean that this is now fixed?
Component: Audio/Video → Audio/Video: Playback
You need to log in before you can comment on or make changes to this bug.