0

I attempted implementation of a new feature on working code. However, I know from commenting out that the following lines of code are causing my program to crash.

tmp = (char*)realloc(list->array[list->size], sizeof(char)*nameLength);
list->array[list->size] = tmp;

list->array[list->size] is defined in a struct in a .h file. list->size is an int that increases as the size of the array increases. I know that the .h file is correct and am pretty certain the rest of my code was written correctly because I tested it all. However I can't seem to figure out this realloc function. Can somebody please help me? I've searched stackoverflow and couldn't find the answer. This is my first time posting a question on stackoverflow. I hereby promise to help others on this site if I can get some help with this problem.

typedef struct ArrayList
{
// We will store an array of strings (i.e., an array of char arrays)
char **array;

// Size of list (i.e., number of elements that have been added to the array)
int size;

// Length of the array (i.e., the array's current maximum capacity)
int capacity;

} ArrayList;

I didn't want to clutter the post with long code. But just in case someone can find something I overlooked I am posting my main.c.

int main(void)
{
    int i;
    char buffer[32];
    ArrayList *L1 = createArrayList(-1);

    FILE *ifp = fopen("names.txt", "rb");
    //Read all names from the file and add them to L1.
    while (fscanf(ifp, "%s", buffer) != EOF)
        put(L1, buffer);

    //printf("the size is %d", L1->size);   //test

    for(i=0; i<L1->capacity; i++) { //free columns
        free((void*)L1->array[i]);
    }
    free((void*)L1->array); //free rows
    free((void*)L1); // free struct
    system("pause");
    return 0;
 }

 char * put(ArrayList *list, char *str)
 {
    int i = 0,length = 0, nameLength;
    char* tmp, tmp2;

  //free((void*)list->array[list->size]);   //free columns
  //list->array[list->size] = NULL;         //set old pointer to NULL
    nameLength = strlen(str)+1;             //Aquire length of name +1
    tmp = (char*)realloc(list->array[list->size], sizeof(char)*nameLength);
    list->array[list->size] = tmp;
  //list->array[list->size] = str;
  //strcpy(list->array[list->size], str);
    printf("the name in the .txt file is %s: put \n", list->array[list->size]);

  //if array is full add space
    length = (list->capacity)*2 + 1;
  //if ( list->capacity <= list->size ) {
        ArrayList *nlist = expandArrayList(list, length);
  //}
    list->size ++;
 }

 ArrayList *expandArrayList(ArrayList *list, int length)
 {
    int i;
    //create newCharArray
    char **newCharArray = (char*) malloc(sizeof(char*)*(length+1));  //alocate rows//2
    for(i=0; i<length+1; i++) {  // should fill ten rows + 1//2
        newCharArray[i] = malloc(100 * sizeof(char*));          //alocate Columns
        newCharArray[i] = "yoO";
    }
    //copy old list-Array into newCharArray
    for( i=0; i<list->capacity; i++) { //was length
        newCharArray[i] = list->array[i];
    }
    //print the values you inserted     //test
    for( i=0; i<list->capacity + 1; i++) {//2
        printf("the value in slot %d is %s : expandArrayList \n", i, newCharArray[i]);
    }
    //free the list->array columns and rows
    for( i=0; i<list->capacity; i++) {
        free((void*)list->array[i]);
    }
    free((void*)list->array);
    //set adress of newCh.. = our old Struct
    list->array = newCharArray;
    //print to test we did what was asked //test
    for( i=0; i<length + 1; i++) {//2
        printf("value in slot %d is %s : expandArrayList \n", i, list->array[i]);
    }

    return list;
 }

 ArrayList *createArrayList(int length)
 {
    int ncolumns, nrows, i;
    char stringVar[5]; //set rows = empty
    if(length < DEFAULT_INIT_LEN)
        length = DEFAULT_INIT_LEN;
    nrows = length;

    ArrayList *theArray;
    theArray = (ArrayList*)malloc (sizeof(struct ArrayList));  //alocate struct
    theArray->array = (char*)malloc(sizeof(char*)*(nrows)); //alocate Rows 10th row will be null

    for(i=0; i<nrows; i++) {  // should fill ten rows
        theArray->array[i] = (char*)malloc(2 * sizeof(char)); //alocate Columns //realloc later
        theArray->array[i] = '\0';  //(char*)"pointer char will be returned malloc(100*sizeof(char): char*100 will be alocated
    }

  //theArray->array[nrows] = '\0'; //should fill the 11'th pointer slot array index 10
    theArray->capacity = nrows; //total rows that are available for use ("not including last null")
    theArray->size = 0; //Amount of rows used
    printf("capacity = %d :createArraylist \n", theArray->capacity);

    return theArray;
}
Matt
  • 22,721
  • 17
  • 71
  • 112
ShowLove
  • 899
  • 1
  • 12
  • 21
  • in `put()`, you have commented out `strcpy(list->array[list->size], str);`. Why? – chux - Reinstate Monica May 27 '13 at 19:45
  • 1
    In C, don't cast the return value of `malloc` or `realloc`, this can hide bugs, easily. If you didn't `#include` the right header, this could just be your error. – Jens Gustedt May 27 '13 at 19:48
  • What is the value of the macro DEFAULT_INIT_LEN? – Étienne May 27 '13 at 19:52
  • Wait, what? `newCharArray[i] = "yoO";`? Seriously? You know, that throws awway every one of those `malloc(100 *sizeof(char *))` calls, replacing each with a pointer to a string literal... – This isn't my real name May 27 '13 at 19:53
  • Also, in C, you should never cast the return value of `malloc()` or `realloc()`, nor should you cast the pointer that you're passing to `realloc()` or `free()`, and multiplying by `sizeof(char)` is pointless. `sizeof(char)` is *guaranteed* to be `1`. – This isn't my real name May 27 '13 at 19:55
  • 2
    Multiplying by `sizeof(char)` is indeed mathematical pointless because it is 1, but it has usefulness in documenting that the code was allocating a char array. In any case, I rather promote instead `nameLength*sizeof(*tmp)` – chux - Reinstate Monica May 27 '13 at 20:05

2 Answers2

1

In your expandArrayList() function, you're wandering down your array, allocating storage for strings. After each allocation, however, you're immediately throwing away your pointer to allocated storage, and replacing it with a pointer to "yoO", a string literal. String literals are arrays of char, but they are not allocated with malloc(), do not belong to you, and you aren't allowed to free() or realloc() them. Slightly further down the same function, however, when you're trying to free the previous copy of the array list, guess what you're passing to free()?

Edit

In createArrayList(), you do nearly the same thing, except that each allocated pointer that you throw away, you're replacing with '\0', which is, effectively, a null pointer in this context.

Not to mention, how in expandArrayList(), immediately after filling each slot with a newly malloc()ed piece of memory (and then throwing it away...), you immediately copy the pointers from the old array list (which you subsequently also free()...)

This isn't my real name
  • 4,869
  • 3
  • 17
  • 30
  • I deleted the casts ,"yoO" and '0' and made the other minor changes suggested. the code ran smoothly. The only thing i still don't understand is how to correctly free space for the char** in the struct ArrayList. i want to make sure my destroy function doesn't segfault when passed a null pointer. here's my function. did i do it correctly? – ShowLove May 29 '13 at 07:14
  • ArrayList *destroyArrayList(ArrayList *list){ int i; if( list == '\0' ) return '0'; for(i=0; icapacity; i++){ //free columns free((void*)list->array[i]); } free((void*)list->array); //free rows free((void*)list); // free struct list = '\0'; return list; } – ShowLove May 29 '13 at 07:21
  • BTW i learned much from your comments. I extend my gratitude. You guys have encouraged me to help others on stackoverflow. this website is an awesome idea i hope this post helps others. – ShowLove May 29 '13 at 07:24
  • You're still casting the pointers that you're passing to `free()`. You shouldn't. Also, `'\0'` is a character constant, not a null pointer. In pointer context, you should be using `NULL` for assignment and comparison, not `'\0'`. – This isn't my real name May 29 '13 at 14:57
1
char **newCharArray = (char*) malloc(sizeof(char*)*(length+1));

There is a problem in your allocation, you cast to char* instead of char**, and your variable is named CharArray whereas it's an array of C strings. Anyway in C you don't need to cast the return of malloc, just remove the cast completely.

for(i=0; i<length+1; i++) {  // should fill ten rows + 1//2
    newCharArray[i] = malloc(100 * sizeof(char*));          //alocate Columns
    newCharArray[i] = "yoO";
}

In expandArrayList you forgot a level a dereferenciation, you probably wanted:

for(i = 0; i<length+1; i++) {
    newCharArray[i] = malloc(100 * sizeof(char*));
    strcpy(newCharArray[i], "yo0");
}

And in createArrayList you did the same error:

for(i=0; i<nrows; i++) {  // should fill ten rows
    theArray->array[i] = (char*)malloc(2 * sizeof(char)); //alocate Columns //realloc later
    theArray->array[i] = '\0';  //(char*)"pointer char will be returned malloc(100*sizeof(char): char*100 will be alocated
}

You wanted to do something like this:

for(i=0; i<nrows; i++) {
    theArray->array[i] = malloc(2);
    *(theArray->array[i]) = '\0';
}
Étienne
  • 4,773
  • 2
  • 33
  • 58