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

[Michael Pyne]

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)".

Regards,
 - Michael Pyne