1
0
Fork 0
mirror of https://github.com/TerryCavanagh/VVVVVV.git synced 2024-12-22 17:49:43 +01:00

Fix undefined behavior with left-click logic in editor

There's an if-else chain that first deals with figuring out if there's
an entity where your left-click happened, and to do this it uses
edentat(), which returns a sentinel value of -1 if there is NOT an
entity where your cursor is.

It's very important to check that the value returned ISN'T -1 before you
start indexing the 'edentity' vector, since if you DO index it with that
-1, it'll result in Undefined Behavior because you're doing an
out-of-bounds array access.

Now, here's what the if-else chain looked like before:

    if(tmp==-1 && ed.free(ed.tilex,ed.tiley)==0)
    {
        ...
    }
    else if(edentity[tmp].t==1)

The bug here is very subtle but it was an easy oversight. Basically, if
'ed.free' ended up not being zero, control flow would jump to the next
"else if" over, which then ends up asking for the -1th index of
'edentity', which is Undefined Behavior.

This undefined behavior has now resulted in a crash on my system after
TerryCavanagh/VVVVVV#172, due it shuffling things around juuuuust enough
such that this UB would end up resulting in a segfault instead of
chugging along and working fine. For me and my system, this meant that
if my first left-click in the editor upon opening the game was me
placing down a tile and not placing down an entity, the game would
crash. But, it would be fine if I first placed down an entity and then
afterwards placed down tiles, because it's UB.

And I'm almost certain this was the cause of the very strange bug where
you couldn't hold down left-click for the foreground-placing tool (but
you COULD for the background-placing tool) that seemed to occur most
often on Windows (TerryCavanagh/VVVVVV#25).

The solution to this is to stick in another conditional in the tree
before any indexing occurs, such that there's no way any other
conditionals with the indexing in the conditional tree could end up
being hit. In summary, the if-else chain looks like this now:

    if(tmp==-1 && ed.free(ed.tilex,ed.tiley)==0)
    {
        ...
    }
    else if(tmp == -1)
    {
        //Important! Do nothing, or else Undefined Behavior will happen
    }
    else if(edentity[tmp].t==1)
This commit is contained in:
Misa 2020-03-01 21:25:29 -08:00 committed by Ethan Lee
parent 8d44d9387b
commit b82a8a0925

View file

@ -5099,6 +5099,10 @@ void editorinput( KeyPoll& key, Graphics& dwgfx, Game& game, mapclass& map, enti
ed.lclickdelay=1; ed.lclickdelay=1;
} }
} }
else if(tmp == -1)
{
//Important! Do nothing, or else Undefined Behavior will happen
}
else if(edentity[tmp].t==1) else if(edentity[tmp].t==1)
{ {
edentity[tmp].p1=(edentity[tmp].p1+1)%4; edentity[tmp].p1=(edentity[tmp].p1+1)%4;