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

Crash on startup in mSDLAttachPlayer #1239

Closed
eevee opened this issue Nov 23, 2018 · 6 comments

Comments

@eevee
Copy link
Contributor

commented Nov 23, 2018

Pulled commit 741ac61 and rebuilt, and crashed right away. Problem is on line 206:

        if (events->preferredJoysticks[player->playerId] && strcmp(events->preferredJoysticks[player->playerId], joystickName) == 0) {

joystickName was NULL, so strcmp exploded. SDL_GetError complained that the joystick wasn't open, but I don't know why that would be a problem now when it wasn't before.

Testing for joystickName == NULL does avoid the problem, with the clumsy side effect that both of my "joysticks" (actually my wacom tablet) are nameless in the settings dialog.

@endrift

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

why is SDL_Joystick so bad

Can you try bisecting to see if the issue started at some point or if it's something else that changed on your system?

@eevee

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

I tried a clean rebuild of commit 7d79db7, which is what I had checked out before, and get the same problem. That makes SDL 2.0.9 a likely culprit, but I don't see anything in the changelog that sounds relevant.

I traced this back a bit and found that SDL_JoystickOpen is also returning NULL (which makes sense); SDL_GetError() gives "Unable to open /dev/input/event9", and errno is EACCES. But that device is owned by root:input and mode 640, and I'm in the input group and can open it just fine from the shell, so I'm not sure what's going on here.

@eevee

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

Tried LÖVE (which also uses SDL and exposes joystick list), and it simply reports that I have zero joysticks.

Not sure what's up with my system — although having my drawing tablet not register as a joystick is a welcome change :) — but mGBA probably just needs a bit of error handling here.

@endrift

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

I don't have any joysticks handy until tomorrow; can you test this patch?

diff --git a/src/platform/sdl/sdl-events.c b/src/platform/sdl/sdl-events.c
index 00d6df378..9f4b218b2 100644
--- a/src/platform/sdl/sdl-events.c
+++ b/src/platform/sdl/sdl-events.c
@@ -64,8 +64,12 @@ bool mSDLInitEvents(struct mSDLEvents* context) {
 		if (!SDL_JoystickListSize(&context->joysticks)) {
 			int i;
 			for (i = 0; i < nJoysticks; ++i) {
+				SDL_Joystick* sdlJoystick = SDL_JoystickOpen(i);
+				if (!sdlJoystick) {
+					continue;
+				}
 				struct SDL_JoystickCombo* joystick = SDL_JoystickListAppend(&context->joysticks);
-				joystick->joystick = SDL_JoystickOpen(i);
+				joystick->joystick = sdlJoystick;
 				joystick->index = SDL_JoystickListSize(&context->joysticks) - 1;
 #if SDL_VERSION_ATLEAST(2, 0, 0)
 				joystick->id = SDL_JoystickInstanceID(joystick->joystick);
@@ -203,6 +207,9 @@ bool mSDLAttachPlayer(struct mSDLEvents* events, struct mSDLPlayer* player) {
 #else
 		joystickName = SDL_JoystickName(SDL_JoystickIndex(SDL_JoystickListGetPointer(&events->joysticks, i)->joystick));
 #endif
+		if (!joystickName) {
+			continue;
+		}
 		if (events->preferredJoysticks[player->playerId] && strcmp(events->preferredJoysticks[player->playerId], joystickName) == 0) {
 			index = i;
 			break;
@@ -253,6 +260,9 @@ void mSDLPlayerLoadConfig(struct mSDLPlayer* context, const struct Configuration
 #else
 		const char* name = SDL_JoystickName(SDL_JoystickIndex(context->joystick->joystick));
 #endif
+		if (!name) {
+			return;
+		}
 		mInputProfileLoad(context->bindings, SDL_BINDING_BUTTON, config, name);
 
 		const char* value;
@@ -304,6 +314,9 @@ void mSDLPlayerSaveConfig(const struct mSDLPlayer* context, struct Configuration
 #else
 		const char* name = SDL_JoystickName(SDL_JoystickIndex(context->joystick->joystick));
 #endif
+		if (!name) {
+			return;
+		}
 		char value[16];
 		snprintf(value, sizeof(value), "%i", context->rotation.axisX);
 		mInputSetCustomValue(config, "gba", SDL_BINDING_BUTTON, "tiltAxisX", value, name);
@@ -332,8 +345,12 @@ void mSDLUpdateJoysticks(struct mSDLEvents* events, const struct Configuration*
 	SDL_Event event;
 	while (SDL_PeepEvents(&event, 1, SDL_GETEVENT, SDL_JOYDEVICEADDED, SDL_JOYDEVICEREMOVED) > 0) {
 		if (event.type == SDL_JOYDEVICEADDED) {
+			SDL_Joystick* sdlJoystick = SDL_JoystickOpen(event.jdevice.which);
+			if (!sdlJoystick) {
+				continue;
+			}
 			struct SDL_JoystickCombo* joystick = SDL_JoystickListAppend(&events->joysticks);
-			joystick->joystick = SDL_JoystickOpen(event.jdevice.which);
+			joystick->joystick = sdlJoystick;
 			joystick->id = SDL_JoystickInstanceID(joystick->joystick);
 			joystick->index = SDL_JoystickListSize(&events->joysticks) - 1;
 #if SDL_VERSION_ATLEAST(2, 0, 0)
@@ -347,16 +364,18 @@ void mSDLUpdateJoysticks(struct mSDLEvents* events, const struct Configuration*
 			joystickName = SDL_JoystickName(SDL_JoystickIndex(joystick->joystick));
 #endif
 			size_t i;
-			for (i = 0; (int) i < events->playersAttached; ++i) {
-				if (events->players[i]->joystick) {
-					continue;
-				}
-				if (events->preferredJoysticks[i] && strcmp(events->preferredJoysticks[i], joystickName) == 0) {
-					events->players[i]->joystick = joystick;
-					if (config) {
-						mInputProfileLoad(events->players[i]->bindings, SDL_BINDING_BUTTON, config, joystickName);
+			if (joystickName) {
+				for (i = 0; (int) i < events->playersAttached; ++i) {
+					if (events->players[i]->joystick) {
+						continue;
+					}
+					if (events->preferredJoysticks[i] && strcmp(events->preferredJoysticks[i], joystickName) == 0) {
+						events->players[i]->joystick = joystick;
+						if (config) {
+							mInputProfileLoad(events->players[i]->bindings, SDL_BINDING_BUTTON, config, joystickName);
+						}
+						return;
 					}
-					return;
 				}
 			}
 			for (i = 0; (int) i < events->playersAttached; ++i) {
@@ -364,7 +383,7 @@ void mSDLUpdateJoysticks(struct mSDLEvents* events, const struct Configuration*
 					continue;
 				}
 				events->players[i]->joystick = joystick;
-				if (config) {
+				if (config && joystickName) {
 					mInputProfileLoad(events->players[i]->bindings, SDL_BINDING_BUTTON, config, joystickName);
 				}
 				break;
@eevee

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

Seems good! I also dug up my actual gamepad, and now it's the only joystick listed, which also fixes a problem where mGBA kept thinking I was holding left via my tablet. :)

I guess something changed to make my Wacom stuff not readable as a joystick, though I can't imagine why SDL is failing in open for a device I should be able to read. Computers??

@endrift

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

(I'm waiting for a chance to test this myself before I merge it, btw)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.