realloc an array of pointers is doing nothing

Question

I have an array of strings, and I would like to extand it when it no longer has NULL pointers (meaning the array is full). I have tried realloc with no success, I think i'm not thinking right pointer-wise.

Here is my code:

int storage; //global, outside of main
int i, key;
char **people;
char **phones;

printf("Please enter a storage cacpity:\n");
scanf("%d",&storage);
printf("\n");

people=malloc(storage*sizeof(char *));
phones=malloc(storage*sizeof(char *));

for (i=0; i<storage; i++) {
    people[i] = NULL;
    phones[i] = NULL;
}

void AddNewContact(char * people[], char * phones[]) {
    char name[100];
    char phone[12];
    int i, listfull = 0;

    printf("Enter a contact name:\n");
    scanf("%s",&name);
    printf("Enter a phone number:\n");
    scanf("%s",&phone);

    for (i=0; i<storage; i++) {
        if (people[i]==NULL) {
            people[i] = (char *)malloc(strlen(name));
            phones[i] = (char *)malloc(strlen(phone));
            strcpy(people[i],name);
            strcpy(phones[i],phone);
            break;
        }
        listfull = 1;
    }

    if (listfull == 1) {
        storage++;
        people = realloc(&people,(storage)*sizeof(char *));
        phones = realloc(&phones,(storage)*sizeof(char *));
        people[storage-1] = NULL;
        phones[storage-1] = NULL;
        strcpy(people[storage-1],name);
        printf("\nData Base extanded to %d",storage);
    }
    printf("\n");
    return;
}

void PrintAll(char * people[], char * phones[]) {
    int i;
    for (i=0; i<storage; i++) {
        if (NULL != people[i]) {
            printf("Name: %s, ",people[i]);
            printf("Number: %s\n",phones[i]);
        }
    }
    printf("\n");
    return;
}

Any help would be highly appreciated, I'm stuck on it for a few hours and having no luck sovling this.

Answer 1

You have 4 important mistakes, first you are passing the address of the array to scanf() that's wrong, you should change

scanf("%s", &name);

to

scanf("%s", name);

as well as scanf("%s",&phone);, I also should recommend to use length specifiers for scanf to prvent buffer overflow, for example

scanf("%99s", name);

i.e. the length of the name array -1, for the '\0' terminator.

Second, your realloc call is also wrong, you should pass the pointer instead of it's address, instead of this

people = realloc(&people,(storage)*sizeof(char *));

you should do this

people = realloc(people, storage * sizeof(char *));

but even this is not 100% right, because in case realloc fails, you will overwrite the pointer, and then you wont have a chance to clean up memory, so you should actually do something like

void *pointer;
pointer = realloc(people, storage * sizeof(char *));
if (pointer == NULL)
    free_people_andCleanUpOtherResourcesAndExitFromThisFunction();
people = pointer;

the same goes for phones.

Third you should always allocate space for one extra character, the terminating '\0', this

people[i] = (char *)malloc(strlen(name));

should read

people[i] = malloc(1 + strlen(name));

notice that I removed the cast which is not necessary.

Fourth you break out from the loop in the first iteration leaving listfull == 1, even when there is list is not full yet.

for (i=0; i<storage; i++) {
    if (people[i]==NULL) {
        people[i] = malloc(1 + strlen(name));
        phones[i] = malloc(1 + strlen(phone));
        strcpy(people[i],name);
        strcpy(phones[i],phone);
        break;
    }
    listfull = 1;
}

I'd recommend this outside of the loop

listfull = (i == storage);

Note: it doesn't matter how unlikely a function will fail, if it theoretically would fail, you should always check for it's failure, that will save you hours of debugging to find a very stupid mistake, you didn't check for the possible failure.

Answer 2

You have

listfull = 1;

inside your (for i ...) loop, it should be outside the loop, like this

if (i == storage)           // if loop completed
    listfull = 1;

Next, your partial program comments that some variables are declared globally, yet they are followed by executable code statements which must be within a function, so are the "global" variables actually, local?