Problem investigating a heap-use-after-free in kwindowsystem

[Martin Graesslin]

On Friday, October 2, 2015 8:38:20 PM CEST Michael Pyne wrote:
> On Fri, October 2, 2015 23:59:00 Albert Astals Cid wrote:
> > El Friday 02 October 2015, a les 22:24:00, Martin Graesslin va escriure:
> > > Hi all,
> > > 
> > > I added a new unit test to kwin today [1] and it failed on the CI system
> > > due to the enabled ASAN with an error in kwindowsystem. I'm able to
> > > reproduce it locally, but fail to understand it. To me the code and the
> > > test conditions looks correct. Relevant part of the bt:
> > > #0 0x7f5198ac2c84 in strncpy
> > > (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x73c84) #1 0x7f5190b2be24 in
> > > nstrndup /home/martin/src/kf5/frameworks/
> > > kwindowsystem/src/platforms/xcb/netwm.cpp:110
> > > 
> > >     #2 0x7f5190b425ff in NETRootInfo::update(QFlags<NET::Property>,
> > > 
> > > QFlags<NET::Property2>)
> > > /home/martin/src/kf5/frameworks/kwindowsystem/src/
> > > platforms/xcb/netwm.cpp:2142
> > > 
> > >     #3 0x7f5190b2f8bf in NETRootInfo::activate() /home/martin/src/kf5/
> > > 
> > > frameworks/kwindowsystem/src/platforms/xcb/netwm.cpp:614
> > > 
> > >     #4 0x7f519852be87 in KWin::VirtualDesktopManager::load()
> > > 
> > > /home/martin/src/ kf5/kde/workspace/kwin/virtualdesktops.cpp:348
> > > 
> > > Full backtrace in [2]. The content of the list names is according to
> > > qDebug: ("Desktop 1"). From trying with qdebug it seems to fail in the
> > > first iteration of the for loop.
> > > 
> > > Any help appreciated to get that fixed more than appreciated (I like my
> > > tests green ;-) ).
> 
> Look at the definition of get_stringlist_reply in xcb/netwm.cpp:
> 
>     if (reply->type == type && reply->format == 8 && reply->value_len > 0) {
> const char *data = (const char *) xcb_get_property_value(reply); int len =
> reply->value_len;
> 
>         if (data) {
>             const QByteArray ba = QByteArray::fromRawData(data, data[len -
> 1] ? len : len - 1);
>             list = ba.split('\0');
>         }
>     }
> 
>     free(reply);
> 
> QByteArray::fromRawData is highly dangerous here: proper use requires that
> the source of the 'raw data' is kept alive for the entirety of the new
> QByteArray's own lifetime.
> 
> Of course in this case it appears that we immediately use the new QByteArray
> to split into a list, with what will presumably be new QByteArrays
> constructed using the normal (copying) constructor, and that therefore this
> is simply a safe local optimization.
> 
> But look at QByteArray::split()'s API docs: "If sep [\0 here] does not match
> anywhere in the byte array, split() returns a single-element list
> containing ***this*** byte array." (emphasis mine).
> 
> I'm willing to bet you're running into a case where libxcb is sending a
> reply that doesn't contain '\0' (after all, the resultant QList<> contains
> only 1 item), which ends up in a QByteArray that is ::fromRawData()'d, but
> had the source of that data freed at "free(reply)".

This could in deed be the case. The autotests of KWindowSystem cover the code 
in question and do not fail there. So the normal condition seems to work.

> 
> Regards,
>  - Michael Pyne
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20151003/cdb31003/attachment.sig>