Closed
Bug 582416
Opened 14 years ago
Closed 6 years ago
nsVideoFrame::BuildLayer reads uninitialised stack allocated memory
Categories
(Core :: Audio/Video: Playback, defect)
Tracking
()
RESOLVED
INACTIVE
People
(Reporter: jseward, Unassigned)
References
Details
(Keywords: valgrind)
Attachments
(1 file, 1 obsolete file)
927 bytes,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•14 years ago
|
||
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.
Reporter | ||
Updated•14 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.
Updated•14 years ago
|
Component: Layout → Video/Audio
QA Contact: layout → video.audio
Reporter | ||
Comment 3•14 years ago
|
||
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+
Reporter | ||
Updated•14 years ago
|
Keywords: checkin-needed,
valgrind
patch is duplicated by part 3 in bug 574481
Depends on: 574481
Keywords: checkin-needed
Comment 6•14 years ago
|
||
Could this be the cause of these ff4 b4 crashes? http://crash-stats.mozilla.com/report/index/bp-c96ad30c-e5e8-45ae-a794-fd5332100821 http://crash-stats.mozilla.com/report/index/bp-86b210c0-a6c0-47a1-9541-efa962100821 There seems to be a close correlation in the stacks.
Updated•9 years ago
|
Component: Audio/Video → Audio/Video: Playback
Comment 8•6 years ago
|
||
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.
Description
•