-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
"Split terminal horizontally" segfaults on Plasma Wayland. #806
Comments
Not reproducible under Weston. The problem may be in Plasma Wayland. Please attach a backtrace when reporting a crash! |
Trace generated when debugging with Qt Creator: backtrace.txt |
I confirm that splitting makes the latest git QTerminal crash under Plasma Wayland with the backtrace @uthidata attached. More precisely:
No crash under Weston. |
I succeeded in preventing the crash here and in #820 by using: diff -ruNp qtermwidget-orig/lib/TerminalCharacterDecoder.cpp qtermwidget/lib/TerminalCharacterDecoder.cpp
--- qtermwidget-orig/lib/TerminalCharacterDecoder.cpp
+++ qtermwidget/lib/TerminalCharacterDecoder.cpp
@@ -79,6 +79,16 @@ void PlainTextDecoder::decodeLine(const
_linePositions << pos;
}
+ // check the real length
+ for (int i = 0 ; i < count ; i++)
+ {
+ if (characters + i == nullptr)
+ {
+ count = i;
+ break;
+ }
+ }
+
//TODO should we ignore or respect the LINE_WRAPPED line property?
//note: we build up a QString and send it to the text stream rather writing into the text
But I'm not familiar with the code and don't know if this makes sense. @yan12125? EDIT: I made the patch more logical. |
Sorry for the late response. My todo queue is way too long 😥 The issue seems related to input methods, while I cannot reproduce crashes with either creating new tabs or splitting terminals under Plasma wayland with Fcitx 5 input method, so I don't know the exact cause. I guess this fix from Konsole is relevant. Although I believe decoders should be called with correct parameters, tsujan's patch is still good to have to avoid crashes due to other bugs. Some |
I'd noticed The above-mentioned patch prevents the crashes here. IMO, the method
They happen only under Plasma-Wayland (= KWin's Wayland), not under other Wayland compositors. |
I agree
Yeah I tested under the latest Plasma Wayland via sddm under Arch Linux. With or without Fcitx 5 (my daily Chinese input method), qterminal does not crash. |
Konsole checks nullity in a different way and skips it, without checking the range (Konsole → PlainTextDecoder.cpp → PlainTextDecoder::decodeLine). To me, also that code seems hanging by a hair. My patch was just a simple check that worked here; it wasn't based on a knowledge of the whole code (which I don't have). Because of that, it could interfere with your importing Konsole's fixes later. When you imported Konsole's fixes, just tell me to test them; the crash is 100% reproducible here with a non-patched |
Ah forgot to say, I don't plan to backport Konsole's commit anymore after reading it. It seems to return the whole line for |
OK, I'll make a small PR today. I think it'll be harmless if the root cause is eliminated. |
With this check, I can't make QTerminal crash by splitting or opening new tabs under Plasma Wayland. Fixes lxqt/qterminal#806 and fixes lxqt/qterminal#820
With this check, I can't make QTerminal crash by splitting or opening new tabs under Plasma Wayland. Fixes lxqt/qterminal#806 and fixes lxqt/qterminal#820
With this check, I can't make QTerminal crash by splitting or opening new tabs under Plasma Wayland. Fixes lxqt/qterminal#806 and fixes lxqt/qterminal#820
Expected Behavior
Program does not crash when splitting terminal horizontally.
Current Behavior
Program does crash when splitting terminal horizontally.
This does not happen in X11.
Possible Solution
Steps to Reproduce (for bugs)
Context
I was trying out wayland on plasma and checked if everything I used still works there.
System Information
The text was updated successfully, but these errors were encountered: