A zeroed control map matches gamepad South, and a NULL handler_off segfaults on release #81

New Issue

tachikoma · 2026-08-02T21:54:11-04:00

tachikoma commented

2026-08-02 21:54:11 -04:00

Source: found sweeping for stale TODO.md references; the tutorials warn about both, and neither had an issue (at ad71072)

Two defects in akgl_controller_handle_event (src/controller.c) that a caller meets together,
because both are properties of filling in an akgl_ControlMap entry.

1. A zeroed button field is a real button. The two arms of the match are evaluated as one
expression (src/controller.c:312-321):

eventButtonComboMatch = (
    ((event->type == SDL_EVENT_GAMEPAD_BUTTON_DOWN || ... ) &&
     (curmap->jsid == 0 || event->gbutton.which == curmap->jsid) &&
     event->gbutton.button == curcontrol->button) ||
    ((event->type == SDL_EVENT_KEY_DOWN || ... ) &&
     (curmap->kbid == 0 || event->key.which == curmap->kbid) &&
     event->key.key == curcontrol->key)
);

SDL_GAMEPAD_BUTTON_SOUTH is 0. So a keyboard-only binding, declared as a stack local and
zeroed the way every example does it, also matches every press of A/Cross on any gamepad --
jsid == 0 matches any device, and button == 0 matches the south face button. The binding
fires for an input its author never named.

The tutorial already documents the workaround, which is what says this is a real trap rather
than a theoretical one: Chapter 21 tells the reader to "set button
to SDL_GAMEPAD_BUTTON_INVALID" because "the keyboard and gamepad arms of the match are
evaluated together, and 0 is a real button." A default that has to be written out in prose is a
default in the wrong place.

Fix, and it is a decision rather than an obvious patch. SDL_GAMEPAD_BUTTON_INVALID is -1,
so the field cannot simply default to it in a zeroed struct. Either the match requires the event
kind to have been declared (an explicit kind on the control), or the arms test the field that
identifies the binding rather than trusting a sentinel. The wildcard-device work took the first
approach for jsid/kbid by spending 0 deliberately; that reasoning does not carry here,
because 0 is not free on button.

2. handler_on and handler_off are called without a NULL check (src/controller.c:323
and :326):

if ( event->type == curcontrol->event_on && eventButtonComboMatch) {
    CATCH(errctx, curcontrol->handler_on(curmap->target, event));
} else if ( event->type == curcontrol->event_off && eventButtonComboMatch ) {
    CATCH(errctx, curcontrol->handler_off(curmap->target, event));
}

A control that acts on press and has nothing to do on release is the ordinary case, and it
segfaults on the first key release. The tutorial's answer is to install jrpg_cmhf_ignore, a
handler that does nothing -- so every consumer writes the same empty function.

Fix: check both pointers, and treat a NULL handler as "nothing to do" rather than as an
error. That is what the caller means by leaving it NULL, and it deletes the do-nothing handler
from every consumer.

Tests: tests/controller.c dispatches a gamepad south-button press at a keyboard-only
binding and asserts it does not fire; and dispatches a key release at a control with a NULL
handler_off and asserts it returns rather than crashing. Both fail against the current code.

Filed by Tachikoma (Claude Code, Opus 5, 1M context)

**Source:** found sweeping for stale `TODO.md` references; the tutorials warn about both, and neither had an issue (at ad71072) Two defects in `akgl_controller_handle_event` (`src/controller.c`) that a caller meets together, because both are properties of filling in an `akgl_ControlMap` entry. **1. A zeroed `button` field is a real button.** The two arms of the match are evaluated as one expression (`src/controller.c:312-321`): ```c eventButtonComboMatch = ( ((event->type == SDL_EVENT_GAMEPAD_BUTTON_DOWN || ... ) && (curmap->jsid == 0 || event->gbutton.which == curmap->jsid) && event->gbutton.button == curcontrol->button) || ((event->type == SDL_EVENT_KEY_DOWN || ... ) && (curmap->kbid == 0 || event->key.which == curmap->kbid) && event->key.key == curcontrol->key) ); ``` `SDL_GAMEPAD_BUTTON_SOUTH` is 0. So a **keyboard-only** binding, declared as a stack local and zeroed the way every example does it, also matches every press of A/Cross on any gamepad -- `jsid == 0` matches any device, and `button == 0` matches the south face button. The binding fires for an input its author never named. **The tutorial already documents the workaround**, which is what says this is a real trap rather than a theoretical one: [Chapter 21](docs/21-tutorial-jrpg.md) tells the reader to *"set `button` to `SDL_GAMEPAD_BUTTON_INVALID`"* because *"the keyboard and gamepad arms of the match are evaluated together, and 0 is a real button."* A default that has to be written out in prose is a default in the wrong place. **Fix, and it is a decision rather than an obvious patch.** `SDL_GAMEPAD_BUTTON_INVALID` is -1, so the field cannot simply default to it in a zeroed struct. Either the match requires the event kind to have been declared (an explicit `kind` on the control), or the arms test the field that identifies the binding rather than trusting a sentinel. The wildcard-device work took the first approach for `jsid`/`kbid` by spending 0 deliberately; that reasoning does not carry here, because 0 is not free on `button`. **2. `handler_on` and `handler_off` are called without a NULL check** (`src/controller.c:323` and `:326`): ```c if ( event->type == curcontrol->event_on && eventButtonComboMatch) { CATCH(errctx, curcontrol->handler_on(curmap->target, event)); } else if ( event->type == curcontrol->event_off && eventButtonComboMatch ) { CATCH(errctx, curcontrol->handler_off(curmap->target, event)); } ``` A control that acts on press and has nothing to do on release is the ordinary case, and it segfaults on the first key release. The tutorial's answer is to install `jrpg_cmhf_ignore`, a handler that does nothing -- so every consumer writes the same empty function. **Fix:** check both pointers, and treat a NULL handler as "nothing to do" rather than as an error. That is what the caller means by leaving it NULL, and it deletes the do-nothing handler from every consumer. **Tests:** `tests/controller.c` dispatches a gamepad south-button press at a keyboard-only binding and asserts it does *not* fire; and dispatches a key release at a control with a NULL `handler_off` and asserts it returns rather than crashing. Both fail against the current code. --- Filed by Tachikoma (Claude Code, Opus 5, 1M context)

tachikoma added this to the 0.9.x milestone 2026-08-02 21:54:11 -04:00

tachikoma added the defect api-gap blast-radius:medium status::grooming labels 2026-08-02 21:54:11 -04:00

tachikoma referenced this issue from a commit

2026-08-02 22:02:07 -04:00

Repoint every stale TODO.md citation in the tree

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: andrew/libakgl#81