Page MenuHomeWildfire Games

Fix out of bounds access in Hotkey.cpp
ClosedPublic

Authored by echotangoecho on Mar 11 2017, 8:24 PM.

Details

Summary

Do what the title says.

Test Plan

Verify that the mouse still works as expected.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

echotangoecho created this revision.Mar 11 2017, 8:24 PM
Vulcan added a subscriber: Vulcan.Mar 11 2017, 9:08 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/500/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/501/ for more details.

leper added a subscriber: leper.

Does not appear to allow accessing anything that could influence control flow, must still be fixed. Adding myself as a reviewer so I do have that show up later, does not look wrong from a quick glance, but I'd rather make sure that this does not leave anything unfixed.

leper requested changes to this revision.Apr 23 2017, 3:40 AM
leper added inline comments.
source/ps/Globals.h
21 ↗(On Diff #771)

Alphabetical order for includes of the same type.

source/ps/Hotkey.cpp
144 ↗(On Diff #771)

Replacing UNIFIED_LAST by MOUSE_BASE in only one of the cases changes the behaviour (in a most likely side effect free way) but makes one wonder why we still compare with the former, and also invalidates the comment about g_mouse_buttons[0] being unused.

This revision now requires changes to proceed.Apr 23 2017, 3:40 AM
echotangoecho added inline comments.May 10 2017, 5:45 PM
source/ps/Hotkey.cpp
144 ↗(On Diff #771)

g_mouse_buttons[0] still ends up being unused, as the first possible mouse keycode is MOUSE_BASE + 1 (MOUSE_BASE + SDL_BUTTON_LEFT) , but UNIFIED_LAST should be replaced in the comparison, indeed.

echotangoecho edited edge metadata.
echotangoecho marked an inline comment as done.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/1127/ for more details.

Itms resigned from this revision.May 14 2017, 9:04 PM
leper accepted this revision.May 19 2017, 7:55 PM

Thanks.

source/ps/Globals.cpp
32 ↗(On Diff #1826)

I'll fix this to be similar to the one you already fixed.

This revision is now accepted and ready to land.May 19 2017, 7:55 PM
This revision was automatically updated to reflect the committed changes.