Crash in GuiMLTextCtrl::reflow() / allocBitmap()

#1 I think i'm walking away from this for the time being as there are bigger fires,
but for any interested:

in GuiMLTextCtrl::allocBitmap() -
// oxe 20070326 - crash notes:
// sometimes in this code, bitmapName is nigh-unterminated, but bitmapNameLen looks correct,
// so maybe that's okay.
// but also, walk->bitmapNameLen will match bitmapNameLen,
// but walk->bitmapName will be "<Bad Ptr>". actually it's bitmapName minus 32832 (0x8040).
// and dStrncmp() will crash.

#2 Do you have any more info about reproducing this?

One thing which looks suspicious is the use of the buffer:

char nameBuffer[256];
   dStrncpy(nameBuffer, bitmapName, bitmapNameLen);
   nameBuffer[bitmapNameLen] = 0;

Length never checked - just written into - possible overrun memory stomp. Unless there's a restriction on bitmapNameLen elsewhere and it's <= 256...

[To whoever wrote that code - bad programmer! - no cookie.]

#3 Hey andy - thanks for looking at this.
unfortunately the repro was pretty sporadic.
i had a GuiMLTextCtrl with several instances of the same small bitmap in it,
and the entire control is sometimes dynamically resized many times a second,
and *sometimes* the problem would occur.

the code you post definitely looks bad.
i would guess it should be something more like:

char nameBuffer[256];
   AssertFatal(sizeof(nameBuffer) >= bitmapNameLen, "GuiMLTextCtrl::allocBitmap() - bitmap name too long");
   dStrncpy(nameBuffer, bitmapName, sizeof(nameBuffer));
   nameBuffer[bitmapNameLen] = 0;

i'll make that change and see if it improves things,
altho i doubt it'll have much effect since i would expect bitmapNameLen to be less than 256,
and in my particular case it was actually 32. (the 32 was just coincidentally the length of the path, not fixed as 2^5 or whatever)

edit - more error checking

#4 Just an update, this is currently our number one client crasher,
so uh, if anyone happens to have fixed this that would be awesome,
otherwise look for more from in the nearish future.

#5 From GuiMLTextCtrl::allocBitmap():

ret->bitmapName = bitmapName;

Theory: I suspect the pointer that's being assigned here is getting munged at some point. Then when it goes to check if it's already got the BitMap loaded in the for loop at the top of the function, it go BOOM.

Possible solution: Make Bitmap::bitmapName a buffer and strncpy into it instead of assigning the pointer.

#6 Now that I think about it, that would explain the intermittent crashes because the dStrncmp only happens when bitmapNameLen == walk->bitmapNameLen.

Also, I know you're on an old codebase with fixes merged in, so make sure you have this in the for loop at the top:

if (bitmapNameLen == walk->bitmapNameLen &&
   [b]![/b]dStrncmp(walk->bitmapName, bitmapName, bitmapNameLen))

Note the ! in front of the dStrncmp - it was missing on pre-1.5.1 TGE and was causing the wrong bitmaps to be returned in some cases.

#7 Heya -
yep, we've got that fix; thanks!

i'm wondering if perhaps the TextCtrl is being reflowed after it's been deleted.

in any event,
yr theory seems like a good fix,
altho i'd love to figure out who's freeing/stomping that original pointer.

still no repro on this, btw,
but i'll make the change for our next push and see if the crash reports change.

#8 Okay, have changed the method to put the bitmapName into the stringTable,
and then store the resulting StringTableEntry in the bitmapStruct instead.
note to do this you still have to copy the original bitmapName param into a temporary buffer in order to null-terminate the string.

here's what the function looks like now:
this part isn't strictly necessary, but in guiMLTextCtrl.h,
change const char* bitmapName; to StringTableEntry bitmapName;.

in guiMLTextCtrl.cc:

GuiMLTextCtrl::Bitmap *GuiMLTextCtrl::allocBitmap(char *bitmapName, U32 bitmapNameLen)
{
   for(Bitmap *walk = mBitmapList; walk; walk = walk->next)
      if(bitmapNameLen == walk->bitmapNameLen &&
         !dStrncmp(walk->bitmapName, bitmapName, bitmapNameLen))
         return walk;

   char nameBuffer[512];
   AssertFatal(bitmapNameLen < sizeof(nameBuffer), "bitmapNameLen too large!");

   dStrncpy(nameBuffer, bitmapName, bitmapNameLen);
   nameBuffer[bitmapNameLen] = '[[62817a98cf56e]]';

   StringTableEntry bitmapNameSTE = StringTable->insert(nameBuffer);


   Bitmap *ret = constructInPlace((Bitmap *) mResourceChunker.alloc(sizeof(Bitmap)));
   ret->bitmapName = bitmapNameSTE;
   ret->bitmapNameLen = bitmapNameLen;
   ret->bitmapHandle = TextureHandle(bitmapNameSTE, BitmapTexture);
   if(bool(ret->bitmapHandle))
   {
      ret->next = mBitmapList;
      mBitmapList = ret;
      return ret;
   }
   return NULL;
}

#9 This problem also did drive me a bit crazy ;) I did not crash but didn't display the right images - when I replaced the text with setText.

After setText the pointer to bitmapName points to junk, which cause this trouble.

The only thing I don't understand is the
nameBuffer[bitmapNameLen] = '[[4ab7bcf35b57d]]';
I did simply use
nameBuffer[bitmapNameLen] = NULL;

and in 1.52 it needs
GuiMLTextCtrl::Bitmap *GuiMLTextCtrl::allocBitmap(const char *bitmapName, U32 bitmapNameLen)

greetings thomas

#10 So I'm guessing this is one of those essential gui BugFixes for 1.5.2?

#11 Maybe not essential, but if you use images in GuiMLTextCtrl, I would suggest to use this fix ;)