multidimensional malloc() and free()

[netraven5000]

Hello,

I'm writing some C code for a class, and as part of it, I'm dynamically creating two two-dimensional arrays.

One of them is a dynamic array of dynamic arrays - so I know that to clean that up I need to loop through each array and use free() before I use free() on the whole thing. But one of them is an array of arrays (strings) that the program didn't create (it's basically an array of different filenames taken from the command line).

Am I supposed to do a "looped free()" in this case too? I know normally when it's not a dynamically-created array it gets cleaned up automatically - is that also the case when you stick it inside a dynamic array?

[netraven5000]

Too quick to ask... sorry, just had my question answered by my computer:

*** glibc detected *** ./tee2: free(): invalid pointer: 0x000000000040111c ***

So I guess no, you shouldn't do it.

[Salem]

> Am I supposed to do a "looped free()" in this case too?
Yes, every allocation you do must have a corresponding deallocation.

In C
malloc -> free

And for C++
new -> delete
new [ ] -> delete [ ]

> *** glibc detected *** ./tee2: free(): invalid pointer: 0x000000000040111c ***
That just means you screwed it up, not that you shouldn't do it.

You would need to post code, but my guess is you did ptr = malloc( strlen(myString) ); where you should have done ptr = malloc( strlen(myString) + 1 );

[Dave Sinkula]

http://c-faq.com/aryptr/dynmuldimary.html

[netraven5000]

http://c-faq.com/aryptr/dynmuldimary.html

No I think maybe I wasn't clear.

I have two multidimensional arrays. I have a two-dimensional array of characters (which are filenames), and I have a multidimensional array full of file descriptors (which gets filled using pipe()).

The code dealing with the arrays look like such:

#include <unistd.h> // needed for pipe(), fork()
#include <stdio.h>  // needed for fopen()
#include <stdlib.h> // needed for exit()
#include <string.h> // needed for strlen()
#include <signal.h>  // needed for signal handling
#include <limits.h> // needed for PIPE_BUF

int main(int argc, char** argv){
  int index,  // internal loop index
      **fileDescs,  // array of file descriptors for piping
      fileNamesSize,  // size of fileNames array
      fileCount;  // number of files to pipe to
  char **fileNames; // names of files to write to
  fileNamesSize = 2;
  fileNames = malloc(fileNamesSize * sizeof(char));
  
  fileCount = 1;  // 1 because we always have stdout
  fileNames[0] = "";
  
  for(index = 1; index < argc; index++){
    if(argv[index][0] == '-'){
    }
    // if '-' is not at the beginning, this is a filename
    else{
      if (fileCount * 2 >= fileNamesSize){
        fileNamesSize = fileNamesSize * 2;
        fileNames = realloc(fileNames, fileNamesSize * sizeof(char *));
      }
      fileNames[fileCount] = argv[index];
      fileCount++;
    }
  }
  
  // dynamically allocate an array to hold arrays of file descriptors
  fileDescs = malloc(fileCount * sizeof(int *));
  if (fileDescs == NULL){
    perror("Memory error");
    exit(EXIT_FAILURE);
  }
  
  for (index = 0; index < fileCount; index++){
    // allocate the space for the inner array(s) for our file descriptors
    fileDescs[index] = malloc(2 * sizeof(int));
    if (fileDescs[index] == NULL){
      perror("Memory error");
      exit(EXIT_FAILURE);
    }
  }
  
  // we're done with fileNames - let's clean it up
  free(fileNames);
  for (index = 0; index < fileCount; index++){
    close(fileDescs[index][1]);
    free(fileDescs[index]);
  }
  free(fileDescs);
  exit(EXIT_SUCCESS);
}

See, at the end I run a loop to clear out my arrays of file descriptors before I clear out my array of arrays of file descriptors. But when I tried to do the same thing with my array of arrays of chars, I got that error - which makes me think that maybe since they're not dynamically created I shouldn't run free() on each "sub-array" like I should for my file descriptors.

[Dave Sinkula]

See, at the end I run a loop to clear out my arrays of file descriptors before I clear out my array of arrays of file descriptors. But when I tried to do the same thing with my array of arrays of chars, I got that error - which makes me think that maybe since they're not dynamically created I shouldn't run free() on each "sub-array" like I should for my file descriptors.

Yeah. And Salem already answered that.

I didn't know whether or not there was confusion with malloc/free for multidimensional dynamic arrays.

[netraven5000]

I think I just forgot for a moment there how memory allocation works. You don't get pretty things happening when you deallocate memory you didn't allocate.

#include <stdlib.h>

int main(){
  int test[2];
  free(test);
}

Compiling this gives you:
test2.c:5: warning: attempt to free a non-heap object test

Then running gives a segmentation fault. But this compiles and runs fine:

#include <stdlib.h>

int main(){
  int *test;
  test = malloc(2);
  free(test);
}

I see now - the system is planning to clean up static arrays on its own, so you don't run free() to clean them up, and since the original arrays are now in the new array that means you don't free() those like you do the dynamic arrays. Makes sense. I forgot - in C you're passing memory locations around, not values. So that array in there is a static array - it's not a dynamic array that's a copy of a static array.

[Salem]

> fileNames = malloc(fileNamesSize * sizeof(char));
One obvious mistake, you're not allocating ENOUGH memory here.
It's an array of char*, not chars.

Use this form for calling malloc - works every time fileNames = malloc( fileNamesSize * sizeof(*fileNames) ); > fileNames[0] = "";
Unless your free loop makes a special case of 0, then blindly trying free( fileNames[0] ); will blow up as well.
It's a completely unnecessary step IMO, just use the first slot for the first filename and the problem goes away.

I'll discuss your realloc mistakes later on.

[netraven5000]

One obvious mistake, you're not allocating ENOUGH memory here.
It's an array of char*, not chars.

I'm not sure what happened there - I know that's wrong (not what you said but what I said) because it's a pointer to a pointer. I know this forum was giving me trouble yesterday, whenever I tried to post it told me that I wasn't logged in yet (even though it had "Logged in as netraven5000"...) so I had to copy, hit back, then paste - must've messed something up by accident. What I have in the file is this:

fileNames = malloc(fileNamesSize * sizeof(char *));

Which I'm positive is fine.

Unless your free loop makes a special case of 0, then blindly trying free( fileNames[0] ); will blow up as well.
It's a completely unnecessary step IMO, just use the first slot for the first filename and the problem goes away.

No, the child processes check for the filename "" and use stdout instead of a regular file. I have this hardcoded because the program always writes to stdout in addition to any files taken as arguments.

I'll discuss your realloc mistakes later on.

The idea was that if at least half the array is already filled, I'll double the size. Is that not what it's doing? (Notice - fileCount starts at 1, and is not incremented until after the file is added to the array.)
So if fileCount is 1 and fileNamesSize is 2, I double fileNamesSize to get 4 and then increase the size of fileNames (which has slot 0 taken already) to 4 so that I now have 3 available storage spaces.
Maybe it's not the most efficient way to do it, but I don't see any reason why it wouldn't be allocating enough space. Are you talking about the fact that it allocates too much space? Because this array can be free()d almost immediately (and ultimately that's what I want to do). The point was to not have to realloc() every time.

[Salem]

It seems I was wrong about the freeing of filenames, since all you're doing is pointing at argv.

Which brings me to the next point.
The only reason to use realloc is when you don't know how many things you're going to get. In this case, you do - it's in argc.

Now you do strip out all the argv's that begin with a minus, but that's hardly going to be a significant number (compared to the number of filenames.

It would on the whole be a hell of a lot simpler just to have:

char **filenames = malloc( argc * sizeof *filenames );

You of course continue to count the number of valid filenames, but it's not going to be that much smaller than argc (and certainly not larger than argc).

The realloc mistake I mentioned was you're trashing your only pointer, should realloc fail. Here's how the code should look:

void *temp = realloc( ptr, ( number + increase ) * sizeof(*ptr) );
if ( temp != NULL ) {
  // it's all good
  ptr = temp;
  number += increase;
} else {
  // original ptr and number are still good.
  // perhaps a 'panic' save, or some other recovery action
}

One final bit of magic for your fileDescs array. Since the inner dimension is a constant, you can do the whole allocation in a single call.

int (*fileDescs)[2];
fileDescs = malloc( argc * sizeof(*fileDescs) );

This gives you in a single contiguous block fileDescs[0][0] to fileDescs[argc-1][1]

[netraven5000]

Well I've already turned in my code so it's done. Thanks for the help!

I decided to just make fileNames an array of size argc of pointers (char *fileNames[argc]). There's not much benefit to doing malloc() and realloc() - the professor will try to "break" the program but there's no way he's going to add enough arguments to have argc wrap around or something, and on normal use most of the arguments would be filenames unless there's only a few arguments.

Your trick for the fileDescs array though - I just left that alone, we haven't seen any of that sort of syntax in class at all, and I'm not fully sure I understand how or why what you did works, so I figure it's better NOT to throw it in there in case I get asked about it... but would you be able to point me somewhere where this syntax gets explained? I noticed that if I removed the parenthesis around *fileDescs it gets confused - are you basically doing two declarations at once or something? (I tried to Google for an answer but didn't really get anywhere...)