missionpinball / mpf

Mission Pinball Framework: Open source software to run a real pinball machine.
http://missionpinball.org
MIT License
217 stars 143 forks source link

Timer event order pollution #1851

Closed avanwinkle closed 3 weeks ago

avanwinkle commented 3 weeks ago

This PR fixes a bug (found by https://groups.google.com/g/mpf-users/c/JtZxG7oOceQ/m/P3htggdmAwAJ) where the order of a Timer's control_events could cause a crash.

The Problem

There has been a longstanding (7+ years) flaw in the timer control event code where the event handler kwargs were a singleton dictionary, so each event handler would alter/amend the same dictionary and attach it as kwargs. This oversight slipped by largely unnoticed until earlier this year, when the timer code was revamped to pass more event kwargs through (to support dynamic placeholder values for tick value, tick interval, et cetera., see #1761).

Even then, this oversight slipped through because the timer kwarg pollution is dependent on the order in which the timer's control_events are listed. Only if a control event with a value comes before a reset/restart will the former event's timer_value kwarg be polluted into the reset/restart handler, causing a duplicate argument error.

The Fix

This PR simply moves the kwargs = {} declaration to be inside the control event iteration block, so each control event starts with a fresh, empty, unpolluted kwarg dictionary. With this change, the order of control events doesn't matter and pollution is avoided.

tick tock tick tock

sonarcloud[bot] commented 3 weeks ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud