173862 2017-06-26 21:53:55 -0700 [GTK] Layout Test webrtc/video.html issues "stack smashing detected" 2017-06-26 23:41:56 -0700 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 fujii fujii alex bugs-noreply cgarcia commit-queue mcatanzaro oldest_to_newest 1323063 0 fujii 2017-06-26 21:53:55 -0700 [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. 1323065 1 313896 fujii 2017-06-26 22:07:36 -0700 Created attachment 313896 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? 1323069 2 313896 cgarcia 2017-06-26 22:31:05 -0700 Comment on attachment 313896 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. 1323070 3 cgarcia 2017-06-26 22:31:56 -0700 (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. 1323074 4 mcatanzaro 2017-06-26 22:36:49 -0700 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. 1323081 5 313900 fujii 2017-06-26 23:10:31 -0700 Created attachment 313900 Patch Thank you, KaL and Michael. I did audit other usages. I didn't find any other problem. 1323082 6 313900 cgarcia 2017-06-26 23:13:36 -0700 Comment on attachment 313900 Patch Excellent, thank you! 1323086 7 313900 commit-queue 2017-06-26 23:41:55 -0700 Comment on attachment 313900 Patch Clearing flags on attachment: 313900 Committed r218832: <http://trac.webkit.org/changeset/218832> 1323087 8 commit-queue 2017-06-26 23:41:56 -0700 All reviewed patches have been landed. Closing bug. 313896 2017-06-26 22:07:36 -0700 2017-06-26 23:10:26 -0700 WIP patch w.patch text/plain 830 fujii ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2dzdHJlYW1lci9N ZWRpYVBsYXllclByaXZhdGVHU3RyZWFtZXJPd3IuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZv cm0vZ3JhcGhpY3MvZ3N0cmVhbWVyL01lZGlhUGxheWVyUHJpdmF0ZUdTdHJlYW1lck93ci5jcHAK aW5kZXggNzQxYTVjODc2MTYuLjk5N2JhZjI0OTA5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9wbGF0Zm9ybS9ncmFwaGljcy9nc3RyZWFtZXIvTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVy T3dyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9nc3RyZWFtZXIv TWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyT3dyLmNwcApAQCAtMzYwLDggKzM2MCw4IEBAIHZv aWQgTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyT3dyOjp0cmFja0VuZGVkKE1lZGlhU3RyZWFt VHJhY2tQcml2YXRlJiB0cmFjaykKICAgICAgICAgICAgIHZpZGVvVHJhY2stPnNldFNlbGVjdGVk KGZhbHNlKTsKICAgICB9CiAKLSAgICBib29sIGF1ZGlvRGlzYWJsZWQ7Ci0gICAgYm9vbCB2aWRl b0Rpc2FibGVkOworICAgIGludCBhdWRpb0Rpc2FibGVkOworICAgIGludCB2aWRlb0Rpc2FibGVk OwogICAgIGdfb2JqZWN0X2dldChtX2F1ZGlvUmVuZGVyZXIuZ2V0KCksICJkaXNhYmxlZCIsICZh dWRpb0Rpc2FibGVkLCBudWxscHRyKTsKICAgICBnX29iamVjdF9nZXQobV92aWRlb1JlbmRlcmVy LmdldCgpLCAiZGlzYWJsZWQiLCAmdmlkZW9EaXNhYmxlZCwgbnVsbHB0cik7CiAgICAgaWYgKGF1 ZGlvRGlzYWJsZWQgJiYgdmlkZW9EaXNhYmxlZCkgewo= 313900 2017-06-26 23:10:31 -0700 2017-06-26 23:41:55 -0700 Patch bug-173862-20170627150812.patch text/plain 2727 fujii U3VidmVyc2lvbiBSZXZpc2lvbjogMjE4ODA2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYTQ1Zjg4OTUxNDJlNzQx ZGZjNzNhYzRmNjU5Yjc2NmU1YmQ0Yjg2NS4uNDM0MzJhOThiODM0Mjc2NDMxOTAzZjU1YzViMzkx MTFmMWQzZDZmYSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDE3LTA2LTI2ICBGdWpp aSBIaXJvbm9yaSAgPEhpcm9ub3JpLkZ1amlpQHNvbnkuY29tPgorCisgICAgICAgIFtHVEtdIExh eW91dCBUZXN0IHdlYnJ0Yy92aWRlby5odG1sIGlzc3VlcyAic3RhY2sgc21hc2hpbmcgZGV0ZWN0 ZWQiCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNzM4 NjIKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUZXN0 czogd2VicnRjL3ZpZGVvLmh0bWwKKworICAgICAgICBQYXNzaW5nIGEgYm9vbCB2YXJpYWJsZSB0 byBnX29iamVjdF9nZXQgY2F1c2VzIG91dC1vZi1ib3VuZCB3cml0ZS4KKyAgICAgICAgZ2Jvb2xl YW4gc2hvdWxkIGJlIHVzZWQsIHdoaWNoIGlzIDQgYnl0ZXMgd2hpbGUgYm9vbCBpcyBvbmUgYnl0 ZS4KKworICAgICAgICAqIHBsYXRmb3JtL2dyYXBoaWNzL2dzdHJlYW1lci9NZWRpYVBsYXllclBy aXZhdGVHU3RyZWFtZXJCYXNlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Ok1lZGlhUGxheWVyUHJp dmF0ZUdTdHJlYW1lckJhc2U6Om11dGVkKTogVXNlIGdib29sZWFuIGluc3RlYWQgb2YgYm9vbC4K KyAgICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy9nc3RyZWFtZXIvTWVkaWFQbGF5ZXJQcml2YXRl R1N0cmVhbWVyT3dyLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Ok1lZGlhUGxheWVyUHJpdmF0ZUdT dHJlYW1lck93cjo6dHJhY2tFbmRlZCk6IERpdHRvLgorCiAyMDE3LTA2LTI2ICBLb25zdGFudGlu IFRva2FyZXYgIDxhbm51bGVuQHlhbmRleC5ydT4KIAogICAgICAgICBSZW1vdmUgZXhjZXNzaXZl IGhlYWRlcnMgZnJvbSBXZWJDb3JlL3tNb2R1bGVzLGFuaW1hdGlvbixjcnlwdG8sZG9taml0fQpk aWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvZ3N0cmVhbWVyL01l ZGlhUGxheWVyUHJpdmF0ZUdTdHJlYW1lckJhc2UuY3BwIGIvU291cmNlL1dlYkNvcmUvcGxhdGZv cm0vZ3JhcGhpY3MvZ3N0cmVhbWVyL01lZGlhUGxheWVyUHJpdmF0ZUdTdHJlYW1lckJhc2UuY3Bw CmluZGV4IDUzMGI2MzlhMmVjYTI0MzU1ODkyMzhlMGZlMjliNDcxMTY0ZDkzMGYuLmJlNTIxNTIx ODNmZWQ2ZDRkMWU3NzZhY2ViZjA1MzRmZWY2Zjg4MWUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL2dyYXBoaWNzL2dzdHJlYW1lci9NZWRpYVBsYXllclByaXZhdGVHU3RyZWFt ZXJCYXNlLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9nc3RyZWFt ZXIvTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyQmFzZS5jcHAKQEAgLTUwMCw3ICs1MDAsNyBA QCBib29sIE1lZGlhUGxheWVyUHJpdmF0ZUdTdHJlYW1lckJhc2U6Om11dGVkKCkgY29uc3QKICAg ICBpZiAoIW1fdm9sdW1lRWxlbWVudCkKICAgICAgICAgcmV0dXJuIGZhbHNlOwogCi0gICAgYm9v bCBtdXRlZDsKKyAgICBnYm9vbGVhbiBtdXRlZDsKICAgICBnX29iamVjdF9nZXQobV92b2x1bWVF bGVtZW50LmdldCgpLCAibXV0ZSIsICZtdXRlZCwgbnVsbHB0cik7CiAgICAgcmV0dXJuIG11dGVk OwogfQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvZ3N0cmVh bWVyL01lZGlhUGxheWVyUHJpdmF0ZUdTdHJlYW1lck93ci5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9w bGF0Zm9ybS9ncmFwaGljcy9nc3RyZWFtZXIvTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyT3dy LmNwcAppbmRleCA3NDFhNWM4NzYxNjA4YmY0NGI5ZjA2NDJmZDM1ODgyYmQ2YzVkMDIyLi5lNTFm ZjZjZjc3ZmUzYzE3NjhiYTkwNzEwODZjOTZlMmVkYzlhYTk1IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9nc3RyZWFtZXIvTWVkaWFQbGF5ZXJQcml2YXRlR1N0 cmVhbWVyT3dyLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9nc3Ry ZWFtZXIvTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyT3dyLmNwcApAQCAtMzYwLDggKzM2MCw4 IEBAIHZvaWQgTWVkaWFQbGF5ZXJQcml2YXRlR1N0cmVhbWVyT3dyOjp0cmFja0VuZGVkKE1lZGlh U3RyZWFtVHJhY2tQcml2YXRlJiB0cmFjaykKICAgICAgICAgICAgIHZpZGVvVHJhY2stPnNldFNl bGVjdGVkKGZhbHNlKTsKICAgICB9CiAKLSAgICBib29sIGF1ZGlvRGlzYWJsZWQ7Ci0gICAgYm9v bCB2aWRlb0Rpc2FibGVkOworICAgIGdib29sZWFuIGF1ZGlvRGlzYWJsZWQ7CisgICAgZ2Jvb2xl YW4gdmlkZW9EaXNhYmxlZDsKICAgICBnX29iamVjdF9nZXQobV9hdWRpb1JlbmRlcmVyLmdldCgp LCAiZGlzYWJsZWQiLCAmYXVkaW9EaXNhYmxlZCwgbnVsbHB0cik7CiAgICAgZ19vYmplY3RfZ2V0 KG1fdmlkZW9SZW5kZXJlci5nZXQoKSwgImRpc2FibGVkIiwgJnZpZGVvRGlzYWJsZWQsIG51bGxw dHIpOwogICAgIGlmIChhdWRpb0Rpc2FibGVkICYmIHZpZGVvRGlzYWJsZWQpIHsK