openplanet-nl / issues

Issue tracker for Openplanet.
10 stars 0 forks source link

Accessing some properties of CGameManiaAppScriptEvent crashes openplanet.dll #185

Closed XertroV closed 1 year ago

XertroV commented 2 years ago

MLHook's inspector, particularly WRT some events, crashes openplanet.dll

example: before I had this:

        string[] data = {tostring(event.KeyCode), event.KeyName, event.CustomEventType, FastBufferWStringToString(event.CustomEventData), event.ExternalEventType, FastBufferWStringToString(event.ExternalEventData), "EMenuNavAction::" + tostring(event.MenuNavAction), event.IsActionAutoRepeat ? 't' : 'f'};
        auto ce = CustomEvent("CGameManiaAppScriptEvent::EType::" + tostring(event.Type), ArrStringToFastBufferWString(data), EventSource::MA_SE, "", event.CustomEventLayer);

Basically I'd try to serialize all fields of the event.

Before the latest update, I noticed warning msgs in the ML log about some things trying to access .KeyCode when an event was not a keypress event. That was misattributed to particular ML I think, b/c I was accessing it during intercepted procs.

Based on this, I tried an alternate implementation that selectively serializes different parts of the event based on type:

    void CaptureMAScriptEvent(CGameManiaAppScriptEvent@ event) {
        if (!ShouldCapture) return;
        string[] data = {};
        CGameUILayer@ layer = null;
        // accessing irrelevant parts of an even crashes the game
        if (event.Type == CGameManiaAppScriptEvent::EType::KeyPress) {
            data = {tostring(event.KeyCode), event.KeyName, event.IsActionAutoRepeat ? 't' : 'f'};
        } else if (event.Type == CGameManiaAppScriptEvent::EType::MenuNavigation
                    || uint(event.Type) == 32759) {
            data = {"EMenuNavAction::" + tostring(event.MenuNavAction)};
        } else if (event.Type == CGameManiaAppScriptEvent::EType::LayerCustomEvent) {
            data = {event.CustomEventType, FastBufferWStringToString(event.CustomEventData)};
            @layer = event.CustomEventLayer;
        } else if (event.Type == CGameManiaAppScriptEvent::EType::ExternalCustomEvent) {
            data = {event.ExternalEventType, FastBufferWStringToString(event.ExternalEventData)};
        }
        auto ce = CustomEvent("CGameManiaAppScriptEvent::EType::" + tostring(event.Type)
            , ArrStringToFastBufferWString(data)
            , EventSource::MA_SE
            , ""
            , layer
            // , (event.CustomEventLayer is null) ? null : event.CustomEventLayer // this crashes the game even tho we check for null :(
            );
        _RecordCaptured(ce);
    }

This works. IDK why accessing some parts of the event crashes OP. Note that even if I check whether event.CustomEventLayer is null, attempting to access it still crashes the game.

Note that, with my current version of MLHook, I only seem to intercept CGameManiaAppScriptEvent::EType::32759, so maybe there was a change to that enum? tostring used to print it correctly.

codecat commented 2 years ago

It might crash because irrelevant parts aren't initialized, causing Openplanet to try to handle uninitialized memory, which can potentially cause a crash. (For example, trying to access a garbage pointer, which would also pass the null check as you observed.)

Nadeo recently has been optimizing a lot of stuff for the console versions of the game, so it's very likely this is the change of behavior you're seeing.

I haven't verified it though, so I will leave this issue open for now. Can you create a minimal reproduction plugin that I can test with? (The smallest possible Plugin_*.as you can make that reproduces the crash)

XertroV commented 2 years ago

This seems to work to cause a crash. I couldn't get it to work in Scripts/ but works fine as a plugin this (tho the name / category comes from the .as file)

MinimalCrash.zip