Don't crash when no contexts are present
ClosedPublic

Authored by davispuh on Mar 4 2019, 10:15 PM.

Details

Test Plan
  1. Compiled
  2. Placed malformed XML in ~/.local/share/katepart5/syntax/
  3. Opened a file which uses that previously created highlighter

Diff Detail

Repository
R216 Syntax Highlighting
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
davispuh created this revision.Mar 4 2019, 10:15 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptMar 4 2019, 10:15 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
davispuh requested review of this revision.Mar 4 2019, 10:15 PM

Creating a syntax highlighting file with few unclosed tags made Kate to crash. No matter how malformed XML it loads, editor should never crash as it can cause loss of unsaved work.

Crash is caused by

Context* DefinitionData::initialContext() const
{
    Q_ASSERT(!contexts.isEmpty());
    return contexts.first();
}

which is called from

State AbstractHighlighter::highlightLine(const QString& text, const State &state)
{
    Q_D(AbstractHighlighter);

    // verify definition, deal with no highlighting being enabled
    d->ensureDefinitionLoaded();
    if (!d->m_definition.isValid()) {
        applyFormat(0, text.size(), Format());
        return State();
    }

    // verify/initialize state
    auto defData = DefinitionData::get(d->m_definition);
    auto newState = state;
    auto stateData = StateData::get(newState);
    const DefinitionRef currentDefRef(d->m_definition);
    if (!stateData->isEmpty() && (stateData->m_defRef != currentDefRef)) {
        qCDebug(Log) << "Got invalid state, resetting.";
        stateData->clear();
    }
    if (stateData->isEmpty()) {
        stateData->push(defData->initialContext(), QStringList());
        stateData->m_defRef = currentDefRef;
    }

here's the backtrace itself

#0  0x00007fe9863b4d7f in raise () from /usr/lib/libc.so.6
#1  0x00007fe98639f672 in abort () from /usr/lib/libc.so.6
#2  0x00007fe9867567fc in QMessageLogger::fatal(char const*, ...) const () from /usr/lib/libQt5Core.so.5
#3  0x00007fe986755c28 in qt_assert(char const*, char const*, int) () from /usr/lib/libQt5Core.so.5
#4  0x00007fe985e966f3 in KSyntaxHighlighting::DefinitionData::initialContext (this=<optimized out>) from /usr/lib/libKF5SyntaxHighlighting.so.5
#5  0x00007fe985e981c2 in KSyntaxHighlighting::AbstractHighlighter::highlightLine (this=this@entry=0x55c256cba090, text=..., state=...)
    at /usr/include/qt/QtCore/qlist.h:151
#6  0x00007fe988464b6f in KateHighlighting::doHighlight (this=0x55c256cba090, prevLine=<optimized out>, textLine=0x55c25690cb70, 
    nextLine=nextLine@entry=0x55c2569b7420, ctxChanged=@0x7ffe2d101ae3: false, tabWidth=8)
    at /usr/src/debug/ktexteditor/src/buffer/katetextline.h:311
#7  0x00007fe9883facbd in KateBuffer::doHighlight (invalidate=<optimized out>, endLine=<optimized out>, startLine=<optimized out>, 
    this=0x55c257a097d0) at /usr/src/debug/ktexteditor/src/document/katebuffer.h:203
#8  KateBuffer::doHighlight (this=0x55c257a097d0, startLine=<optimized out>, endLine=<optimized out>, invalidate=<optimized out>)
    at /usr/src/debug/ktexteditor/src/document/katebuffer.cpp:360
#9  0x00007fe9883d8cf4 in KTextEditor::DocumentPrivate::kateTextLine (this=0x55c255de2c10, i=0)
    at /usr/src/debug/ktexteditor/src/document/katedocument.cpp:5249
#10 0x00007fe98844a0a5 in KateLineLayout::textLine (this=0x55c256b75980, reloadForce=reloadForce@entry=false)
    at /usr/src/debug/ktexteditor/src/render/katelinelayout.cpp:79
#11 0x00007fe988440350 in KateRenderer::layoutLine (this=0x55c256cef2a0, lineLayout=..., maxwidth=-1, cacheLayout=<optimized out>)
    at /usr/src/debug/ktexteditor/src/render/katerenderer.cpp:1021
#12 0x00007fe988444591 in KateLayoutCache::line (this=0x55c256cee360, realLine=0, virtualLine=<optimized out>)
    at /usr/include/c++/8.2.1/bits/atomic_base.h:295
#13 0x00007fe988446a6a in KateLayoutCache::updateViewCache (this=this@entry=0x55c256cee360, startPos=..., 
    newViewLineCount=newViewLineCount@entry=40, viewLinesScrolled=viewLinesScrolled@entry=0)
    at /usr/src/debug/ktexteditor/src/render/katelayoutcache.cpp:242
#14 0x00007fe98848916b in KateViewInternal::updateView (this=0x55c255f63a50, changed=true, viewLinesScrolled=viewLinesScrolled@entry=0)
    at /usr/src/debug/ktexteditor/src/include/ktexteditor/cursor.h:98
#15 0x00007fe98846f3a7 in KTextEditor::ViewPrivate::updateView (this=0x55c255f30f40, changed=<optimized out>)
    at /usr/src/debug/ktexteditor/src/view/kateview.cpp:2047
#16 0x00007fe9883dc786 in KTextEditor::DocumentPrivate::makeAttribs (this=this@entry=0x55c255de2c10, needInvalidate=needInvalidate@entry=false)
    at /usr/src/debug/ktexteditor/src/document/katedocument.cpp:2910
#17 0x00007fe9883dc7fc in KTextEditor::DocumentPrivate::bufferHlChanged (this=0x55c255de2c10)
    at /usr/src/debug/ktexteditor/src/document/katedocument.cpp:1876
#18 0x00007fe9883faaad in KateBuffer::setHighlight (this=0x55c257a097d0, hlMode=<optimized out>)
    at /usr/src/debug/ktexteditor/src/document/katebuffer.cpp:346
#19 0x00007fe9883ec4ff in KTextEditor::DocumentPrivate::updateFileType (this=this@entry=0x55c255de2c10, newType=..., user=user@entry=false)
    at /usr/src/debug/ktexteditor/src/document/katedocument.cpp:5061
#20 0x00007fe9883ef5b4 in KTextEditor::DocumentPrivate::openFile (this=0x55c255de2c10) at /usr/src/debug/ktexteditor/src/utils/kateglobal.h:252
#21 0x00007fe988281873 in ?? () from /usr/lib/libKF5Parts.so.5
#22 0x00007fe98828286f in KParts::ReadOnlyPart::openUrl(QUrl const&) () from /usr/lib/libKF5Parts.so.5
#23 0x00007fe9883debf2 in KTextEditor::DocumentPrivate::openUrl (this=0x55c255de2c10, url=...) at /usr/src/debug/ktexteditor/src/document/katedocument.cpp:2736
davispuh updated this revision to Diff 53175.Mar 4 2019, 11:20 PM

Actually fix it

Hi, first: good thing to get this fixed, yes, it shall not crash.

For the fix: Wouldn't it be enough to just change the

if (!d->m_definition.isValid()) {

applyFormat(0, text.size(), Format());

​ ...

shortcut to check for

if (!d->m_definition.isValid() || defData->isLoaded()) {

Hi, first: good thing to get this fixed, yes, it shall not crash.

For the fix: Wouldn't it be enough to just change the

if (!d->m_definition.isValid()) {

applyFormat(0, text.size(), Format());

​ ...

shortcut to check for

if (!d->m_definition.isValid() || defData->isLoaded()) {

No, because defData isn't defined there yet, it's created only later with auto defData = DefinitionData::get(d->m_definition);

No, because defData isn't defined there yet, it's created only later with auto defData = DefinitionData::get(d->m_definition);

> you can just move that up some lines, or?

I actually have taken a short look, perhaps a issue is already that "isValid()" is true even if we fail to load anything useful.

This would be my proposed patch + unit test.

davispuh added a comment.EditedMar 5 2019, 7:11 PM

This would be my proposed patch + unit test.

are you sure we're allowed to call DefinitionData::get(d->m_definition); when m_definition->isValid isn't valid ?

Calling the ::get is no issue.

Calling the ::get is no issue.

That's good then, but we can't use applyFormat(0, text.size(), Format()); because Kate asserts when !Format->isValid() which it won't be so we have to return State(); and also see D19533

Calling the ::get is no issue.

That's good then, but we can't use applyFormat(0, text.size(), Format()); because Kate asserts when !Format->isValid() which it won't be so we have to return State(); and also see D19533

:=) I guess we just need to if (!format.isValid()) return; in the KTextEditor code, seems I didn't read the docs of applyFormat, it just works by chance ATM as I filter out !isValid() definitions there ATM.

davispuh updated this revision to Diff 53234.Mar 5 2019, 8:21 PM

Improvements

Ok with that.
Volker, ok, too?
I think the if(...isLoaded) removal is a diff artifact, that was never in the original code bug just in the first try here.

Ok with that.
Volker, ok, too?
I think the if(...isLoaded) removal is a diff artifact, that was never in the original code bug just in the first try here.

Yep, replacing the assert certainly makes sense. This would assume though that the caller of initalContext() expects a nullptr return value already, which I guess is the case if this doesn't crash somewhere else now :)

cullmann requested changes to this revision.Mar 6 2019, 8:05 PM

Ok with that.
Volker, ok, too?
I think the if(...isLoaded) removal is a diff artifact, that was never in the original code bug just in the first try here.

Yep, replacing the assert certainly makes sense. This would assume though that the caller of initalContext() expects a nullptr return value already, which I guess is the case if this doesn't crash somewhere else now :)

Hmm, actually that part of the diff is not needed, either.
No caller is able to handle nullptrs, but that just doesn't happen.

I would like to have that change removed, then we can apply this.

This revision now requires changes to proceed.Mar 6 2019, 8:05 PM

Could you change the unit test please to not mix with the other ones?

  • add a new void testBrokenDefinition()
  • use a local Repository repo;
  • add custom search oath to this repo
  • and then check only the broken file.

This would be much cleaner and better to understand & maintain. Maybe even put broken.xml into autotest/input/syntax/broken/

I the unit test is separated, no longer all things will be tested with the invalid definitions.

Could you change the unit test please to not mix with the other ones?

  • add a new void testBrokenDefinition()
  • use a local Repository repo;
  • add custom search oath to this repo
  • and then check only the broken file.

    This would be much cleaner and better to understand & maintain. Maybe even put broken.xml into autotest/input/syntax/broken/

Hmm, I disagree: then not all functions are tested with a broken file in the repo.

As you wish.

Questions remains: does it fix https://bugs.kde.org/show_bug.cgi?id=401480

As you wish.

Questions remains: does it fix https://bugs.kde.org/show_bug.cgi?id=401480

No, will just crash later.
Therefore one needs a proper variant of https://phabricator.kde.org/D19533 in addition.

cullmann accepted this revision.Mar 9 2019, 3:11 PM

I will commit the minimal needed fix to get this solved.

This revision is now accepted and ready to land.Mar 9 2019, 3:11 PM
This revision was automatically updated to reflect the committed changes.