Fix race condition in TextBuffer
AbandonedPublic

Authored by thomassc on Sep 4 2019, 4:36 PM.

Details

Reviewers
None
Group Reviewers
KTextEditor
Summary

In the "shortcut: try last block first" branch in TextBuffer::blockForLine(),
m_lastUsedBlock is accessed multiple times. If another tread simultaneously
executes the binary block search below in the same function, then
m_lastUsedBlock may be modified by that thread. This may result in the first
thread returning a wrong block, since it may return the modified value of
m_lastUsedBlock instead of the original value that it used to determine whether
the given line is in the block.

Also, it seems like it may be safer to use std::atomic for m_lastUsedBlock,
but I am not sure about that.

This change is intended to fix a crash where an out-of-bounds line is accessed
in katetextblock.cpp, which occurs about one or two times per month for me when
using KDevelop. I have no idea whether this possible race condition is actually
the reason for these crashes, but it seems like a plausible candidate.

Test Plan

I only tested that the code still works after the change.

Diff Detail

Repository
R39 KTextEditor
Branch
fix_katetextbuffer_race_condition
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16088
Build 16106: arc lint + arc unit
thomassc created this revision.Sep 4 2019, 4:36 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptSep 4 2019, 4:36 PM
Restricted Application added subscribers: kde-frameworks-devel, kwrite-devel. · View Herald Transcript
thomassc requested review of this revision.Sep 4 2019, 4:36 PM
thomassc edited the summary of this revision. (Show Details)Sep 4 2019, 4:37 PM
thomassc added a reviewer: KTextEditor.

Hmm, the use of atomic + a local var might avoid that the compiler optimizes away the local var and just uses m_lastUsedBlock again directly, but I am not sure if we should go that way.
If one has concurrent accesses to the buffer from different threads, all is lost, as no part of the buffer is really thread save. (e.g. it will still random crash if the buffer is modified in the main thread).

Okay, I wasn't aware of the overall situation wrt. threading. Still, couldn't it be useful to at least allow for concurrent read accesses which is probably not a large effort (in contrast to concurrent read and write)? I guess that people might not expect blockForLine() to not be re-entrant, and it seems like the proposed robustification might not add too much overhead for performance and code complexity.

KTextEditor is nowhere designed to be thread-safe. Even if this patch solves a particular issue you have in multithreaded access, we can be sure there will be other (corner?) cases where it won't work: Think only of modifying the text at some other place at the same time. It simply does not work right now. That said: Please always access KTextEditor API only from the main thread, and none else.

-1 from my side.

thomassc abandoned this revision.Sep 4 2019, 8:04 PM

Okay, thanks for your feedback. Discarding this then. I think I might have found the problematic access in KDevelop already in the meantime ...