Anonymous Login
2018-12-16 11:18 UTC

View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0000583OpenClonkEngine - C4Scriptpublic2017-12-23 11:37
ReporterGünther 
Assigned To 
PrioritynormalSeverityminorReproducibilityalways
StatusacknowledgedResolutionopen 
Product Version 
Target Versiongit masterFixed in Version 
Summary0000583: Script Parse errors can leak C4Strings
DescriptionMakeSetter stores a reference to a C4String* in a C4AulBCC on the stack. Normally, this reference is later moved to a BCC in Code and cleaned up in ClearCode. But a script error can prevent that, and the reference is leaked.

This isn't a real problem since it's at most one tiny string per script error, and since the strings are interned even huge amount of errors over a long run of the engine won't pose a problem.

But it'd be nice to not make the C4String leak detector trigger, without risking a slowdown in the normal case.
TagsNo tags attached.
Attached Files

-Relationships
related to 0000742acknowledged C4String leak in Boomshire.ocs 
+Relationships

-Notes

~0001597

Newton (administrator)

Hmm, I don't really understand the issue here. Will the engine crash, or be slower (memleak?), or something else? And, in what case would that happen?

~0001599

Günther (developer)

The only real problem is that debug engines will trigger an assertion on exit. The amount of leaked memory because of this bug is insignificant (just a few strings that would be kept in memory anyway if the script error hadn't occured), but I want to keep that assertion to catch bugs that do leak a significant amount of memory.

~0002537

Sven2 (developer)

Last edited: 2013-03-17 15:30

View 2 revisions

For me (MSVC10), the assertion on shutdown triggers a bug in the custom assertion handler because some resources are already freed at that time. This crashes the engine in a debug build.

It also sucks because when I test a minimal scenario that has LocalOnly=1 definitions, there are a couple of script error in System.ocg. This means I cannot check for memory leaks with this minimal scenario.

Why not just put the C4String * into an auto_ptr?

~0002538

Isilkor (developer)

Last edited: 2013-03-17 17:08

View 2 revisions

> Why not just put the C4String * into an auto_ptr?
Please don't ever use auto_ptr. Use std::unique_ptr (C++11) or boost::scoped_ptr (C++98) instead.

> It also sucks because when I test a minimal scenario that has LocalOnly=1 definitions, there are a couple of script error in System.ocg.
IMO this is a bug in System.ocg; global functions that require specific definitions to be loaded probably shouldn't be in there.

~0005072

occ (reporter)

Hi! There's been a check-in that references this bug. For more information you can visit the repository browser at this address:
https://git.openclonk.org/openclonk.git/commitdiff/6866375c6742d2be1bbd886306ffc69d14b8cafc

Changeset 6866375 by Günther Brammer <gbrammer@gmx.de>
Don't leak references to Strings on script errors (0000583)

~0005077

occ (reporter)

Hi! There's been a check-in that references this bug. For more information you can visit the repository browser at this address:
https://git.openclonk.org/openclonk.git/commitdiff/e22ca51f722dbdae61fe0514de3727e36b90f4b0

Changeset e22ca51 by Günther Brammer <gbrammer@gmx.de>
Don't leak references to Strings on script errors (0000583)

~0005963

Luchs (administrator)

Possibly fixed now? In any case, not important for this particular release.
+Notes

-Issue History
Date Modified Username Field Change
2011-02-24 00:54 Günther New Issue
2011-02-24 10:52 Newton Note Added: 0001597
2011-02-24 21:50 Günther Note Added: 0001599
2013-01-08 23:19 Newton Relationship added related to 0000742
2013-03-17 15:29 Sven2 Note Added: 0002537
2013-03-17 15:30 Sven2 Note Edited: 0002537 View Revisions
2013-03-17 17:05 Isilkor Note Added: 0002538
2013-03-17 17:08 Isilkor Note Edited: 0002538 View Revisions
2015-10-16 00:33 Sven2 Target Version => 7.0
2015-11-30 12:11 Clonkonaut Status new => acknowledged
2015-11-30 12:11 Clonkonaut Target Version 7.0 => 8.0
2016-04-23 19:29 occ Note Added: 0005072
2016-04-24 19:15 occ Note Added: 0005077
2017-12-23 11:35 Luchs Target Version 8.0 => git master
2017-12-23 11:37 Luchs Note Added: 0005963
+Issue History