Bug 173862 – [GTK] Layout Test webrtc/video.html issues "stack smashing detected"

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Fujii Hironori]

[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.

[Fujii Hironori]

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?

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

(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.

[Michael Catanzaro]

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.

[Fujii Hironori]

Created attachment 313900 [details]
Patch

Thank you, KaL and Michael. I did audit other usages. I didn't find any other problem.

[Carlos Garcia Campos]

Comment on attachment 313900 [details]
Patch

Excellent, thank you!

[WebKit Commit Bot]

Comment on attachment 313900 [details]
Patch

Clearing flags on attachment: 313900

Committed r218832: <http://trac.webkit.org/changeset/218832>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.