gbadev.org forum archive

This is a read-only mirror of the content originally found on forum.gbadev.org (now offline), salvaged from Wayback machine copies. A new forum can be found here.

DS development > malloc overwriting previously allocated memory

#177566 - fluffypants - Wed Sep 05, 2012 4:26 am

Hi everyone. Longtime lurker, first time poster. I am running into an issue dynamically allocating arrays. I'll post the section of code after I give a quick overview of the problem.

I am using nitroFS to read in a file containing texture data. The format is something like this:

Code:
//Palette length
10
//Palette entries R,G,B newline deliniated
1
1
1
21
21
21
10
7
10
....
//Tile length
96
//Tiledata len*64 (8x8)
1
1
5
6
...


Nothing too complicated there. I've stuck various iprintf statements for debugging to ensure that the values I'm getting are correct. So far no problems, I get the exact values I see in my file.

However, since I don't know the size of the palette and tiles beforehand, I have to dynamically allocate pent and tent (paletteEntries and tileEntries, I realize I should rename these to something more descriptive) as I parse the file. This works for the first array, but as soon as I malloc the second array, the last 4 positions in the first get clobbered. I'm baffled.

Here's my entry point from main:
Code:
int main(int argc, char **argv) {
   int paletteLen;
   u16 *paletteEntries;
   int tileLen;
   u8 *tileEntries;

   ReadGraphics(&paletteLen, &paletteEntries, &tileLen, &tileEntries, "bg_tiles.h");
....
}


and here's the function in question:

Code:
void ReadGraphics(int *plen, u16 **pent, int *tlen, u8 **tent, char *fileName)
{
   if (nitroFSInit()) {
      {
         // now, try reading a file to make sure things are working OK.
         FILE* fileIn = fopen(fileName,"rb");
         if(fileIn)
         {
            int i,j;
            int R, G, B;

            {
               char line[64] = { 0 };
            
               // Get palette length
               readLine(line, fileIn);   // first line is a comment
               readLine(line, fileIn);   // Palette length
               *plen = (int)atoi(line);
               //allocate a big enough palette
               *pent = (u16 *) malloc(*plen);
               // Get palette entries
               readLine(line, fileIn);   // line is a comment
               for(i = 0; i < *plen; i++)
               {
                  readLine(line, fileIn);
                  R = (int)atoi(line);
                  readLine(line, fileIn);
                  G = (int)atoi(line);
                  readLine(line, fileIn);
                  B = (int)atoi(line);


                  (*pent)[i] = (u16)RGB15(R, G, B);
                  
               }

               // Get tile block len
               readLine(line, fileIn);   // line is a comment
               readLine(line, fileIn);   // number of 8x8 blocks
               *tlen = (int)atoi(line);
               
               // Get tiles
               readLine(line, fileIn);   // line is a comment
               
               iprintf("pE func: %d\n",(*pent)[7]);
               *tent = (u8*) malloc((*tlen)*64);
               iprintf("pE func: %d\n",(*pent)[7]);
               
               for(i = 0; i < *tlen; i++)
               {
                  for(j = 0; j < 64; j++)
                  {
                     readLine(line, fileIn);
                     (*tent)[i*64 + j] = (u8)atoi(line);
                  }
               }
            }

            fclose(fileIn);
         }
      }
   } else {
      iprintf("nitroFSInit failure: terminating\n");
   }
   
}



This is the part that I'm confused about:

Code:
iprintf("pE func: %d\n",(*pent)[7]);
*tent = (u8*) malloc((*tlen)*64);
iprintf("pE func: %d\n",(*pent)[7]);


The first print statement gives me:
23254 which is correct. After the malloc line, that same print now gives me 0. If I comment that line out then pent[7] retains the correct value. Also, the issue happens regardless of how much memory I am allocating (even 1 byte creates a problem). Most of the paletteEntries are fine, it's just a couple at the tail end of my array that get clobbered, and the tent array is just fine.

I'm still very much a novice, so feel free to point out any glaring problems with my code, related or not to the issue at hand. And thanks for reading thus far.

#177567 - Quirky - Wed Sep 05, 2012 7:58 am

We don't know what "readLine" does, but it could be overwriting the end of the line buffer, which would mess up the stack and cause something like this. Do you ensure you never read more than 64 bytes at a time?

Also, general style things: you have a couple of extra scopes that aren't needed (extra {}), but that could be a side effect of snipping the code for posting here if you've redacted it a bit. There are lots of spurious casts. atoi returns an int, no need to cast that. In C you don't need to cast the return value of malloc either. I prefer the "if (exceptional condition) { return; }" style to avoid really deep nesting down the happy-path of the code, but that one is debatable.

If I were you, I'd try and get this working on the PC first. It'd compile if you #define iprintf printf, change your u8 and u16 to uint8_t and uint16_t and #include <stdint.h>. That way you can use gdb or whatever to debug the problem more easily.

#177568 - fluffypants - Wed Sep 05, 2012 9:09 am

Thanks Quirky,
Quote:
Also, general style things: you have a couple of extra scopes that aren't needed (extra {}), but that could be a side effect of snipping the code for posting here if you've redacted it a bit

I actually just modified part of the nitrofs example in devkitPro, hence a couple of straggling curlies. It certainly needs cleanup and I'm always embarrassed to show code, especially a frankenstein.
Quote:
We don't know what "readLine" does, but it could be overwriting the end of the line buffer, which would mess up the stack and cause something like this. Do you ensure you never read more than 64 bytes at a time?

Currently readLine doesn't do any buffer overflow checks. I'm planning on adding that, but was trying to get the simplest scenario working. I've double checked the contents of 'line' for the entire file and it seems fine (no lines bigger than a couple of characters, besides the comments). It's only when I start memory management that it goes awry. I've printed the entire file line by line, and it seems OK. I was thinking that I might be thrashing my stack for a while, but then I noticed that even allocating 1 byte was clobbering my first array (which is only 20 bytes long). I suppose the next test would be to just malloc the two arrays without reading the file at all to see what happens.
Quote:
There are lots of spurious casts. atoi returns an int, no need to cast that. In C you don't need to cast the return value of malloc either.

Oh, man, I totally spaced those (int)atoi(), completely redundant. The mallocs I didn't know about, chalk it up to being nervous about data alignment. I'll remove them.
Quote:
If I were you, I'd try and get this working on the PC first. It'd compile if you #define iprintf printf, change your u8 and u16 to uint8_t and uint16_t and #include <stdint.h>. That way you can use gdb or whatever to debug the problem more easily.

This is an excellent suggestion. Debugging on the console has been pretty tedious. I'll have to leave it there for the moment as it's getting late. When I get some time tomorrow, I'll see what I can figure out.

Thanks again for the pointers, Quirky

#177569 - sverx - Wed Sep 05, 2012 10:17 am

fluffypants wrote:

This is the part that I'm confused about:
Code:
iprintf("pE func: %d\n",(*pent)[7]);
*tent = (u8*) malloc((*tlen)*64);
iprintf("pE func: %d\n",(*pent)[7]);


The first print statement gives me:
23254 which is correct. After the malloc line, that same print now gives me 0. If I comment that line out then pent[7] retains the correct value.


Simply allocating memory into the heap shouldn't modify any value in your array... are you modifying pent pointer value? I mean, is your tent variable using the same memory location as pent? :|
_________________
libXM7 | NDS programming tutorial (Italiano) | Waimanu DS | A DS Homebrewer's Diary

#177576 - PypeBros - Wed Sep 05, 2012 4:56 pm

[quote="sverx"]
fluffypants wrote:

This is the part that I'm confused about:
Code:
iprintf("pE func: %d\n",(*pent)[7]);
*tent = (u8*) malloc((*tlen)*64);
iprintf("pE func: %d\n",(*pent)[7]);


The first print statement gives me:
23254 which is correct. After the malloc line, that same print now gives me 0. If I comment that line out then pent[7] retains the correct value.

That suggests to me that you should double-check plen. If it wasn't as high as you expected, then pent may have been allocated only a very small area and [7] is already beyond what malloc gave you ...
_________________
SEDS: Sprite Edition on DS :: modplayer

#177578 - elhobbs - Wed Sep 05, 2012 5:15 pm

this is a problem:
Code:
*pent = (u16 *) malloc(*plen);

this does not allocate enough memory for for *plen u16 values - so this buffer is too short by half. u16 is 2 bytes.
sb
Code:

*pent = (u16 *) malloc((*plen)*sizeof(u16));


there are more problems I am sure

#177583 - fluffypants - Wed Sep 05, 2012 8:17 pm

I think you are right elhobbs. I didn't calculate the appropriate size on either mallocs. Likely tent would have had this issue too if the data wasn't u8.

I'll test this tonight after work and try to clean up this code a bit. Thanks everyone for your help so far, I know these are amateur mistakes, but I'm learning a lot.

EDIT: Yep, that was it. I wasn't allocating enough for u16. Thanks all!

#177585 - fluffypants - Thu Sep 06, 2012 5:33 am

Quirky wrote:
Quote:
In C you don't need to cast the return value of malloc either

I cleaned up the unnecessary casts, but found that without casting malloc it spit out a compiler error "invalid conversion from 'void *' to 'u16 *'", so I left those in there.

Here's the revised function. Hopefully it's a little tidier. I would love any final input/critique about my code (style, readability, better methods).

Code:
void ReadGraphics(int *plen, u16 **pent, int *tlen, u8 **tent, char *fileName)
{
   // now, try reading a file to make sure things are working OK.
   FILE* fileIn = fopen(fileName,"rb");
   
   if(fileIn)
   {
      int i,j;
      int R, G, B;

      char line[64] = { 0 };
         
      // Get palette length
      readLine(line, fileIn);   // first line is a comment
      readLine(line, fileIn);   // Palette length
      *plen = atoi(line);
      //allocate a big enough palette
      *pent = (u16 *)malloc(*plen * sizeof(u16));
      // Get palette entries
      readLine(line, fileIn);   // line is a comment
      for(i = 0; i < *plen; i++)
      {
         readLine(line, fileIn);
         R = atoi(line);
         readLine(line, fileIn);
         G = atoi(line);
         readLine(line, fileIn);
         B = atoi(line);

         (*pent)[i] = RGB15(R, G, B);
      }

      // Get tile block len
      readLine(line, fileIn);   // line is a comment
      readLine(line, fileIn);   // number of 8x8 blocks
      *tlen = atoi(line);
            
      // Get tiles
      readLine(line, fileIn);   // line is a comment
            
      iprintf("pE func: %d\n",(*pent)[7]);
      *tent = (u8 *)malloc((*tlen) * 64 * sizeof(u8));
      iprintf("pE func: %d\n",(*pent)[7]);
            
      for(i = 0; i < *tlen; i++)
      {
         for(j = 0; j < 64; j++)
         {
            readLine(line, fileIn);
            (*tent)[i*64 + j] = (u8)atoi(line);
         }
      }

      fclose(fileIn);
   }
   else { iprintf("could not open: %s\n", fileName); }
}

#177586 - elhobbs - Thu Sep 06, 2012 7:42 am

a couple suggestions
create a struct to hold your graphics object
Code:

//define the struct
typedef struct {
  int palette_len;
  u16 *palette;
  int tile_len;
  u8 *tiles;
} graphic_t;

//declare a variable of type graphic_t
graphic_t graphic;

//modified function spec
int ReadGraphics(char *filename, graphic_t * graphic);

//call the function
if(ReadGraphics("filename",&graphic) != 0) {
   //failed to read graphic
}

this avoids the confusing pointers and pointers to pointers. the return value from ReadGraphics would be used to determine if the graphic was read correctly.

I would change
Code:
FILE* fileIn = fopen(fileName,"rb");
   
   if(fileIn)
   {
      //read data
   }
   else
   {
      iprintf("could not open: %s\n", fileName);
   }

to
Code:
FILE* fileIn = fopen(fileName,"rb");
   
   if(fileIn == 0) {
      iprintf("could not open: %s\n", fileName);
      return -1;
   }

   //read data
   return 0;

I find it a little easier to read as it keeps the error handling closer to the source of the error - in this case the fopen call

#177587 - sverx - Thu Sep 06, 2012 8:51 am

Good that the bug has been found and squashed, but I'd really like to know how that code

Code:
iprintf("pE func: %d\n",(*pent)[7]);
*tent = (u8*) malloc((*tlen)*64);
iprintf("pE func: %d\n",(*pent)[7]);


could print two different values. After all, malloc() it's only writing a pointer into *tent variable, not writing to (*pent)[7]...

Any idea?
_________________
libXM7 | NDS programming tutorial (Italiano) | Waimanu DS | A DS Homebrewer's Diary

#177589 - elhobbs - Thu Sep 06, 2012 1:01 pm

malloc allocates extra space in front of the returned buffer for housekeeping. Buffer size etc.

#177590 - sverx - Thu Sep 06, 2012 1:44 pm

elhobbs wrote:
malloc allocates extra space in front of the returned buffer for housekeeping. Buffer size etc.


There's always something new to learn :) Thanks!
_________________
libXM7 | NDS programming tutorial (Italiano) | Waimanu DS | A DS Homebrewer's Diary

#177591 - fluffypants - Thu Sep 06, 2012 7:10 pm

elhobbs, thanks for the suggestions. points well taken :)

#177609 - Quirky - Tue Sep 11, 2012 11:49 am

fluffypants wrote:
Quirky wrote:
Quote:
In C you don't need to cast the return value of malloc either

I cleaned up the unnecessary casts, but found that without casting malloc it spit out a compiler error "invalid conversion from 'void *' to 'u16 *'", so I left those in there.


ah, if you're compiling it as C++ then you do need the casts. In that case "new" is more idiomatic C++ - you'd do
Code:
new u16[*plen];
or whatever. That would have avoided the bug too probably, since you wouldn't worry about the size of the things you allocate, just the number of them.