#2078 Invalid memory access in MoveSelectedLines with rectangular or thin selections

[Colomban Wendling]

Editor::MoveSeletcedLines(), called by SCI_MOVESELECTEDLINESUP and SCI_MOVESELECTEDLINESDOWN performs invalid memory access and thus incorrect buffer edition (in the best case) when the selection is in rectangular or thin mode (I'd generalize to anything but stream selection, but I'm not 100% sure).
It's very easy to reproduce: just create a thin selection, and call one of the above: garbage will appear in the file.

This is due to the inconsistency between selectionText.Data() and selectionLength, but AFAICT this function just cannot behave correctly with anything but actual lines selected, so should probably convert the selection to stream selection first thing.

Reproduced with both Geany and latest SciTE, GTK3, but shouldn't be platform specific.
See also https://github.com/geany/geany/issues/2066

[Neil Hodgson]

To avoid garbage characters, the insertion should be limited to selectedText.Length() with

@@ -1057,7 +1061,7 @@
        pdoc->InsertString(pdoc->Length(), eol, strlen(eol));
    GoToLine(currentLine + lineDelta);

-   selectionLength = pdoc->InsertString(CurrentPosition(), selectedText.Data(), selectionLength);
+   selectionLength = pdoc->InsertString(CurrentPosition(), selectedText.Data(), selectedText.Length());
    if (appendEol) {
        const Sci::Position lengthInserted = pdoc->InsertString(CurrentPosition() + selectionLength, eol, strlen(eol));
        selectionLength += lengthInserted;

Even with that, the behaviour with rectangular or thin still damages the text so that it should be avoided by doing nothing in those modes:

@@ -1005,6 +1005,10 @@

 void Editor::MoveSelectedLines(int lineDelta) {

+   if (sel.IsRectangular()) {
+       return;
+   }
+
    // if selection doesn't start at the beginning of the line, set the new start
    Sci::Position selectionStart = SelectionStart().Position();
    const Sci::Line startLine = pdoc->SciLineFromPosition(selectionStart);

Ideally, the rectangular selection would be reapplied over the same characters so that it is sticky through this operation but that appears more complex so more work. If someone wants to implement conversion to stream then that would be OK.

I'll apply the safety patches in this message unless there are objections.

[Neil Hodgson]

Fixed garbage text and invalid memory access by changes mentioned in previous post as [df5c32].

Not marked as fixed as this fix just ignores the commands when the selection is rectangular or thin.

[Colomban Wendling]

Fixed garbage text and invalid memory access by changes mentioned in previous post as [df5c32].

Good. However, declaration of selectionLength could be moved down and current initial value for it could be removed -- basically, line 1045 could be removed, just moving the decalartion itself to (current) line 1058.

Not marked as fixed as this fix just ignores the commands when the selection is rectangular or thin.

IMO, the selection should simply be reset to stream/line, basically as currently documented.

Another nicer approach (IMO), but less compatible and possibly trickier to implement, would be to simply move the lines themselves, preserving the selection, whichever it is. I'm not sure it's worth it though, as nobody asked for it or complained about the current behavior.

[Neil Hodgson]

Tidied up code with [25a036].

[Neil Hodgson]

Fixed rectangular and thin selections by converting first to stream selections with [a76e94].