TerryCavanagh / VVVVVV

The source code to VVVVVV! http://thelettervsixtim.es/
Other
6.93k stars 555 forks source link

Fix segfault from attempting to unwordwrap a string that starts with two newlines #1125

Closed InfoTeddy closed 7 months ago

InfoTeddy commented 7 months ago

This fixes a segmentation fault caused by an out-of-bounds indexing caused by an attempt to unwordwrap a string that starts with two newlines.

The problem here is that in the branch of the function string_unwordwrap() where consecutive_newlines == 1, the function does not check that the string result isn't empty before attempting to index result.size()-1. If result is empty, then result.size() is 0, and result.size()-1 becomes -1, and indexing a string at position -1 is always undefined behavior.

Funnily enough, a similar indexing happens just a few lines down, but this time, there is a check to make sure that the string isn't empty first. I'm unsure of how @Daaaav forgot that check a few lines earlier.

This situation can happen in practice, with custom level localizations. I made a level with a filename of testloc.vvvvvv and created a file at lang/fr/levels/testloc/custom_cutscenes.xml with the following content:

<?xml version="1.0" encoding="UTF-8"?>
<cutscenes>
    <cutscene id="test" explanation="">
        <dialogue speaker="cyan" english="This is text..." translation="blarg"/>
    </cutscene>
</cutscenes>

Then I switched to French, created a script named test, and created a text box that started with two newlines (so in total, the text box must be at least 3 lines in length). Running the script triggers the segfault when the text box is created. (Well, technically, on my machine, it triggers an assertion fail in libstdc++ and aborts, but that's basically the same thing.)

To fix this while still preserving the exact amount of newlines, if result is empty, we add a newline instead of attempting to index the string.

Legal Stuff:

By submitting this pull request, I confirm that...

Daaaav commented 7 months ago

I'm unsure of how @Daaaav forgot that check a few lines earlier.

Oopsie, I guess that seemed like a place where we had to have added something to result already...

It's not correct that this will happen on an empty string though. That's what that other !result.empty() is for. This specific segfault will only happen if the string starts with at least two newlines.

Other than that, the fix works (though I think it turns two newlines at the start of a string into one newline? So maybe we should make sure leading and trailing newlines work exactly as we want them to)

InfoTeddy commented 7 months ago

It's not correct that this will happen on an empty string though. That's what that other !result.empty() is for. This specific segfault will only happen if the string starts with at least two newlines.

Other than that, the fix works (though I think it turns two newlines at the start of a string into one newline? So maybe we should make sure leading and trailing newlines work exactly as we want them to)

Thanks for the correction. I've updated the patch accordingly and the two leading newlines are now preserved accordingly.

InfoTeddy commented 7 months ago

Suggestions applied.