User Track Player Crash

[marigi64]

When I have any cleo scripts installed, the game will eventually crash when using the game's user track player radio station (switching to the station, entering a vehicle or going to the next song).

I found the best way to test this is to spam the user track skip button, it will eventually crash the game. I also found it crashes sooner if mod loader is installed.

Here's the crash log for when modloader is installed. My cleo log doesn't come up with anything.

Game version: GTA SA 1.0 US Unhandled exception at 0x777EE925 in ntdll.dll (+0x3e925): 0xC0000005: Access violation writing location 0x00000024. Register dump: EAX: 0x00000024 EBX: 0x17785428 ECX: 0x00000020 EDX: 0x00211000
EDI: 0x00B6B970 ESI: 0x00000048 EBP: 0x12ABFE80 EIP: 0x777EE925
ESP: 0x12ABFE80 EFL: 0x00010202 CS: 0x00000023 SS: 0x0000002B
GS: 0x0000002B FS: 0x00000053 ES: 0x0000002B DS: 0x0000002B

Stack dump:
    0x12ABFE80:  12ABFEBC 00823FD0 00000020 00823763 00000000 00B6B970
    0x12ABFE98:  00000048 17785428 12ABFE90 12ABFE94 12ABF844 12ABFF10
    0x12ABFEB0:  00825EA4 00887F70 FFFFFFFF 00000000 005389A4 00000000
    0x12ABFEC8:  00000201 00000000 004F30BB 00000000 00000201 00000000
    0x12ABFEE0:  00000058 00000089 00000048 00B6B970 00000006 004F3660
    0x12ABFEF8:  00000006 00B606CC 18220FA8 00000006 00000089 00B6B970
    0x12ABFF10:  12ABFF50 0083C1C7 FFFFFFFF 004F1404 00000006 00B606CC
    0x12ABFF28:  00B60601 0000F6AD 76610F00 01E9AB2E 00000006 00000002
    0x12ABFF40:  00000001 00000089 00B60704 0000001B 12ABFFCC 0083C106
    0x12ABFF58:  FFFFFFFF 004F1619 004F15C0 12ABFF80 004F15C0 00B606CC
    base: 0x128C0000   top: 0x12ABFE80   bottom: 0x12AC0000
    
Backtrace (may be wrong):
    =>0x777EE925 RtlEnterCriticalSection+0x15 in ntdll.dll (+0x3e925) (0x12ABFE80) 
      0x00823FD0 in gta-sa.exe (+0x423fd0) (0x12ABFEBC) 
      0x005389A4 in gta-sa.exe (+0x1389a4) 

[thelink2012]

Which scripts do you have?

Are these installed in modloader or CLEO?

Which ASI scripts do you have installed in modloader?

Could you try deleting modloader/.data/plugins/gta3/std.asi.dll and check if the problem is reproducible again? Note that this file is responsible for loading ASI and CLEO mods in Mod Loader, so they may stop working during the test. So perhaps you should move these ASI/CLEO into main dir while testing this. I'm trying to verify the hypothesis that the issue is in the ASI loading subsystem of Mod Loader.

[marigi64]

The main cleo scripts I normally use are food eating fix and ped tweaks that I chuck into my mod loader folder but those scripts don't seem to work if I move them into the cleo folder so for testing this I tried tp to marker and surfly in both cleo and the modloader folder. It crashed with whatever script i had loaded whether it was in modloader or cleo.

I don't have any asi scripts installed in the modloader folder.

Deleting that file has helped a bit. Seems it still crashes but the crash happens a lot later.

[thelink2012]

Could you please try this CLEO.asi? I suspect there's a race condition in here and here. If gta_sa.exe:CAEStreamThread's chdir/readUtrax/unchdir overlaps with the start/end tick of a custom script, you get a crash.

[marigi64]

Yup that fixed the crashing. Couldn't get it to crash with and without modloader. :)

[thelink2012]

Thanks for testing.

cc @x87 @Deji69

I see the following solutions (in no particular preference order at the moment of writing):

• Save a boolean indicating this script has used SET_CURRENT_DIRECTORY. In this case, script local chdir feature gets enabled. Would be nice if we could disable it somehow after the script does something specific, but I can't visualize what that is at the moment. Too non-deterministic.
• Remove the script local chdir feature. Chdir is prone to race conditions, using it in any form should be avoided, not encouraged. Thus I ask you @Deji69 if you could please give context on the introduction of this feature in 4.3.
• Fix CAEStreamThread. Considering CAEStreamThread behavior is so subtle and non-deterministic, probably breaking so many other mods with their authors not even noticing, a fix on that should probably be good. That fix should go in the direction of not using chdir to read files in User Files from another thread.

FYI @marigi64 I've sent you a CLEO.asi patched with the second fix.

[Deji69]

@thelink2012 I don't remember much but the basis of localising chdir is likely just bug prevention... separate scripts should ideally run independently of each other so there's no interoperability issues between multiple scripts, which is a common source of bugs and complaints by users (I think this thinking was also the basis of the other script independent changes like ensuring multiple scripts could use their own sets of sprites without reaching limits). A better alternative is probably to just avoid chdir by operating on any paths each script is set to use, so if they use a relative path then the dir the script last gave for chdir is prepended, or the default path is used... however I'm not sure if maybe some scripts might want to do chdir then call an exe function to manipulate a path that in-game function uses... then that could cause issues (so maybe also do a real chdir before/after calling in-game functions from scripts?)

[thelink2012]

I see. Thank you for the overview. The reasoning makes sense. Though prepending is hard to enforce considering .cleo plugins might manipulate files as well :/

Thinking again of the solutions in the past post:

• Saving a boolean for scripts that use SET_CURRENT_DIRECTORY moves the problem from "hey, it's a CLEO issue!" to "hey, it's this script issue!". So people can easily mitigate by removing scripts that use said command. And documentation should discourage its use.
• Removing local chdir might break some script developed in 4.3+ that relies on that. Perhaps better not to touch it.
• Changing CAEStreamThread is the ultimate solution, but requires additional research. Also it's not clear whether it's CLEO's job to fix this issue.

[x87]

I think adding chdir command in user scripts was a mistake as it mutates the global state and lead to subtle bugs and race conditions. Ultimately the goal was to get access to the User Files directory, as in normal case the script should only operate within the game directory using relative paths. This is why initially chdir only accepted 0 and 1 to choose between the two directories. I don't think scripts should be able to set the current directory anywhere else but that ship has sailed already.

regarding a possible solution, if you'd ask me I'm all for the Deji's proposal:

just avoid chdir by operating on any paths each script is set to use, so if they use a relative path then the dir the script last gave for chdir is prepended, or the default path is used...

We only need to adjust the file manipulation opcodes (open_file, create_directory, file_exists, directory_exists). As being said there never was an idea to allow scripts changing the current directory to an arbitrary path. If the script needs it, they can use global paths. I would not care about any other opcode unless you can provide a clear use case when it's absolutely needed.

Unfortunately the same behavior had been implemented in CLEO for III/VC https://github.com/cleolibrary/III.VC.CLEO/blob/master/source/III.VC.CLEO/CustomOpcodes.cpp#L1334 so we would need to fix it there too.

[thelink2012]

I also think it may have been a mistake. CLEO does have several such little mistakes that we have to live with, and unfortunately keep some kind of compatibility.

We only need to adjust the file manipulation opcodes (open_file, create_directory, file_exists, directory_exists).

Also IniOperations. Unfortunately though I've seen third party plugins capable of e.g. loading texture files from disk and I am afraid this direction could break them.

As being said there never was an idea to allow scripts changing the current directory to an arbitrary path. If the script needs it, they can use global paths.

I would agree. But as you said the boat has sailed and there may be several scripts that depend on this behaviour.

Breaking things in the scale CLEO has is a bit troublesome (adoption of newer versions will be low and we'd have a ecosystem break). Instead, I think an opt-in to a "modern CLEO" would work better in some future. In the meantime, compatibility should be a concern.

Hyrum's Law may relate to this: https://www.hyrumslaw.com/

Unfortunately the same behavior had been implemented in CLEO for III/VC

Fortunately in this game chdir only happen in the main thread. So it's not too game breaking as happened in this issue. Problem's is more on cooperative scripts racing between WAITs.

On Sun, Feb 28, 2021, 11:55 AM x87 [email protected] wrote:

I think adding chdir command in user scripts was a mistake as it mutates the global state and lead to subtle bugs and race conditions. Ultimately the goal was to get access to the User Files directory, as in normal case the script should only operate within the game directory using relative paths. This is why initially chdir only accepted 0 and 1 to choose between the two directories. I don't think scripts should be able to set the current directory anywhere else but that ship has sailed already.

regarding a possible solution, if you'd ask me I'm all for the Deji's proposal:

just avoid chdir by operating on any paths each script is set to use, so if they use a relative path then the dir the script last gave for chdir is prepended, or the default path is used...

We only need to adjust the file manipulation opcodes (open_file, create_directory, file_exists, directory_exists). As being said there never was an idea to allow scripts changing the current directory to an arbitrary path. If the script needs it, they can use global paths. I would not care about any other opcode unless you can provide a clear use case when it's absolutely needed.

Unfortunately the same behavior had been implemented in CLEO for III/VC https://github.com/cleolibrary/III.VC.CLEO/blob/master/source/III.VC.CLEO/CustomOpcodes.cpp#L1334 so we would need to fix it there too.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cleolibrary/CLEO4/issues/38#issuecomment-787465166, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABNGVTNHMOQLBKVPNWXGMZ3TBJKMTANCNFSM4YBYJSFA .

[Deji69]

a "modern CLEO"

Sounds like something that could lead to many diversions, the decision to build a whole reusable and relatively high-performance scripting toolset for modding games, and eventually going AWOL from the modding scene due to too much wasted time and uncertainty about the actual need for such a thing! But that's a story for another thread... in short, try to avoid getting as ambitious as me :)

[x87]

Fortunately in this game chdir only happen in the main thread. So it's not too game breaking as happened in this issue. Problem's is more on cooperative scripts racing between WAITs.

yea, I was thinking more of scripts prone to errors happening when another script changes the current directory. Their relative paths are not going after that, are they?

script A:

chdir "C:/"
wait 0

script B:

load_file ("b.txt") // assumes `game_directory\b.txt` but loads `C:\b.txt` instead

[pmsobrado]

So is there a way to identify which script may be calling chdir, or can we just use the CLEO.asi from this comment, (https://github.com/cleolibrary/CLEO4/issues/38#issuecomment-786815186)?

Thanks!

[Megas97]

Also some of the other scripts i have just stop working after some time when using the User Track Player which gets annoying. They're loaded but the keys to activate them don't do anything anymore. I'd also like to know if using the CLEO.asi from #38 would fix these problems without causing new ones.

[x87]

I've sent you a CLEO.asi patched with the second fix.

what was the exact change? removing CCustomScript::StoreScriptCustoms and CCustomScript::RestoreScriptCustoms() ?

[thelink2012]

@x87 I removed the chdir calls from there.

diff --git a/source/CScriptEngine.cpp b/source/CScriptEngine.cpp
index 39758cb..fc6b91b 100644
--- a/source/CScriptEngine.cpp
+++ b/source/CScriptEngine.cpp
@@ -573,12 +573,12 @@ namespace CLEO
         static char tempDir[MAX_PATH];
         _getcwd(tempDir, sizeof(tempDir));
         working_path = tempDir;
-        _chdir(StoredDir);
+        //_chdir(StoredDir);
     }
     void CCustomScript::RestoreScriptCustoms()
     {
         _getcwd(StoredDir, sizeof(StoredDir));
-        _chdir(working_path.c_str());
+        //_chdir(working_path.c_str());
     }
     void CCustomScript::StoreScriptSpecifics()
     {

[x87]

Thanks! I think we should merge it to master an publish as a new version. I'm not too concerned about a few scripts relying on this (undefined) behavior, but the fact it crashes the game when using mp3 radio is far more broad issue.

Thoughts?

[thelink2012]

Yes. I guess that can be removed if compatibility isn't an issue.

[x87]

https://github.com/cleolibrary/CLEO4/releases/tag/v4.4.2 - test version

[Octavio1994]

I got this exact crash log, cleo.asi file you shared is from cleo 4.4, so let's see how it goes, meanwhile thank you :)