Bug 432376

Created attachment 135350 [details]
double line height, also seems to randomly remove the text entirely sometimes

Just to clarify the title: this is related to the new "reflowing" of the scroll history thing.

just tried to build with asan and ubsan and it crashes immediately when it tries to reflow, assuming it is related:

==131591==ERROR: AddressSanitizer: heap-use-after-free on address 0x60800002f520 at pc 0x7f38df17a22b bp 0x7ffc78b00f80 sp 0x7ffc78b00f70
READ of size 1 at 0x60800002f520 thread T0
    #0 0x7f38df17a22a in QVarLengthArray<unsigned char, 64>::insert(unsigned char const*, int, unsigned char const&) (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x167c22a)
    #1 0x7f38df16c48c in QVarLengthArray<unsigned char, 64>::insert(int, unsigned char const&) (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x166e48c)
    #2 0x7f38df13c12d in Konsole::Screen::resizeImage(int, int) ../src/Screen.cpp:479
    #3 0x7f38df0b9698 in Konsole::Emulation::setImageSize(int, int) ../src/Emulation.cpp:317
    #4 0x7f38deed7843 in Konsole::Session::updateTerminalSize() ../src/session/Session.cpp:753
    #5 0x7f38deed6495 in Konsole::Session::onViewSizeChange(int, int) ../src/session/Session.cpp:726
    #6 0x7f38def1afe5 in QtPrivate::FunctorCall<QtPrivate::IndexesList<0, 1>, QtPrivate::List<int, int>, void, void (Konsole::Session::*)(int, int)>::call(void (Konsole::Session::*)(int, int), Konsole::Session*, void**) /usr/include/qt/QtCore/qobjectdefs_impl.h:152

0x60800002f520 is located 0 bytes inside of 89-byte region [0x60800002f520,0x60800002f579)
freed by thread T0 here:
    #0 0x7f38e0f9f0e9 in __interceptor_free /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:123
    #1 0x7f38df171e80 in QVarLengthArray<unsigned char, 64>::realloc(int, int) (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x1673e80)
    #2 0x7f38df1683fb in QVarLengthArray<unsigned char, 64>::resize(int) (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x166a3fb)
    #3 0x7f38df17a1d8 in QVarLengthArray<unsigned char, 64>::insert(unsigned char const*, int, unsigned char const&) (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x167c1d8)
    #4 0x7f38df16c48c in QVarLengthArray<unsigned char, 64>::insert(int, unsigned char const&) (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x166e48c)
    #5 0x7f38df13c12d in Konsole::Screen::resizeImage(int, int) ../src/Screen.cpp:479
    #6 0x7f38df0b9698 in Konsole::Emulation::setImageSize(int, int) ../src/Emulation.cpp:317

previously allocated by thread T0 here:
    #0 0x7f38e0f9f459 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:145
    #1 0x7f38df171901 in QVarLengthArray<unsigned char, 64>::realloc(int, int) (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x1673901)
    #2 0x7f38df1683fb in QVarLengthArray<unsigned char, 64>::resize(int) (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x166a3fb)
    #3 0x7f38df13da7b in Konsole::Screen::resizeImage(int, int) ../src/Screen.cpp:511
    #4 0x7f38df0b9698 in Konsole::Emulation::setImageSize(int, int) ../src/Emulation.cpp:317

It does 'insert' in array size(), it is intended to, and it will never crash because when it is size() the 'insert' will turn into 'append', it is in 'QVarLengthArray' class documentation.
Characters in konsole doesn't have high or size attribute to be changed in reflow.

I'll try to replicate and test this bug, here.

(In reply to Martin Sandsmark from comment #3)
> just tried to build with asan and ubsan and it crashes immediately when it
> tries to reflow, assuming it is related:
> 
> ==131591==ERROR: AddressSanitizer: heap-use-after-free on address
> 0x60800002f520 at pc 0x7f38df17a22b bp 0x7ffc78b00f80 sp 0x7ffc78b00f70
> READ of size 1 at 0x60800002f520 thread T0
>     #0 0x7f38df17a22a in QVarLengthArray<unsigned char, 64>::insert(unsigned
> char const*, int, unsigned char const&)
> (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x167c22a)
>     #1 0x7f38df16c48c in QVarLengthArray<unsigned char, 64>::insert(int,
> unsigned char const&)
> (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x166e48c)
>     #2 0x7f38df13c12d in Konsole::Screen::resizeImage(int, int)
> ../src/Screen.cpp:479
>     #3 0x7f38df0b9698 in Konsole::Emulation::setImageSize(int, int)
> ../src/Emulation.cpp:317
>     #4 0x7f38deed7843 in Konsole::Session::updateTerminalSize()
> ../src/session/Session.cpp:753
>     #5 0x7f38deed6495 in Konsole::Session::onViewSizeChange(int, int)
> ../src/session/Session.cpp:726
>     #6 0x7f38def1afe5 in QtPrivate::FunctorCall<QtPrivate::IndexesList<0,
> 1>, QtPrivate::List<int, int>, void, void (Konsole::Session::*)(int,
> int)>::call(void (Konsole::Session::*)(int, int), Konsole::Session*, void**)
> /usr/include/qt/QtCore/qobjectdefs_impl.h:152
> 
> 0x60800002f520 is located 0 bytes inside of 89-byte region
> [0x60800002f520,0x60800002f579)
> freed by thread T0 here:
>     #0 0x7f38e0f9f0e9 in __interceptor_free
> /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:123
>     #1 0x7f38df171e80 in QVarLengthArray<unsigned char, 64>::realloc(int,
> int) (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x1673e80)
>     #2 0x7f38df1683fb in QVarLengthArray<unsigned char, 64>::resize(int)
> (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x166a3fb)
>     #3 0x7f38df17a1d8 in QVarLengthArray<unsigned char, 64>::insert(unsigned
> char const*, int, unsigned char const&)
> (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x167c1d8)
>     #4 0x7f38df16c48c in QVarLengthArray<unsigned char, 64>::insert(int,
> unsigned char const&)
> (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x166e48c)
>     #5 0x7f38df13c12d in Konsole::Screen::resizeImage(int, int)
> ../src/Screen.cpp:479
>     #6 0x7f38df0b9698 in Konsole::Emulation::setImageSize(int, int)
> ../src/Emulation.cpp:317
> 
> previously allocated by thread T0 here:
>     #0 0x7f38e0f9f459 in __interceptor_malloc
> /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cpp:145
>     #1 0x7f38df171901 in QVarLengthArray<unsigned char, 64>::realloc(int,
> int) (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x1673901)
>     #2 0x7f38df1683fb in QVarLengthArray<unsigned char, 64>::resize(int)
> (/home/sandsmark/src/konsole/build/bin/libkdeinit5_konsole.so+0x166a3fb)
>     #3 0x7f38df13da7b in Konsole::Screen::resizeImage(int, int)
> ../src/Screen.cpp:511
>     #4 0x7f38df0b9698 in Konsole::Emulation::setImageSize(int, int)
> ../src/Emulation.cpp:317

Understanding how highlight selection is: the display array is not the same as the "memory" (screen+history vectors) it is a copy with window lines size of the scroll position you are in "memory". After copy information to the array, it will invert the color of your selection in this copy, your selection color is not saved in the "memory".

Ex.: Suppose you have 1030 lines "memory" and a windows with 30 lines, your scroll is in 0 the display array have 30 lines and copy from 0 to 29. 
If your scroll is in 31, the display array have 30 lines and copy from 31 to 60.

How is it possible to keep the highlight? I don't know, my guess is your konsole window image is not being updated the way it should somehow. Unfortunately I don't know yet how to reproduce the bug. But I'll keep trying.

Easiest way to reproduce is to select some text, and then resize the window small enough so that it has to rewrap/reflow the selected text (in one go, because resizing clears the selection, which I have been putting off fixing for some time..)

Just because I love ugly oneliners, run this:

   echo $(printf 'a%.0s' $(seq 1 $(($(stty size | cut -d\  -f2) - 1))))

Then double click the a's to select them, and then resize the terminal a couple of columns smaller

Never mind me, this is independent of selection.

I just need to shrink the window after running my oneliner to crash konsole (assuming konsole is built with asan/ubsan, that is, otherwise you just get invisble random memory corruption)

I can confirm the crash when built w/ubsan.

(In reply to Carlos Alves from comment #4)
> It does 'insert' in array size(), it is intended to, and it will never crash
> because when it is size() the 'insert' will turn into 'append', it is in
> 'QVarLengthArray' class documentation.
> Characters in konsole doesn't have high or size attribute to be changed in
> reflow.

Not sure if I understood this, but I assume the problem on line 479 is the _lineProperties[currentPos] passed to insert, not the insert itself. I. e. _lineProperties.size() is less than _screenLines.size(), I think.

Not sure what "auto values" there is (this is why I don't like "auto"), but the insert with that might be getting them out of sync, if there's more than one value being inserted?

(In reply to Carlos Alves from comment #5)
> How is it possible to keep the highlight? I don't know, my guess is your
> konsole window image is not being updated the way it should somehow.


As I said earlier, the highlight is supposed to be cleared when resizing (to avoid complex logic), so I assume the reason for highlight not being cleared as well as suddenly getting double height characters is because we use out of bounds values in _lineProperties.

(In reply to Martin Sandsmark from comment #10)
> Not sure if I understood this, but I assume the problem on line 479 is the
> _lineProperties[currentPos] passed to insert, not the insert itself. I. e.
> _lineProperties.size() is less than _screenLines.size(), I think.
> 
> Not sure what "auto values" there is (this is why I don't like "auto"), but
> the insert with that might be getting them out of sync, if there's more than
> one value being inserted?

Can you test it?
Maybe on line 478:
if (_screenLines.size() != _lineProperties.size() || currentPos >= _lineProperties.size()) {
    qDebug() << "Size error " << _screenLines.size() << _lineProperties.size() << currentPos;
}
And see something in terminal while running konsole.

The "auto values" use is in line 480.
If you want to verify if the "auto values" is correct, there is a function to qDebug its content, just add this:
toDebug(values);
it will print just the string without the colors.

That doesn't print anything, for some reason.

But it's fairly easy to build with asan and ubsan to get stacktraces to the usage of freed memory (and other invalid memory stuff), just add "include(ECMEnableSanitizers)" to the CMakeLists.txt, i. e.:

diff --git a/CMakeLists.txt b/CMakeLists.txt
  index df5da50a..9accafa2 100644
  --- a/CMakeLists.txt
  +++ b/CMakeLists.txt
  @@ -36,6 +36,7 @@ include(ECMGenerateHeaders)
   include(GenerateExportHeader)
   include(FeatureSummary)
   include(ECMQtDeclareLoggingCategory)
  +include(ECMEnableSanitizers)
   
   ecm_setup_version(${RELEASE_SERVICE_VERSION} VARIABLE_PREFIX KONSOLEPRIVATE
       SOVERSION ${RELEASE_SERVICE_VERSION_MAJOR}


and then run cmake with "cmake -DECM_ENABLE_SANITIZERS='address;undefined' [... whatever other options you use]"

Bumping priority, this is random memory corruption and I don't think there really are more serious issues (bar crash looping on startup).

It can lead to everything from random user data corruption and security issues to random crashes. I think it should at least be considered to revert the commits introducing it tbh.

(In reply to Carlos Alves from comment #12)
> The "auto values" use is in line 480.
> If you want to verify if the "auto values" is correct, there is a function
> to qDebug its content, just add this:
> toDebug(values);
> it will print just the string without the colors.

I meant the data type, since the crash is related to the stuff done there.

Created attachment 137326 [details]
Hacky patch

I still don't understand the problem, but stumbled upon something that at least stops it from crashing: I'm not familiar with QVarLengthArray (or understand why it is used there), so I tried changing it to QVector to debug.

Should probably still find the root cause and verify that we understand why it happens so we can make sure we don't just make it harder to actually fix the issue.

Carlos, are you still looking at this?

Created attachment 137465 [details]
attachment-10423-0.html



On Fri, Apr 9, 2021 at 2:00 PM Kurt Hindenburg <bugzilla_noreply@kde.org>
wrote:

> https://bugs.kde.org/show_bug.cgi?id=432376
>
> --- Comment #17 from Kurt Hindenburg <kurt.hindenburg@gmail.com> ---
> Carlos, are you still looking at this?
>
> --
> You are receiving this mail because:
> You are the assignee for the bug.

(In reply to Martin Sandsmark from comment #16)
> Created attachment 137326 [details]
> Hacky patch
> 
> I still don't understand the problem, but stumbled upon something that at
> least stops it from crashing: I'm not familiar with QVarLengthArray (or
> understand why it is used there), so I tried changing it to QVector to debug.
> 
> Should probably still find the root cause and verify that we understand why
> it happens so we can make sure we don't just make it harder to actually fix
> the issue.

QVarLengthArray was in use before I changed the code, but I understand what it does, it creates a stack vector (faster access), it will reserve the space in the stack (64 LineProperties in this case) and if it needs to use more it will send to heap memory.

Created attachment 137470 [details]
sanitizer enabled

(In reply to Martin Sandsmark from comment #13)
> That doesn't print anything, for some reason.
> 
> But it's fairly easy to build with asan and ubsan to get stacktraces to the
> usage of freed memory (and other invalid memory stuff), just add
> "include(ECMEnableSanitizers)" to the CMakeLists.txt, i. e.:
> 
> diff --git a/CMakeLists.txt b/CMakeLists.txt
>   index df5da50a..9accafa2 100644
>   --- a/CMakeLists.txt
>   +++ b/CMakeLists.txt
>   @@ -36,6 +36,7 @@ include(ECMGenerateHeaders)
>    include(GenerateExportHeader)
>    include(FeatureSummary)
>    include(ECMQtDeclareLoggingCategory)
>   +include(ECMEnableSanitizers)
>    
>    ecm_setup_version(${RELEASE_SERVICE_VERSION} VARIABLE_PREFIX
> KONSOLEPRIVATE
>        SOVERSION ${RELEASE_SERVICE_VERSION_MAJOR}
> 
> 
> and then run cmake with "cmake -DECM_ENABLE_SANITIZERS='address;undefined'
> [... whatever other options you use]"

I never had this error here. I just attached a video running with those options.

(In reply to Carlos Alves from comment #20)
> I never had this error here. I just attached a video running with those
> options.

Could you run this thing before resizing?

    echo $(printf 'a%.0s' $(seq 1 $(($(stty size | cut -d\  -f2) - 1))))

It forces it to fill the array completely, if I understand correctly.

Git commit 16d0df1f716885bf271ddca47f30b29ec4da0de9 by Martin T. H. Sandsmark.
Committed on 07/06/2021 at 15:46.
Pushed by tcanabrava into branch 'master'.

add test to expose memory corruption
CCMAIL: cbc.alves@gmail.com

A  +10   -0    tests/resize-test.sh

https://invent.kde.org/utilities/konsole/commit/16d0df1f716885bf271ddca47f30b29ec4da0de9