Bug 173862 - [GTK] Layout Test webrtc/video.html issues "stack smashing detected"
Summary: [GTK] Layout Test webrtc/video.html issues "stack smashing detected"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-26 21:53 PDT by Fujii Hironori
Modified: 2017-06-26 23:41 PDT (History)
5 users (show)

See Also:


Attachments
WIP patch (830 bytes, patch)
2017-06-26 22:07 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (2.66 KB, patch)
2017-06-26 23:10 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2017-06-26 21:53:55 PDT
[GTK] Layout Test webrtc/video.html issues "stack smashing detected"

webrtc/video.html issues "stack smashing detected" in my linux box (Ubuntu 17.04) while build bot does't.
I'm using Gtk port, trunk@218806, Release build.

> fujii@ubuntu $ ./Tools/Scripts/run-webkit-tests --gtk --release --no-new-test-results -v --iterations=3 webrtc/video.html
> Using port 'gtk-wk2'
> Test configuration: <, x86, release>
> Placing test results in /home/fujii/work/webkit/ga/WebKitBuild/Release/layout-test-results
> Baseline search path: platform/gtk -> platform/wk2 -> generic
> Using Release build
> Pixel tests disabled
> Regular timeout: 15000, slow test timeout: 75000
> Command line: /home/fujii/work/webkit/ga/Tools/jhbuild/jhbuild-wrapper --gtk run /home/fujii/work/webkit/ga/WebKitBuild/Release/bin/WebKitTestRunner -
> 
> Found 1 test; running 1 (3 times each: --repeat-each=1 --iterations=3), skipping 0.
> 
> Running 1 test
> 
> Running 1 WebKitTestRunner.     
> 
> [1/3] webrtc/video.html*** stack smashing detected ***: /home/fujii/work/webkit/ga/WebKitBuild/Release/bin/WebKitWebProcess terminated
> [1/3] webrtc/video.html failed (text diff)
> [2/3] webrtc/video.html*** stack smashing detected ***: /home/fujii/work/webkit/ga/WebKitBuild/Release/bin/WebKitWebProcess terminated
> [2/3] webrtc/video.html failed (text diff)
> [3/3] webrtc/video.html*** stack smashing detected ***: /home/fujii/work/webkit/ga/WebKitBuild/Release/bin/WebKitWebProcess terminated
> [3/3] webrtc/video.html failed (text diff)
> 
> All 3 tests ran as expected.
Comment 1 Fujii Hironori 2017-06-26 22:07:36 PDT
Created attachment 313896 [details]
WIP patch

It seems that g_object_get needs to be passed an int pointer instread of a bool pointer.
WIP patch solved the stack smashing.

But, there are other similar codes using g_object_get and bool pointers.

> bool MediaPlayerPrivateGStreamerBase::muted() const
> {
>     if (!m_volumeElement)
>         return false;
> 
>     bool muted;
>     g_object_get(m_volumeElement.get(), "mute", &muted, nullptr);
>     return muted;
> }

Does anyone have any thought?
Comment 2 Carlos Garcia Campos 2017-06-26 22:31:05 PDT
Comment on attachment 313896 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313896&action=review

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerOwr.cpp:364
> +    int audioDisabled;
> +    int videoDisabled;

Use gboolean instead of int.
Comment 3 Carlos Garcia Campos 2017-06-26 22:31:56 PDT
(In reply to Fujii Hironori from comment #1)
> Created attachment 313896 [details]
> WIP patch
> 
> It seems that g_object_get needs to be passed an int pointer instread of a
> bool pointer.

Yes, this has caused problems in the past.

> WIP patch solved the stack smashing.
> 
> But, there are other similar codes using g_object_get and bool pointers.
> 
> > bool MediaPlayerPrivateGStreamerBase::muted() const
> > {
> >     if (!m_volumeElement)
> >         return false;
> > 
> >     bool muted;
> >     g_object_get(m_volumeElement.get(), "mute", &muted, nullptr);
> >     return muted;
> > }
> 
> Does anyone have any thought?

We should fix all of them, by using gboolean instead of bool.
Comment 4 Michael Catanzaro 2017-06-26 22:36:49 PDT
You're right. I made this mistake once before, and it took hours before I realized the problem. Of course a GObject property is going to be a gboolean (an int), not a C++ bool (one byte). This is such an easy mistake to make when not being very careful. If you pass just one byte, then g_object_get() will write three bytes out of bounds.

It's probably impractical to audit the entire codebase for this issue, but we should check the revision that introduced the bug to check for other misuses, and also check surrounding code.
Comment 5 Fujii Hironori 2017-06-26 23:10:31 PDT
Created attachment 313900 [details]
Patch

Thank you, KaL and Michael. I did audit other usages. I didn't find any other problem.
Comment 6 Carlos Garcia Campos 2017-06-26 23:13:36 PDT
Comment on attachment 313900 [details]
Patch

Excellent, thank you!
Comment 7 WebKit Commit Bot 2017-06-26 23:41:55 PDT
Comment on attachment 313900 [details]
Patch

Clearing flags on attachment: 313900

Committed r218832: <http://trac.webkit.org/changeset/218832>
Comment 8 WebKit Commit Bot 2017-06-26 23:41:56 PDT
All reviewed patches have been landed.  Closing bug.