SiegeLord / DAllegro5

D binding to the Allegro5 game development library
Other
42 stars 15 forks source link

`argv` pointer casting in `al_run_allegro`/`main_runner` causes crash when running from a macOS .app bundle #56

Closed Rhys-T closed 2 weeks ago

Rhys-T commented 2 months ago

I was trying to get my macOS build of Lix packaged up as a proper double-clickable .app bundle, but for some reason, even though the executable ran by itself, as soon as I tried it as part of the bundle it would get a SIGSEGV in _D8allegro56system14al_run_allegroFMDFZiZ11main_runnerUiPPaZi (which demangles to int extern(C)main_runner(int, char**) allegro5.system.al_run_allegro(scope int() delegate)). I did some digging, and I think I see what's going on.

If I'm reading this library's system.d correctly, it's giving the underlying al_run_main call a fake argv, with the address of the game's callback function stored where the characters of argv[0] would normally be. main_runner then casts the pointer back the other way to find and invoke the callback function. The problem is that on macOS, if Allegro detects that the program is running from a .app bundle, it ignores the argc/argv it was passed and instead creates its own fake one containing the path to the app bundle[^notexe] and the path to the document dropped on the app, if any:

https://github.com/liballeg/allegro5/blob/5114ee60410c9aac6a0f23402570eb41a4cc9095/src/macosx/osx_app_delegate.m#L158-L192

So instead of getting the callback pointer, main_runner ends up using the first 8 bytes of the bundle path as an address, and crashes.

[^notexe]: Just the path to the bundle, not all the way to the executable, for some reason.

SiegeLord commented 2 months ago

Nice job finding the mechanism (I knew of the crash, but not why it happened). That's some weird logic in Allegro.

Here's the alternative (based on global variables) that I used in a pinch last time I hit this:

int al_run_allegro(scope int delegate() user_main)
{
    __gshared int delegate() user_main2;
    user_main2 = user_main;
    extern(C) static int main_runner(int argc, char** argv)
    {   
        version(OSX)
        {   
            thread_attachThis();
            rt_moduleTlsCtor();
        }   

        auto main_ret = user_main2();

        version(OSX)
        {   
            thread_detachThis();
            rt_moduleTlsDtor();
        }   

        return main_ret;
    }   

    return al_run_main(0, null, &main_runner);
}
Rhys-T commented 2 months ago

I tried patching that version into the copy of the library I was building Lix with, and so far it seems to work perfectly. (Other than the game suddenly not being Retina-capable, but it's not the library's fault that I haven't fully set up the Info.plist file.) Thanks!

I think the reason Allegro ignores the original argv is that on macOS, dropped or double-clicked documents don't get passed via argv - and sometimes the app gets an extra argument that looks like -psn0123456, which seems to just be used to set its 'process serial number' for some mostly-deprecated Mac APIs - so changing it to just the app and the document helps make things look as consistent across different platforms as possible. (Still seems weird that argv[0] ends up as the entire .app bundle and not the executable, though…)

Is using a global like this going to work as the official fix? I'm not that familiar with D, or Allegro for that matter, but I figured the fake argv trick was there for a reason. Are there any thread-safety concerns with using the global? Is there any way that a (non-broken) program could have multiple al_run_allegros going at once, fighting over the variable?

SimonN commented 2 weeks ago

Thanks for digging into these details!

Should usercode (games written with Allgero 5) use this fixed al_run_allegro in general, and not call DAllegro5's al_run_allegro anymore?

Or will there be a fix in DAllegro5?

Rhys-T commented 1 week ago

@SimonN Looks like the fix has been released as v4.0.7+5.2.0. It's not listed on the Releases page, but it's there under Tags, and in the DUB registry. I've tried building Lix against it, and it seems to work.

SiegeLord commented 1 week ago

Whoops, forgot that DUB needs a release (is that the case?).

Rhys-T commented 1 week ago

I think the DUB release is fine. I was just saying it didn't show up if you were looking at the GitHub release pages, so if you went there it looked like 4.0.6+5.2.0 was still the latest.

SimonN commented 1 week ago

Thanks! I'll bump my DAllegro dependency to 4.0.7+5.2.0 for the next release, then.