Closed Bug 582416 Opened 14 years ago Closed 6 years ago

nsVideoFrame::BuildLayer reads uninitialised stack allocated memory

Categories

(Core :: Audio/Video: Playback, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED INACTIVE

People

(Reporter: jseward, Unassigned)

References

Details

(Keywords: valgrind)

Attachments

(1 file, 1 obsolete file)

m-c, checkout as of today, x86_64-linux build,
content/media/test/test_playback.html

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


Analysis
~~~~~~~~

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,
here:

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
                 (FrameLayerBuilder.cpp:1006)
   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
                 (FrameLayerBuilder.cpp:1173)
   by 0x57179E8: nsDisplayList::PaintForFrame
                 (nsDisplayList.cpp:395)
   by 0x5728D70: nsLayoutUtils::PaintFrame
                 (nsLayoutUtils.cpp:1343)
   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)
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.
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
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+
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
Mass closing do to inactivity.
Feel free to re-open if still needed.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INACTIVE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: