Make ScmFunction lookup 1-based by x87 · Pull Request #550 · cleolibrary/CLEO5

x87 · 2026-03-10T19:23:20Z

Fixes an issue when the script uses return on the root level using a default ScmFunction Id (0).

{$CLEO}
script_name {name} "A"

foo()

debug_on
trace "after foo"
terminate_this_custom_script

function foo()
    while true
        wait {time} 0
        print_formatted_now {format} "inside foo" {time} 50
    end
end

{$CLEO}
script_name {name} "B"

wait {time} 5000
cleo_return 0

terminate_this_custom_script

Without fix:

"After foo" is printed, meaning script B is executing code from script A, plus destroying its call stack.

With fix:

Nothing is printed

MiranDMC

There is zero error checks for scenarios outside normal working conditions.

x87 · 2026-03-10T19:44:19Z

Storage validates max capacity, it is the only thing it should care. Anything else is validated on the caller side.

Get should not throw errors, because it can be used, e.g. for counting of used/free spots.

MiranDMC · 2026-03-10T19:48:55Z

Storage validates max capacity, it is the only thing it should care. Anything else is validated on the caller side.

Get should not throw errors, because it can be used, e.g. for counting of used/free spots.

Then implement checks in other places. I'm bored of wasting time troubleshooting crashes that could be prevented by basic coding discipline.
Apparently you are not using CLEO to anything so problems like that do not affect you.

x87 · 2026-03-10T19:50:07Z

Does this MR fix the issue with the script in the first post? What other crashes you think of?

MiranDMC · 2026-03-10T19:50:35Z

Storage validates max capacity, it is the only thing it should care. Anything else is validated on the caller side.

Get should not throw errors, because it can be used, e.g. for counting of used/free spots.

It should be method of the class then. Nobody has businesses to do things like that from outside.

MiranDMC · 2026-03-10T19:52:18Z

Does this MR fix the issue with the script in the first post? What other crashes you think of?

Any situation where invalid input arguments are used. Referencing not allocated/already deleted function ID is sign of serious problems. Currently situations like that are silently ignored, or some other function is returned if it happens to exists at that slot.

x87 · 2026-03-10T19:54:55Z

Does this MR fix the issue with the script in the first post? What other crashes you think of?

Any situation where invalid input arguments are used. Referencing not allocated/already deleted function ID is sign of serious problems. Currently situations like that are silently ignored, or some other function is returned if it happens to exists at that slot.

I'm not fixing imaginary issues. I spent last 6 months tirelessly fixing concrete, difficult bugs, caused sometimes by careless refactoring. If you have a specific scenario, please share it. Talk is cheap.

MiranDMC · 2026-03-10T20:13:42Z

 {
    ScmFunction* ScmFunction::store[Store_Size] = {0};
-    size_t ScmFunction::allocationPlace         = 0;
+    const size_t Default_Index                  = 1;


Two year of refactoring and you adding stuff like this. Why not declared in header file. Why it is member of CLEO namespace?

what's the difference?

Because it is class related property. Single look at header gives idea about general contents of the entire class.

constants belong to the logic in my book. why would I need to open header when I have my cpp file open? It's only used here

That's how spaghetti is created. You either look at the header and have general idea what it is all about or spend time analyzing every function inside cpp file.

not very convincing, but ok

MiranDMC · 2026-03-10T21:15:01Z

Technically fixes the issue but entire solution is so lazy it is even below vibe-coding quality.
If it turn out for example the MoonLoader scripts has field SCMFunction initialized with value 13, then are you going to make slot 13 unused too?

I would give approval but I'm not looking forward for rebaseing my branch.

x87 · 2026-03-10T23:38:04Z

Technically fixes the issue but entire solution is so lazy it is even below vibe-coding quality. If it turn out for example the MoonLoader scripts has field SCMFunction initialized with value 13, then are you going to make slot 13 unused too?

I would give approval but I'm not looking forward for rebaseing my branch.

lol.

Ok, show me how your BULLETPROOF coding solution helps to catch errors in this case

{$CLEO}
script_name {name} "A"

foo()

debug_on
trace "after foo"
terminate_this_custom_script

function foo()
    while true
        wait {time} 0
        print_formatted_now {format} "inside foo" {time} 50
    end
end

{$CLEO}
script_name {name} "B"

wait {time} 5000

40@ = 257 // ONLY THIS LINE ADDED TO ORIGINAL EXAMPLE
cleo_return 0

terminate_this_custom_script

MiranDMC · 2026-03-11T22:42:02Z

Ok, show me how your BULLETPROOF coding solution helps to catch errors in this case

On master branch your example only allocates function with index 0 then tries to use index 1.

Updated the script:

{$CLEO}
script_name {name} "A"

foo(1)

debug_on
trace "after foo"
terminate_this_script

function foo(depth: int)
    if
        depth <= 0
    then
        while not is_key_just_pressed {keyCode} KeyCode.Num1
            wait {time} 0
            print_formatted_now {format} "Inside foo. 1 to exit" {time} 50
        end
    else
        depth--
        foo(depth)
    end
end

Key breaking infinite loop was added.

As previously "after foo" is executed by script B, but then after pressing 1 button error is shown:

It claims the problem is user scripting error in script A.

Meanwhile on the version from #537 error is thrown the moment B script calls return:

Reason is actually the script log trying to query current stack depth.

After disabling script logging error is thrown also the moment script B tries to return:

Bit unexpected is fact B is suspended but pressing 1 then causes

MiranDMC · 2026-03-11T23:02:42Z

{$CLEO}
int ptr, func
script_name {name} "B"

40@ = 0x100

ptr = get_this_script_struct
func = read_memory_with_offset {address} ptr {offset} 0xDD {size} 2 // CRunningScript::ScmFunction
debug_on
trace "ScmFunction is now %d" func

wait {time} 5000

cleo_return 0

trace "B not suspended"
terminate_this_script

Updated B script

x87 · 2026-03-11T23:13:30Z

'Introduction to Debugging 101' by Miran:

ignore the original code and write completely new, unrelated one from scratch ✅

x87 · 2026-03-11T23:15:09Z

I used version from #537 and it does not produce any errors in the code I posted. But it shows "after foo".

MiranDMC · 2026-03-11T23:32:38Z

I used version from #537 and it does not produce any errors in the code I posted. But it shows "after foo".

Right, it does after running second game session. Bug introduced during implementation of idea of skipping slot 0. I have to untangle it.

MiranDMC · 2026-03-11T23:37:16Z

ignore the original code and write completely new, unrelated one from scratch ✅

By doing 40@ = 257 you are also setting bIsMission to true

x87 · 2026-03-11T23:54:26Z

ignore the original code and write completely new, unrelated one from scratch ✅

By doing 40@ = 257 you are also setting bIsMission to true

maybe, I did not validate that, the idea was to modify other two bytes

MiranDMC · 2026-03-11T23:57:28Z

It is not gonna to be bullet proof. But corrupted function id will not always target already allocated slots. Then the correct error message makes sense.
Currently it is exactly same as in case of using return without call. If script logging is active it will report incorrect stack depth in lucky scenario, in bad one it will get caught in loop causing stack overflow and exiting game without any info (no error message, no crash info etc.).

Per script storage would make it more likely to hit empty slots and would solve problem of cross execution. But there come external scripts that we do not know when they get terminated.

Another problem I spotted is after script termination its call stack is not cleared in the storage.

x87 · 2026-03-12T00:01:35Z

If script logging is active it will report incorrect stack depth in lucky scenario, in bad one it will get caught in loop causing stack overflow and exiting game without any info (no error message, no crash info etc.).

Is that true on this branch? My understanding is that it should eventually hit id=0 and stop.

MiranDMC · 2026-03-12T00:19:35Z

Is that true on this branch? My understanding is that it should eventually hit id=0 and stop.

Case I had shows it is somehow possible. Take note the script can get corrupted id then go forward and call more functions.
I would not depend on anything to eventually happen.
Requesting function from storage with id other than 0 and getting nullptr is sign of serious problem inside CLEO.

x87 · 2026-03-12T00:26:52Z

Is that true on this branch? My understanding is that it should eventually hit id=0 and stop.

Case I had shows it is somehow possible. Take note the script can get corrupted id then go forward and call more functions. I would not depend on anything to eventually happen. Requesting function from storage with id other than 0 and getting nullptr is sign of serious problem inside CLEO.

there is only one public function that allows it: CLEO_GetCleoCallStackSize. And It should handle this case better then, instead of default to 0. It can return -1 to the caller and let them decide how to handle this. The logger can put error message in the log, other library can ignore it, third library can show error message, etc.

MiranDMC · 2026-03-12T00:42:19Z

Should it really be caller role to decide if and how to report corruption inside the CLEO? CLEO_GetCleoCallStackSize takes as argument the script pointer, not function id. So there is no other scenario than severe problem already happened or is about to happen.

MiranDMC · 2026-03-12T00:47:29Z

There is another issue. Now I'm getting heap corruption error when:

script B executes "after foo"
then 1 is pressed to cause problem in A
new game session is started

Happens on master, this branch, and the #537 too.

Call stack leads to:

CCustomScript::~CCustomScript()
{
    if (BaseIP) delete[] BaseIP;

It also occurred before I rebased to current master (without recent custom mission BaseIP changes).

x87 · 2026-03-12T04:32:36Z

Double free

MiranDMC · 2026-03-12T20:16:13Z

Well, BaseIP is also stored in ScmFunction stack entry. It is needed when switching modules.

I added fixes to the other branch, but it still does not solve this case. If script uses other thread's return it restores its BaseIP as its own.
No idea how to solve that. Maybe it is time to make it per script, or at least store script pointer and check it's correctness during return.

MiranDMC · 2026-03-12T21:02:51Z

Ok seems I squished all issues on the other branch. "after foo" no longer is executed by B, A is unaffected at all, double free and few other bugs found and fixed.

x87 · 2026-03-12T23:07:14Z

Well, BaseIP is also stored in ScmFunction stack entry. It is needed when switching modules.

#552

x87 · 2026-03-13T03:06:11Z

Moving forward with MVP for better stability

Make ScmFunction lookup 1-based

dcccd47

x87 requested review from MiranDMC March 10, 2026 19:29

MiranDMC requested changes Mar 10, 2026

View reviewed changes

MiranDMC reviewed Mar 10, 2026

View reviewed changes

x87 added 2 commits March 10, 2026 16:40

review

71657a1

fmt

840a741

x87 mentioned this pull request Mar 11, 2026

SCM function storage fixes. #537

Closed

x87 merged commit 54fe1af into master Mar 13, 2026
3 checks passed

x87 deleted the fix_scmfunction_0 branch March 13, 2026 03:06

Conversation

x87 commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MiranDMC left a comment

Choose a reason for hiding this comment

Uh oh!

x87 commented Mar 10, 2026

Uh oh!

MiranDMC commented Mar 10, 2026

Uh oh!

x87 commented Mar 10, 2026

Uh oh!

MiranDMC commented Mar 10, 2026

Uh oh!

MiranDMC commented Mar 10, 2026

Uh oh!

x87 commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MiranDMC Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

x87 Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

MiranDMC Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

x87 Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

MiranDMC Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

x87 Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

MiranDMC commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

x87 commented Mar 10, 2026

Uh oh!

MiranDMC commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MiranDMC commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

x87 commented Mar 11, 2026

Uh oh!

x87 commented Mar 11, 2026

Uh oh!

MiranDMC commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MiranDMC commented Mar 11, 2026

Uh oh!

x87 commented Mar 11, 2026

Uh oh!

MiranDMC commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

x87 commented Mar 12, 2026

Uh oh!

MiranDMC commented Mar 12, 2026

Uh oh!

x87 commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MiranDMC commented Mar 12, 2026

Uh oh!

MiranDMC commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

x87 commented Mar 12, 2026

Uh oh!

MiranDMC commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

x87 commented Mar 10, 2026 •

edited

Loading

x87 commented Mar 10, 2026 •

edited

Loading

MiranDMC commented Mar 10, 2026 •

edited

Loading

MiranDMC commented Mar 11, 2026 •

edited

Loading

MiranDMC commented Mar 11, 2026 •

edited

Loading

MiranDMC commented Mar 11, 2026 •

edited

Loading

MiranDMC commented Mar 11, 2026 •

edited

Loading

x87 commented Mar 12, 2026 •

edited

Loading

MiranDMC commented Mar 12, 2026 •

edited

Loading

MiranDMC commented Mar 12, 2026 •

edited

Loading