View Issue Details

IDProjectCategoryView StatusLast Update
0003812The Dark ModCodingpublic29.11.2017 10:12
ReporterSteveL Assigned Toduzenko  
PrioritynormalSeverityminorReproducibilityhave not tried
Status closedResolutionfixed 
Product VersionTDM 2.03 
Target VersionTDM 2.06Fixed in VersionTDM 2.06 
Summary0003812: idBounds::ToString() returns a pointer to a temporary object that immediately gets destroyed
DescriptionOffending code:

/*
============
idBounds::ToString
============
*/
const char * idBounds::ToString( const int precision ) const {
    return idStr( b[0].ToString( precision ) ) + " -> " + idStr( b[1].ToString( precision ) );
}
Additional InformationI've caught this in action producing garbage when passed to a printf command. Usually you get away with it and the printf uses the value before it gets overwritten, but not always.
TagsNo tags attached.

Relationships

related to 0004682 resolvedduzenko Painting skins broken in 2.06 

Activities

SteveL

SteveL

25.08.2014 09:06

developer   ~0006895

Last edited: 25.08.2014 09:22

View 2 revisions

Also idVec3::ToString()
And idMat3::ToString()
And probably lots more

These are all creating idStr objects on the heap anyway so they should be returning those objects instead of a pointer to the heap data, which they immediately release back to the pool.

tels

tels

29.08.2014 09:41

developer   ~0006901

I've tried to see what other "ToString()" functions do, they too need to solve the same problem.

But looking at idStr::FloatArrayToString, we see it used an array of four values, so if you call it in a nested function more than four times, it starts overwriting it's own data. The 16K fixed buffer does not help, either.

This smells like buffer-overflows. Ugh.

/*
=============
idStr::FloatArrayToString
=============
*/
const char *idStr::FloatArrayToString( const float *array, const int length, const int precision ) {
    static int index = 0;
    static char str[4][16384]; // in case called by nested functions
    int i, n;
    char format[16], *s;

    // use an array of string so that multiple calls won't collide
    s = str[ index ];
    index = (index + 1) & 3;

...
SteveL

SteveL

29.08.2014 14:30

developer   ~0006902

Yep, that's a different but equally dodgy pattern. These should all be returning a const idStr by value. I checked Str.h and it seems idStr doesn't implement lazy copying, but I expect the compiler will do that for itself if the return value is const, and anyway, there's no need to care about it even if the string gets copied on the heap. These functions are for debugging and occasional console output, they're not going to be called thousands of times per frame.

Need to check for any ToString() calls embedded in Printf()s, va()s and the like before making changes, because they'd need to use c_str(). I don't expect there'll be many.
duzenko

duzenko

07.10.2017 18:30

developer   ~0009418

Last edited: 07.10.2017 18:32

View 2 revisions

Only idBounds::ToString is dangerous, the other two return pointer to a static char[] (array overflow and thread safety are separate issues).

duzenko

duzenko

07.10.2017 18:34

developer   ~0009419

idBounds::ToString deleted as unused
grayman

grayman

10.10.2017 12:57

administrator   ~0009446

I use idBounds::ToString for debugging.

Please put it back.
grayman

grayman

10.10.2017 16:20

administrator   ~0009452

1 - revert rev. 7219 so that idBounds.ToString() becomes available again.

2 - Open AI.cpp and look for idAI::Think().

3 - Near the beginning of that method, just before this line:

SetNextThinkFrame();

add this line:

if (idStr(GetName()) == "atdm_ai_citywatch_1") DM_LOG(LC_AAS, LT_DEBUG)LOGSTRING("%s absolute bounds = [%s]\r",GetName(),GetPhysics()->GetAbsBounds().ToString());

4 - Open darkmod/darkmod.ini and set these two values:

LogDebug=1
LogClass_AAS=1

5 - Build TheDarkMod.exe.

Run TheDarkMod.exe and start the mission "Thiefs' Den". There's a character in there named atdm_ai_citywatch_1 and every time he thinks, a debug line gets printed to Darkmod.log.

6 - Quit the game after 10s or so.

7 - Open Darkmod.log and notice the debug lines showing the name of the character and his current absolute bounds.
duzenko

duzenko

11.10.2017 15:18

developer   ~0009455

Svn rev 7231

Issue History

Date Modified Username Field Change
10.08.2014 11:36 SteveL New Issue
25.08.2014 09:06 SteveL Note Added: 0006895
25.08.2014 09:22 SteveL Note Edited: 0006895 View Revisions
25.08.2014 09:23 SteveL Severity normal => minor
29.08.2014 09:41 tels Note Added: 0006901
29.08.2014 14:30 SteveL Note Added: 0006902
07.10.2017 18:30 duzenko Note Added: 0009418
07.10.2017 18:32 duzenko Note Edited: 0009418 View Revisions
07.10.2017 18:34 duzenko Note Added: 0009419
07.10.2017 18:34 duzenko Assigned To => duzenko
07.10.2017 18:34 duzenko Status new => assigned
07.10.2017 18:34 duzenko Status assigned => closed
07.10.2017 18:34 duzenko Resolution open => fixed
07.10.2017 18:34 duzenko Fixed in Version => TDM 2.06
10.10.2017 12:57 grayman Note Added: 0009446
10.10.2017 14:27 duzenko Status closed => feedback
10.10.2017 14:27 duzenko Resolution fixed => reopened
10.10.2017 16:20 grayman Note Added: 0009452
11.10.2017 15:18 duzenko Note Added: 0009455
11.10.2017 15:18 duzenko Status feedback => closed
11.10.2017 15:19 duzenko Resolution reopened => fixed
11.10.2017 15:44 nbohr1more Target Version => TDM 2.06
29.11.2017 10:12 duzenko Relationship added related to 0004682