Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #9588, 140a96b: [Squirrel] Reaching memory limit during script registration could prevent further script detections #9589

Merged
merged 1 commit into from Oct 2, 2021

Conversation

@glx22
Copy link
Contributor

@glx22 glx22 commented Oct 1, 2021

Motivation / Problem

When a script's info.nut triggers Maximum memory allocation exceeded, it can prevent the detection of other scripts of the same type.
This happens because the memory allocation triggering the limit is still accounted in the allocator for all other detections.
And that means this memory is also never freed.

Once the allocated_size issue was fixed, another issue appeared, a script failing to register due to memory limit would still be available ingame (and possibly other scripts that should have failed memory check). That's because scripts are scanned multiple times, and the error was not properly reset.

Finally another memory leak exists, when the memory limit is triggered in a constructor and using placement new.

Regarding the reported crash, it's because DummyAI registration triggers the memory limit due to a previous failing script and incomplete cleaning between registrations.

Description

Properly count allocated memory, and free memory not referenced by squirrel before it's lost.
I added an assert to ensure all allocated memory is managed by squirrel and its garbage collector, or is taken care of otherwise.

Limitations

As now the engine is properly reset, the failing scripts are now reported multiple times in debug console, because many rescan happen during startup.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')
…ript registration could prevent further script detections

Also the memory allocation triggering the limit was never freed.
And if the exception was thrown in a constructor using placement new, the pre-allocated memory was not freed either.
LordAro
LordAro approved these changes Oct 2, 2021
@glx22 glx22 merged commit ccd586a into OpenTTD:master Oct 2, 2021
16 checks passed
Loading
@glx22 glx22 deleted the fix_9588 branch Oct 2, 2021
TrueBrain added a commit to TrueBrain/OpenTTD that referenced this issue Oct 3, 2021
…ript registration could prevent further script detections (OpenTTD#9589)

Also the memory allocation triggering the limit was never freed.
And if the exception was thrown in a constructor using placement new, the pre-allocated memory was not freed either.
TrueBrain added a commit to TrueBrain/OpenTTD that referenced this issue Oct 3, 2021
…ript registration could prevent further script detections (OpenTTD#9589)

Also the memory allocation triggering the limit was never freed.
And if the exception was thrown in a constructor using placement new, the pre-allocated memory was not freed either.
TrueBrain added a commit to TrueBrain/OpenTTD that referenced this issue Oct 3, 2021
…ript registration could prevent further script detections (OpenTTD#9589)

Also the memory allocation triggering the limit was never freed.
And if the exception was thrown in a constructor using placement new, the pre-allocated memory was not freed either.
TrueBrain added a commit to TrueBrain/OpenTTD that referenced this issue Oct 3, 2021
…ript registration could prevent further script detections (OpenTTD#9589)

Also the memory allocation triggering the limit was never freed.
And if the exception was thrown in a constructor using placement new, the pre-allocated memory was not freed either.
TrueBrain added a commit to TrueBrain/OpenTTD that referenced this issue Oct 3, 2021
…ript registration could prevent further script detections (OpenTTD#9589)

Also the memory allocation triggering the limit was never freed.
And if the exception was thrown in a constructor using placement new, the pre-allocated memory was not freed either.
TrueBrain added a commit that referenced this issue Oct 3, 2021
…gistration could prevent further script detections (#9589)

Also the memory allocation triggering the limit was never freed.
And if the exception was thrown in a constructor using placement new, the pre-allocated memory was not freed either.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants