Bug 324013 - NULL pointer dereference in KPtyDevicePrivate::doWait
Summary: NULL pointer dereference in KPtyDevicePrivate::doWait
Status: RESOLVED WORKSFORME
Alias: None
Product: kdelibs
Classification: Frameworks and Libraries
Component: kdecore (show other bugs)
Version: 4.10.5
Platform: Compiled Sources Linux
: NOR crash
Target Milestone: ---
Assignee: kdelibs bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-25 12:44 UTC by Christopher Yeleighton
Modified: 2021-01-16 04:34 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
check readNotifier (1.32 KB, patch)
2013-08-25 13:52 UTC, Christopher Yeleighton
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Yeleighton 2013-08-25 12:44:44 UTC
The expression (readNotifier->isEnabled()) at kpty/kptydevice.cpp:414 dereferences a NULL pointer (readNotifier).


Reproducible: Always

Steps to Reproduce:
  1. /home/abuild/rpmbuild/BUILD/kdelibs-4.10.5/build/kpty/tests/kptyprocesstest

Actual Results:  
  1. 
qttest(8225)/kdecore (KPty/K3Process) KPty::open: Can't open a pseudo teletype 
********* Start testing of KPtyProcessTest *********
Config: Using QTest library 4.8.5, Qt 4.8.5
PASS   : KPtyProcessTest::initTestCase()
QDEBUG : KPtyProcessTest::test_pty_basic() qttest(8225)/kdecore (KPty/K3Process) KPty::open: Can't open a pseudo teletype 
QFATAL : KPtyProcessTest::test_pty_basic() Received signal 11
FAIL!  : KPtyProcessTest::test_pty_basic() Received a fatal error.
   Loc: [Unknown file(0)]
Totals: 1 passed, 1 failed, 0 skipped
********* Finished testing of KPtyProcessTest *********


Expected Results:  
  1. Let the test fail without crashing.

#0  0x00007ffff69783d5 in __GI_raise (sig=sig@entry=6)
    at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x00007ffff6979858 in __GI_abort () at abort.c:90
#2  0x00007ffff706dc54 in qt_message_output (msgType=msgType@entry=QtFatalMsg,
    buf=<optimized out>) at global/qglobal.cpp:2323
#3  0x00007ffff706de08 in qt_message(QtMsgType, const char *, typedef __va_list_tag __va_list_tag *) (msgType=msgType@entry=QtFatalMsg, msg=
    0x7ffff79bb1e0 "Received signal %d", ap=ap@entry=0x7fffffffcb18)
    at global/qglobal.cpp:2369
#4  0x00007ffff706df94 in qFatal (msg=<optimized out>)
    at global/qglobal.cpp:2552
#5  0x00007ffff79a90e4 in QTest::FatalSignalHandler::signal (signum=<optimized out>) at qtestcase.cpp:1729
#6  <signal handler called>
#7  0x00007ffff7bd6e84 in KPtyDevicePrivate::doWait (this=0x651c40, msecs=<optimized out>, reading=true)
    at /home/abuild/rpmbuild/BUILD/kdelibs-4.10.5/build/kpty/kptydevice.moc:108
#8  0x00000000004036ed in KPtyProcessTest::test_pty_basic (
    this=<optimized out>)
    at /home/abuild/rpmbuild/BUILD/kdelibs-4.10.5/kpty/tests/kptyprocesstest.cpp:134
#9  0x00007ffff717be57 in QMetaMethod::invoke (this=this@entry=0x7fffffffd4e0,
    object=object@entry=0x7fffffffdf00, connectionType=connectionType@entry=
    Qt::DirectConnection, returnValue=..., val0=..., val1=..., val2=..., val3=
    ..., val4=..., val5=..., val6=..., val7=..., val8=..., val9=...)
    at kernel/qmetaobject.cpp:1664
#10 0x00007ffff717e22c in QMetaObject::invokeMethod (obj=0x7fffffffdf00,
    member=<optimized out>, type=Qt::DirectConnection, ret=..., val0=..., val1=
    ..., val2=..., val3=..., val4=..., val5=..., val6=..., val7=..., val8=...,
    val9=...) at kernel/qmetaobject.cpp:1179
#11 0x00007ffff79ac2e2 in invokeMethod (val9=..., val8=..., val7=..., val6=
    ..., val5=..., val4=..., val3=..., val2=..., val1=..., val0=..., type=
    Qt::DirectConnection, member=0x65f560 "test_pty_basic",
    obj=<optimized out>) at ../../src/corelib/kernel/qobjectdefs.h:418
#12 qInvokeTestMethodDataEntry (slot=0x65f560 "test_pty_basic")
    at qtestcase.cpp:1423
#13 QTest::qInvokeTestMethod (slotName=
    0x4051f1 <qt_meta_stringdata_KPtyProcessTest+17> "test_pty_basic()",
    data=data@entry=0x0) at qtestcase.cpp:1531
---Type <return> to continue, or q <return> to quit---f 7
#14 0x00007ffff79ad177 in qInvokeTestMethods (testObject=0x7fffffffdf00)
    at qtestcase.cpp:1696
#15 QTest::qExec (testObject=0x7fffffffdf00, argc=<optimized out>,
    argv=<optimized out>) at qtestcase.cpp:1919
#16 0x0000000000402e3a in main (argc=1, argv=0x7fffffffe068)
    at /home/abuild/rpmbuild/BUILD/kdelibs-4.10.5/kpty/tests/kptyprocesstest.cpp:220
Comment 1 Christopher Yeleighton 2013-08-25 13:52:39 UTC
Created attachment 81919 [details]
check readNotifier
Comment 2 Michael Pyne 2013-08-26 01:33:08 UTC
Most of KPtyDevice seems to pay no mind to whether readNotifier is valid, as the object is created as part of opening the PTY. The only exception seems to be if there is already a master file descriptor setup.

So I think the proposed fix is inappropriate, as presumably many more null checks would be needed.

I believe the actual bug is in the testcase itself. KPtyProcess::start() is really KProcess::start(), which is documented as starting the process, waiting for it to complete, and returning the exit code.

When utmp support is enabled, the transition from a running process to a finished process would cause the PTY to be logged out, which invalidates all socket notifiers (including readNotifier). In other words the process was already allowed to run to completion and so we don't bother with its PTY anymore; the underlying KPtyDevice is essentially in an invalid state.

I believe the test case should use p.execute() instead of p.start() with the rest of the testcase being more-or-less satisfactory the way it is. I've CC'ed the KPty dev to double-check my logic though.
Comment 3 Oswald Buddenhagen 2013-08-26 07:45:00 UTC
the test log indicates quite clearly that the root cause is a failure to open a pty.
the test should do QVERIFY(p.pty()->isOpen()); right after instantiating the KPtyProcess.
actual user code of KPtyProcess (konsole in particular) should be checked whether it does this check, too.
Comment 4 Christopher Yeleighton 2013-08-29 17:54:40 UTC
(In reply to comment #3)
> the test log indicates quite clearly that the root cause is a failure to
> open a pty.
> the test should do QVERIFY(p.pty()->isOpen()); right after instantiating the
> KPtyProcess.
> actual user code of KPtyProcess (konsole in particular) should be checked
> whether it does this check, too.

Library code should not rely on the customer observing complicated conditions and transitions or otherwise we will crash.  I understand that is sometimes necessary for efficiency reasons (e.g. invalid iterators) but I think this is not the case this time.
Comment 5 Oswald Buddenhagen 2013-08-29 18:10:45 UTC
it's unreasonable to harden the code against each possible abuse. latest when memory corruption comes into play, there is no way to get it right.
the correct reaction would be refusing to start the process, a condition which is more likely to be tested by the user.
Comment 6 Christopher Yeleighton 2013-08-30 22:10:50 UTC
(In reply to comment #5)
> it's unreasonable to harden the code against each possible abuse. latest
> when memory corruption comes into play, there is no way to get it right.
> the correct reaction would be refusing to start the process, a condition
> which is more likely to be tested by the user.

A NULL pointer is not a manifestation of corrupted memory; just the opposite, it is commonly used to indicate no value.
Comment 7 Justin Zobel 2020-12-17 05:23:01 UTC
Thank you for the crash report.

As it has been a while since this was reported, can you please test and confirm if this issue is still occurring or if this bug report can be marked as resolved.

I have set the bug status to "needsinfo" pending your response, please change back to "reported" or "resolved/worksforme" when you respond, thank you.
Comment 8 Bug Janitor Service 2021-01-01 04:34:53 UTC
Dear Bug Submitter,

This bug has been in NEEDSINFO status with no change for at least
15 days. Please provide the requested information as soon as
possible and set the bug status as REPORTED. Due to regular bug
tracker maintenance, if the bug is still in NEEDSINFO status with
no change in 30 days the bug will be closed as RESOLVED > WORKSFORME
due to lack of needed information.

For more information about our bug triaging procedures please read the
wiki located here:
https://community.kde.org/Guidelines_and_HOWTOs/Bug_triaging

If you have already provided the requested information, please
mark the bug as REPORTED so that the KDE team knows that the bug is
ready to be confirmed.

Thank you for helping us make KDE software even better for everyone!
Comment 9 Bug Janitor Service 2021-01-16 04:34:11 UTC
This bug has been in NEEDSINFO status with no change for at least
30 days. The bug is now closed as RESOLVED > WORKSFORME
due to lack of needed information.

For more information about our bug triaging procedures please read the
wiki located here:
https://community.kde.org/Guidelines_and_HOWTOs/Bug_triaging

Thank you for helping us make KDE software even better for everyone!