Fix mode-pointer lifetime bugs and assign mode ids at enumeration

[poliva]

I am integrating switchres into Fightcade's fork of FBNeo for an upcoming Fightcde update and found several related bugs around current_mode and mode ids, while saving st.current_mode at init and tried to restore it later.

• Dangling current_mode pointer:

get_available_video_modes() stored the address of the loop-local mode via set_current_mode(&mode). That stack slot is reclaimed when the function returns, so current_mode() then returned a dangling pointer and sr_get_state() read current_mode()->id off the freed stack (valgrind: "Invalid read of size 4 ... below stack pointer" in sr_get_state).

Point it at the stable desktop_mode member instead, same desktop-mode content, but it lives for the display's lifetime, which is what restore_desktop_mode() already relies on. Fixed in all three backends (linux, sdl2, windows).

• CRTC use-after-free:

xrandr_timing::set_timing read crtc_info->mode in the return expression after XRRFreeCrtcInfo(crtc_info). Capture the flag before freeing.

• Mode ids assigned at enumeration:

Mode ids were only assigned lazily in display_manager::get_mode() when a mode was selected, so every enumerated mode, including the desktop mode, sat at id 0. A frontend restoring with sr_set_mode(id) searched for id 0 and matched whatever mode happened to be first (e.g. a 240x192 CRT Emudriver mode) instead of the desktop mode.

Assign a unique id to every mode at enumeration time, before the desktop_mode/backup_modes copies are taken so all copies agree (windows, linux, sdl2).

Also fix sr_mode_internal's SR_ACTION_GET_FROM_ID lookup iterating video_modes by value: it stored the address of the loop-local copy as selected_mode, already dead when SR_ACTION_SWITCH dereferenced it.

[antonioginer]

Hi poliva,

Thanks a lot for your PR and for detecting these bugs.

I'm just not merging it directly because I have 2 concerns:

• The issue with set_current_mode you describe is correct but the solution must be different. We don't want m_current_mode referencing the desktop_mode member (our desktop modeline backup). Instead, we want it to point to a specific element into the modeline vector. This is what is broken with the current logic, because it ends up pointing to a temporary object. This bug was introduced when we refactored the old C code to use C++ vectors. Unfortunately fixing this properly may require switching from using pointers to indexes into the vector.
• Assigning IDs only to modes that had been created or selected was intentional, rather than auto-assigning IDs to all listed modes. The idea was that you only requested stored IDs that you had previously obtained from Switchres. So these could eventually be implemented as hashes rather than incremental IDs (like handles). To be honest, it's been a long time since this was decided and I can't remember the exact reasons, or whether there's any drawback in simply auto-assigning incremental IDs like in your implementation. It's likely not a problem.

I need some time to think about this.