<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/128761/">https://git.reviewboard.kde.org/r/128761/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 27th, 2016, 9:14 a.m. UTC, <b>Hugo Pereira Da Costa</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">mmm. But then i think it is better (for commit history etc), to just revert the incriminated commit (and in oxygen as well), with a possible link to this RB. makes sense ?</p></pre>
</blockquote>
<p>On August 27th, 2016, 9:30 a.m. UTC, <b>Peter Wu</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I tested the reverts for breeze and oxygen and that also works on Qt 5.7.0 (tested with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">wireshark -o</code> (<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">exit()</code>s), <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">wireshark</code> + quit and the testcase).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Reverted:
breeze 1430dd22ae74a09ba289dafe2be15628a0eddc15
oxygen e0376e1597f6e6b49e6343874e141b439565b749</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Due to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">delete qApp->style()</code> however, it is in theory still possible to reach a use-after-free since QApplication owns it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'll leave it up to you whether you take this patch or revert it, I'm fine with either.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">i agree, obviously delete qApp->Style() must go too, and in fact that was the original patch introduced by D Faure, to oxygen (commit 2ffe20e1bfe93c921c5372b4d21447b1de308d4b), which i then copied to breeze with many other changes and wich was later on changed to the signal/slot delta function by D Edmundson.
So in the end, maybe it is indeed better to clean it all up in one go, possibly adding the relevant effectively reverted commits in the comments.</p></pre>
<br />
<p>- Hugo</p>
<br />
<p>On August 27th, 2016, 9:12 a.m. UTC, Peter Wu wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Plasma, David Edmundson, David Faure, and Hugo Pereira Da Costa.</div>
<div>By Peter Wu.</div>
<p style="color: grey;"><i>Updated Aug. 27, 2016, 9:12 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=356940">356940</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
oxygen
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Since Qt 5.6.0, Qt5 applications started crashing on exit. All signs
point to this delete-on-destroy hack which was added to avoid outliving
the plugin lifetime.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This method is wrong because the returned style is owned by the caller
(QApplication, QProxyStyle, etc) and will cleaned up when those users
are destructed.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Copied from breeze patch https://git.reviewboard.kde.org/r/128760/</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Started <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QT_STYLE_OVERRIDE=oxygen LD_LIBRARY_PATH=... QT_PLUGIN_PATH=... wireshark -o</code> (an invalid option that triggers <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">exit(1)</code>) and observe a heap-use-after free similar to the one reported in the bug. Apply this patch, rebuild oxygen and notice that the crash is fixed. Also tested with "Testcase (ASAN)" from bug 356940, crash is also gone.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kstyle/oxygenstyleplugin.cpp <span style="color: grey">(70b90d9)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128761/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>