Red Hat Bugzilla – Bug 183033
Allegro crashes when required resources not available
Last modified: 2013-07-02 19:14:24 EDT
Description of problem: I stumbled over this one by accident. When trying to start games using allegro through a ssh terminal connection allegro will segfault. I know this isn't supposed to work, but I would expect an error message to stderr rather then a crash. Version-Release number of selected component (if applicable): allegro-4.2.0-7 overgod-1.0-3.fc5 lacewing-1.10-4.fc5 How reproducible: Everytime. Steps to Reproduce: 1. Remote login to a machine with overgod or lacewing installed (not Xnest or something, just plain shell) 2. Try to start a game. 3. Watch it crash. Actual results: Backtrace from overgod #0 0x0feaa7f0 in _set_gfx_mode (card=-1, w=0, h=0, v_w=0, v_h=0, allow_config=-1) at ./src/graphics.c:637 #1 0x0feaaf60 in set_gfx_mode (card=Variable "card" is not available. ) at ./src/graphics.c:609 #2 0x1004c674 in main () Backtrace from lacewing #0 0x0febc5c8 in install_keyboard () at ./src/keyboard.c:658 #1 0x1004f0f8 in initialize () #2 0x10055cb4 in main () Expected results: A small error message to stderr stating that keyboard, video card, or other resources are not available.
This bug is caused by the fact that allegro lacks checks whether system_driver isn't NULL before accessing system_driver members. For this reason it segfaults at least while initializing keyboard on timer. Patch is coming soon.
There are two possible solutions to this issue. 1) The problem is that the checks are missing almost everywhere in allegro so the solution might be to exit allegro when no valid system driver is found while searching one in _install_allegro() (src/allegro.c). 2) patch various parts of install_keyboard(), install_timer(), _set_gfx_mode(), etc. to deal with the NULL system_driver what it might make it more stable and slower. Personally I like the first idea as it doesn't have much sense to go on in execution of an allegro app when no system driver is found. Frank, Hans, what's your opinion?
Actually I like 1, but instead of exit you could return a status code, because according to: http://www.talula.demon.co.uk/allegro/onlinedocs/en/api.html install_allegro now has an exit code and programs should check it. Problem with this is 4.2 is almost 100% compatible with 4.0 and under 4.0 apps didn't need to check this as allegro_init indeed used to call exit in this case. Actually I think this is not an allegro issue at all because reading the above page install_allegro will already return in error in the described case, so the real problem is probably in lacewing, overgod and likely other allegro users.
Created attachment 125312 [details] Patch as a result of a minor source code audit of allegro. Hans, I had a look into the code and it seems like the the upstream code has checks for non-NULL system_driver at many parts of the code but not everywhere. It might show that an original intention was to make it bullet-proof also for the case that the return value of the allegro_init() aka _install_allegro() isn't properly checked and the program goes on. Maybe for keeping the backward compatibility I'd apply this patch since any such application would crash at keyboard/timer/graphics initialization immediatelly with segfault for lots of legacy code not aware of checking the allegro_init() when no suitable system driver is found. The checks for non-NULLness of system_driver doesn't slow down allegro much as it's used mostly exclusively in initialization functions that ale called once. On many places I see ASSERT(system_driver) before accessing components of the system_driver, however this check is bogus for the non-debug compilation of allegro as the macro does nothing then.
Jindrich, I don't know if this is the best plan, applications will most probably still behave unexpected (and likely just crash somewhere else) if there is no system driver. I'm not saying you're patch is bad, but I do not believe its a complete solution we need to either: 1) restore the exit() call when no system driver is found 2) get all the apps fixed I would prefer 2, we could add a fprintf(stderr, ... with a warning to all the checks in your patch with a request to bugzilla it. Also it might be a good idea to take this issue upstream and let them choose the preferred solution.
Based on the date this bug was created, it appears to have been reported against rawhide during the development of a Fedora release that is no longer maintained. In order to refocus our efforts as a project we are flagging all of the open bugs for releases which are no longer maintained. If this bug remains in NEEDINFO thirty (30) days from now, we will automatically close it. If you can reproduce this bug in a maintained Fedora version (7, 8, or rawhide), please change this bug to the respective version and change the status to ASSIGNED. (If you're unable to change the bug's version or status, add a comment to the bug and someone will change it for you.) Thanks for your help, and we apologize again that we haven't handled these issues to this point. The process we're following is outlined here: http://fedoraproject.org/wiki/BugZappers/F9CleanUp We will be following the process here: http://fedoraproject.org/wiki/BugZappers/HouseKeeping to ensure this doesn't happen again.
This bug is still valid, the problem is there is no clear way howto solve this. The best solution would still be for allegro using applicaitions to check the return value of install_allegro(). Adding an exit() call to install_allegro() would break the API, so thats not really an option. Adding checks for a NULL sysdriver everywhere doesn't help much for applications which assume things will just work, iow those which don't check the return value of allegro_install in the first place. Conclusion, the apps should be fixed, not allegro, closing.