0

I have a struct that contains an array of strings (char arrays) and an int for the maximum capacity of the array. 

typedef struct ArrayList
{
    char **array;
    int capacity;
}

The array is initialized with malloc in its own method.

list->array = malloc(sizeof(char *) * templength);

And the individual strings are initialized as

list->array[nextEmptyString] = malloc(sizeof(str));
strcpy(list->array[nextEmptyString], str);

I need to be able to clear everything about that array from memory and then pass a new pointer to that array, however when I attempt to free the individual strings before freeing the array it scrambles the new array.

for (word = 0; word < list->capacity; word++)
    free(list->array[word]);
free(list->array);
list->array = newArray;

Where list is an ArrayList and newArray is an array of strings. The code runs correctly if I comment out the for loop, but then don't i just have a bunch of orphaned strings floating around in memory?

The intended output is something like Adding Pierre to L1 ... and instead I get Adding �c� to L1 ...

  • 1
    how the memory is allocated? `malloc` or `calloc`? – Sakthi Kumar Feb 01 '14 at 07:22
  • 3
    @SakthiKumar I don't see why that would matter at all. –  Feb 01 '14 at 07:24
  • 1
    @SakthiKumar Who cares? – Brian Roach Feb 01 '14 at 07:24
  • 1
    @H2CO3 it would make an enormous difference if the address in the pointer-array are all offset from one *single* allocation (in which case you would only remove the first one and the pointer-array itself). the *function* means nothing, however. and to that I prop your comment =P – WhozCraig Feb 01 '14 at 07:26
  • If the original list->array is setup using the method like: list->array = originalArray, then the for() loop is NOT needed. – Gary Yin Feb 01 '14 at 07:26
  • 1
    @WhozCraig Hm. Sorry, I have no idea what you are referring to. `ArrayList::array` is a pointer-to-pointer-to-char. `ArrayList::array[i]` is a pointer-to-char. Both are being `free()`d, the inner one in a loop. This means that both have been allocated using a standard allocator function, such as `malloc()` (or something else, which is irrelevant). There are no offsets involved with this whatsoever. There's an array of pointer of which the members point to a string each. This can also be deduced from the fact that there's a `capacity` member actively used in the deallocation process. –  Feb 01 '14 at 07:30
  • @SakthiKuma it was with `malloc` `list->array = malloc(sizeof(char *) * templength);` –  Feb 01 '14 at 07:32
  • just because there is space allocated for the pointer, that doesnt mean that it was set. you should call: `memset(list->array, 0, sizeof(char *) * templength)` – chacham15 Feb 01 '14 at 07:34
  • @chacham15 **No.** `free(NULL);` is a no-op, it's perfectly legal. –  Feb 01 '14 at 07:34
  • @H2CO3 take the principle as a whole. there is no assurance that the members were set at all. – chacham15 Feb 01 '14 at 07:36
  • @chacham15 No, there isn't. And? It's still valid to `free()` a null pointer. You don't have to do `if (array[i] != NULL) free(array[i]);`. –  Feb 01 '14 at 07:37
  • @H2CO3 You know *exactly* to what I'm referring, but I probably said it poorly. After allocating `array` to `capacity` entries, a single buffer something to the effect of `char *p = malloc(capacity*ENTRY_SIZE);` then looping and populating each `array` member with `p+i*ENTRY_SIZE`. In other words, just because there is a big stack of pointers doesn't mean they were all the result of individual allocations. Assuming `capacity` refers to both the size of the pointer array *and* the number of allocations made to populate it isn't a sure thing. I hope that was at least somewhat clearer. – WhozCraig Feb 01 '14 at 07:38
  • @H2CO3 memory returned by malloc isnt zeroed on all oses. forget the null check. – chacham15 Feb 01 '14 at 07:38
  • @WhozCraig Oh, I now see what you are saying, thanks. But I don't think you are right in this case. "In other words, just because there is a big stack of pointers doesn't mean they were all the result of individual mallocs." - of course that is true, but here they **were** indeed mallocated individually, since OP is freeing each of them in a separate call to `free()`. Or am I missing something? I think one can't `free()` a pointer that wasn't `malloc()`ed before. –  Feb 01 '14 at 07:41
  • @WhozCraig you are basically saying the same thing as I am, only in a much more confusing manner. i.e. just because space exists for the pointers, that doesnt mean that it was set. – chacham15 Feb 01 '14 at 07:41
  • 1
    @chacham15 "memory returned by malloc isnt zeroed on all oses" - in fact, it is - fortunately - hardly ever set to 0. I know that. –  Feb 01 '14 at 07:42
  • @chacham15 No, he's referring to something else. –  Feb 01 '14 at 07:42
  • @H2CO3 your statement makes no sense. I am saying that he is freeing an invalid pointer which is causing the error. – chacham15 Feb 01 '14 at 07:43
  • 1
    @chacham15 No, I'm saying just because there's space for pointers doesn't mean they all came from individual mallocs *even if they're all valid pointers*. Your point of whether the pointers themselves are valid at all, is good, but a separate issue. – WhozCraig Feb 01 '14 at 07:43
  • @WhozCraig ohh, youre saying that they were from alloca's or some equivalent? – chacham15 Feb 01 '14 at 07:44
  • 1
    How do you fill the array? – Dmitri Feb 01 '14 at 07:44
  • @chacham15 (apparently, my statement does make sense, and it is true.) –  Feb 01 '14 at 07:44
  • 1
    @chacham15 or a big-ol-malloc. Whatever. – WhozCraig Feb 01 '14 at 07:45
  • @Dmitri `list->array[nextEmptyString] = malloc(sizeof(str));` is how they are all initialized, then `strcpy(list->array[nextEmptyString], str);` –  Feb 01 '14 at 07:47
  • @WhozCraig Oh I see, a better example to demonstrate what you mean would be: `list->array[0] = strcpy(malloc(6), "hello"); list->array[1] = &(list->array[0][3]);` – chacham15 Feb 01 '14 at 07:48
  • 1
    @H2CO3 just saw your comment. sry, man. been a long day. yeah, no freeing that which wasn't a direct allocation return (you know that=P). I think you're likely right about the N+1 allocations, but if so, then barring someone stealing the resources and freeing them separately, or corruption of the heap somewhere else (or the pointer array of course, choose your favorite UB) this code should work. We = need sample =P – WhozCraig Feb 01 '14 at 07:48
  • 3
    `malloc(sizeof(str))` looks wrong... maybe `malloc(strlen(str)+1)`? – Dmitri Feb 01 '14 at 07:49
  • @Dmitri better, but you need a +1 for the NULL terminator `malloc((1+strlen(str))*sizeof(char))` – chacham15 Feb 01 '14 at 07:49
  • 1
    @chacham15 I know... just submitted the comment too soon – Dmitri Feb 01 '14 at 07:50
  • @WhozCraig "We need sample" -- **exactly.** :-) (but if you are right, then your idea **is** indeed the solution: maybe OP is trying to `free()` a pointer that was computed with your offset-based approach.) –  Feb 01 '14 at 07:50
  • 1
    @Dmitri you rock man. pony that up for an answer and I'll quickly up-vote it, hell, even if it *doesn't* solve the problem (and I think it may =P) – WhozCraig Feb 01 '14 at 07:51
  • @Dmitri my malloc should be fine, since str already ends in a NULL terminator (I would expect it to give me a lot more errors if it was wrong). Adding + 1 did nothing to fix it as far as I can tell. –  Feb 01 '14 at 07:55
  • @user3259860 No, your malloc is wrong since you do not understand what the `sizeof` operator does. –  Feb 01 '14 at 07:56
  • 3
    The problem is using `sizeof` instead of `strlen`, as well. – Dmitri Feb 01 '14 at 07:56
  • 2
    @user3259860 If your `str` is anything besides a fixed length array (`char str[some size here])` it is *not* fine. Show what `str` comes from. Where is the variable declared (is it a param, a char*, a char[]` ??) The *exact* declaration and surrounding line or two of code, please. – WhozCraig Feb 01 '14 at 07:57
  • @H2CO3 Just fyi about freeing null pointers: http://stackoverflow.com/a/1938791/516813 – chacham15 Feb 01 '14 at 08:07
  • @WhozCraig str is a `char *` passed into a function `char *put(ArrayList *list, char *str)`, which is then used `list->array[nextEmptyString] = malloc(strlen(str)+1)` `strcpy(list->array[nextEmptyString], str)` –  Feb 01 '14 at 08:11
  • Let's see the code that prints your output, then... – Dmitri Feb 01 '14 at 08:17
  • 2
    You should supply a complete program so that we don't have to guess what you are doing. – David Heffernan Feb 01 '14 at 08:18
  • @Dmitri `for (count = 0; count < stringsInArray; count++) printf("%s\n", list->array[count]);` –  Feb 01 '14 at 08:19
  • 1
    @chacham15 Did you read the first comment to that answer? H2CO3 said the same thing earlier, unless of course you aren't only talking about standards compliant implementations of C. – ajay Feb 01 '14 at 10:08

2 Answers2

4

When allocating the strings in the array, 

malloc(sizeof(str))

only allocates enough space for a char *, rather than the the string it points to (unless str was an ordinary array rather than a pointer). Instead, try 

malloc(strlen(str) + 1)

to allocate enough room for the characters in the string plus a terminating null.

Dmitri
  • 9,175
  • 2
  • 27
  • 34
  • changing my mallocs to `malloc(strlen(str) + 1)` didn't seem to fix the freeing problem. The intended output is something like `Adding Pierre to L1 ...` and instead I get `Adding �c� to L1 ...` –  Feb 01 '14 at 08:13
1

You should use strdup function from string.h

list->array[nextEmptyString] = malloc(sizeof(str));
strcpy(list->array[nextEmptyString], str);

seems better as:

list->array[nextEmptyString] = strdup(str);

And the problematic for: Did you initialize a capacity?

sgflt
  • 68
  • 3
  • 10