#2513 Potential out of bounds read inside `Editor::ChangeCaseOfSelection()`

[Zufu Liu]

When sText and sMapped have different lengths (e.g. sText="a"; sMapped="aa"), the two while loop may have out of bounds read, second loop may overflow.

[Zufu Liu]

Following is the patch I currently test (needs better function/variable names), it's produced with following steps.
Simplify the code by adding 1 for lastDifferenceText and lastDifferenceMapped, this changed the code to following:

size_t firstDifference = 0;
while (sMapped[firstDifference] == sText[firstDifference]) {
    firstDifference++;
}
size_t lastDifferenceText = sText.size();
size_t lastDifferenceMapped = sMapped.size();
while (sMapped[lastDifferenceMapped - 1] == sText[lastDifferenceText - 1]) {
    lastDifferenceText--;
    lastDifferenceMapped--;
}
const size_t endDifferenceText = sText.size() - lastDifferenceText;
const Sci::Position lengthChange = lastDifferenceMapped - firstDifference;
// pdoc->DeleteChars() and pdoc->InsertString()

Fix out of bounds in the while loop, using code similar to Document::TrimReplacement():

Range FindDifference(std::string_view view, std::string_view text) noexcept {
    Range range;
    while (!view.empty() && !text.empty() && view.front() == text.front()) {
        range.start++;
        text.remove_prefix(1);
        view.remove_prefix(1);
    }
    while (!view.empty() && !text.empty() && view.back() == text.back()) {
        range.end++;
        text.remove_suffix(1);
        view.remove_suffix(1);
    }
    return range;
}

const Range difference = FindDifference(sMapped, sText);
const size_t firstDifference = difference.start;
const size_t lastDifferenceText = sText.size() - difference.end;
const size_t lastDifferenceMapped = sMapped.size() - difference.end;
const size_t endDifferenceText = sText.size() - lastDifferenceText;
// pdoc->DeleteChars() and pdoc->InsertString()

Extract common expression and simplify the arithmetic, got following:

const auto [firstDifference, endDifferenceText] = FindDifference(sMapped, sText);
const Sci::Position lengthSame = firstDifference + endDifferenceText;
const Sci::Position insertPos = currentNoVS.Start().Position() + firstDifference;
pdoc->DeleteChars(insertPos, rangeBytes - lengthSame);
const Sci::Position lengthChange = sMapped.size() - lengthSame;
const Sci::Position lengthInserted = pdoc->InsertString(
    insertPos,
    sMapped.c_str() + firstDifference,
    lengthChange);

Not sure whether worth it, pdoc->InsertString() can be simplified by pass first view by reference (as in TrimReplacement()).

std::string_view target = sMapped;
const auto [firstDifference, endDifferenceText] = FindDifference(target, sText);
const Sci::Position lengthSame = firstDifference + endDifferenceText;
const Sci::Position insertPos = currentNoVS.Start().Position() + firstDifference;
pdoc->DeleteChars(insertPos, rangeBytes - lengthSame);
const Sci::Position lengthChange = sMapped.size() - lengthSame;
const Sci::Position lengthInserted = pdoc->InsertString(insertPos, target);

or following code:

std::string_view target = sMapped;
const auto [firstDifference, endDifferenceText] = FindDifference(target, sText);
const Sci::Position insertPos = currentNoVS.Start().Position() + firstDifference;
pdoc->DeleteChars(insertPos, rangeBytes - firstDifference - endDifferenceText);
const Sci::Position lengthInserted = pdoc->InsertString(insertPos, target);
const Sci::Position diffSizes = sMapped.size() - sText.size() + lengthInserted - target.length();

[Zufu Liu]

or following:

std::string_view target = sMapped;
const auto [firstDifference, endDifferenceText] = FindDifference(target, sText);
const Sci::Position lengthSame = firstDifference + endDifferenceText;
const Sci::Position insertPos = currentNoVS.Start().Position() + firstDifference;
pdoc->DeleteChars(insertPos, rangeBytes - lengthSame);
const Sci::Position lengthInserted = pdoc->InsertString(insertPos, target);
const Sci::Position diffSizes = lengthSame - sText.size() + lengthInserted;

[Zufu Liu]

simplified the code further as following, moved two views above the if condition to avoid decouple SSO string data and length twice.

std::string_view text = sText;
std::string_view mapped = sMapped;
if (mapped != text) {
    size_t firstDifference = 0;
    // similar to Document::TrimReplacement()
    while (!mapped.empty() && !text.empty() && mapped.front() == text.front()) {
        firstDifference++;
        text.remove_prefix(1);
        mapped.remove_prefix(1);
    }
    while (!mapped.empty() && !text.empty() && mapped.back() == text.back()) {
        text.remove_suffix(1);
        mapped.remove_suffix(1);
    }
    const Sci::Position insertPos = currentNoVS.Start().Position() + firstDifference;
    pdoc->DeleteChars(insertPos, text.length());
    const Sci::Position lengthInserted = pdoc->InsertString(insertPos, mapped);
    // Automatic movement changes selection so reset to exactly the same as it was.
    const Sci::Position diffSizes = lengthInserted - text.length();
}

[Neil Hodgson]

I'm not sure this is possible. There has to be an input text that case maps to a different output where one is smaller but the smaller is a suffix of the larger. "a" -> "aa" (or reverse) isn't a feasible case mapping.

For input > output, there has to be some prefix of the input that is removed by the case map function. For output > input, some new text has to be inserted at the front of the input. This sort of result could occur for a normalization function that, for example, removed combining accents but I don't think it will occur for case mapping.

There could be some point in hardening against potential problems, either bugs or limitations in any platform calls, with the CaseMapString method.

[Zufu Liu]

That's not possible in sensible implementation, but new code is more simple and readable.

I patched ScintillaWin::CaseMapString() to do custom text transliteration (which is the easiest way to handle multiple selections, much easy than iterate and manipulate selections in application). I found out of bounds read when doing C escape/unescape (e.g. \\ => \\\\).

[Neil Hodgson]

OK, I'll look at this after the release.

[Neil Hodgson]

Committed with [92a455].