- Compiled
- Placed malformed XML in ~/.local/share/katepart5/syntax/
- Opened a file which uses that previously created highlighter
Details
Details
Diff Detail
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.
Comment Actions
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:2736Comment Actions
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()) {
Comment Actions
No, because defData isn't defined there yet, it's created only later with auto defData = DefinitionData::get(d->m_definition);
Comment Actions
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.
Comment Actions
are you sure we're allowed to call DefinitionData::get(d->m_definition); when m_definition->isValid isn't valid ?
Comment Actions
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
Comment Actions
:=) 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.
Comment Actions
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.
Comment Actions
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 :)
Comment Actions
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.
Comment Actions
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/
Comment Actions
I the unit test is separated, no longer all things will be tested with the invalid definitions.
Hmm, I disagree: then not all functions are tested with a broken file in the repo.
Comment Actions
As you wish.
Questions remains: does it fix https://bugs.kde.org/show_bug.cgi?id=401480
Comment Actions
No, will just crash later.
Therefore one needs a proper variant of https://phabricator.kde.org/D19533 in addition.