Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.x] Fix crash on get_joy_button_index_from_string and get_joy_axis_index_from_string for non-existing key #59195

Merged
merged 1 commit into from
Mar 17, 2022

Conversation

Snowapril
Copy link
Contributor

@Snowapril Snowapril commented Mar 16, 2022

This revision fixes issue #59175.

int InputDefault::get_joy_button_index_from_string(String p_button) {
	for (int i = 0; i < JOY_BUTTON_MAX; i++) {
		if (p_button == _buttons[i]) {
			return i;
		}
	}
	ERR_FAIL_V(-1);
}

int InputDefault::get_joy_axis_index_from_string(String p_axis) {
	for (int i = 0; i < JOY_AXIS_MAX; i++) {
		if (p_axis == _axes[i]) {
			return i;
		}
	}
	ERR_FAIL_V(-1);
}

As _buttons and _axes have both valid string and nullptr.
When iterating over them, if a given key exists it will work correctly.

But if given key does not exist, it will end up with String::operator=(nullptr).
As String constructor from nullptr exists, I use it.

JOY_BUTTON_MAX have value 128 and only used parts of array have max length only 23.
When iterating over them, adding check for nullptr and early-quit is also considerable.

Bugsquad edit:

@Snowapril Snowapril requested a review from a team as a code owner March 16, 2022 14:22
@akien-mga
Copy link
Member

When iterating over them, adding check for nullptr and early-quit is also considerable.

I would add this too to be safe. Someone might think in the future that the String() constructor is redundant and remove it, reintroducing the crash. So guarding against the actual unnecessary comparison is safer.

@akien-mga
Copy link
Member

I think the bug may also exist in the master branch, see this code in core/input/input.cpp:

JoyButton Input::_get_output_button(String output) {
        for (int i = 0; i < (int)JoyButton::SDL_MAX; i++) {
                if (output == _joy_buttons[i]) {
                        return JoyButton(i);
                }
        }
        return JoyButton::INVALID;
}

JoyAxis Input::_get_output_axis(String output) {
        for (int i = 0; i < (int)JoyAxis::SDL_MAX; i++) {
                if (output == _joy_axes[i]) {
                        return JoyAxis(i);
                }
        }
        return JoyAxis::INVALID;
}

@Snowapril
Copy link
Contributor Author

@akien-mga It seems that _joy_buttons and _joy_axes limited its length with their max enum value.

static const char *_joy_buttons[(size_t)JoyButton::SDL_MAX] = {
	"a",
	"b",
	"x",
	"y",
	"back",
	"guide",
	"start",
	"leftstick",
	"rightstick",
	"leftshoulder",
	"rightshoulder",
	"dpup",
	"dpdown",
	"dpleft",
	"dpright",
	"misc1",
	"paddle1",
	"paddle2",
	"paddle3",
	"paddle4",
	"touchpad",
};
static const char *_joy_axes[(size_t)JoyAxis::SDL_MAX] = {
	"leftx",
	"lefty",
	"rightx",
	"righty",
	"lefttrigger",
	"righttrigger",
};

JoyButton::SDL_MAX and JoyAxis::SDL_MAX are exactly matched with the length of each array.
Do we also need to add an additional nullptr check for them?

@akien-mga
Copy link
Member

JoyButton::SDL_MAX and JoyAxis::SDL_MAX are exactly matched with the length of each array.
Do we also need to add an additional nullptr check for them?

Ah that's correct, then it seems fine as is.

return i;
}
}
ERR_FAIL_V(-1);
ERR_FAIL_V_MSG(-1, vformat(RTR("Could not find a button index matching the string \"%s\"."), p_button));
Copy link
Member

@akien-mga akien-mga Mar 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't translate error messages so this shouldn't use RTR(). (my suggestion was using C++ R-strings to avoid having to escape ", but it's fine with the escape too)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah,, I misunderstood your suggestion. Ok I applied it.

return i;
}
}
ERR_FAIL_V(-1);
ERR_FAIL_V_MSG(-1, vformat(RTR("Could not find a axis index matching the string \"%s\"."), p_axis));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*an axis index

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I missed it. Sorry 😂

As _buttons and _axes have both valid string and nullptr.
When iterating over them, if given key exists it will work correctly.
But if given key does not exist, it will end up with
String::operator=(nullptr). As String constructor from nullptr exists, I
use it.
@akien-mga akien-mga merged commit 88e2c51 into godotengine:3.x Mar 17, 2022
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 3.4.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants