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.

@NirTzezana: And what exactly is the problem that you're having? — jwodder
– jwodder, Commented Jan 20, 2015 at 20:23
Just as a suggestion, you should probably change your strcpy(people[i], name); and strcpy(phones[i], phone); to strncpy(people[i], name); and strncpy(phones[i], phone);. This won't solve your problem, but it will NULL terminate the string. — Spencer D
– Spencer D, Commented Jan 20, 2015 at 20:25
Oh and as a second suggestion, please NEVER do this: people = realloc(&people,(storage)*sizeof(char *));. If the realloc fails, NULL is returned which means people becomes a NULL pointer. Thus you lose the pointer to that block of memory. You should assign the return value of realloc to a separate variable and if ( NULL != returnedReallocAddress ) people = returnedReallocAddress; — Spencer D
– Spencer D, Commented Jan 20, 2015 at 20:30

Iharob Al Asimi · Accepted Answer · 2015-01-20 21:04:30Z

8

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.

edited Jan 20, 2015 at 21:04

answered Jan 20, 2015 at 20:40

Iharob Al Asimi

53.1k6 gold badges66 silver badges100 bronze badges

Sign up to request clarification or add additional context in comments.

11 Comments

Nir Tzezana Over a year ago

Thanks, the program still crashes when trying to extand the array with realloc.

user3386109 Over a year ago

There is a third important mistake: In C, strings are an array of characters terminated by a NUL character '\0'. The length returned by strlen does not include the NUL character. So all of the buffers that the OP uses are too small by 1 character.

Nir Tzezana Over a year ago

@user3386109 Thanks, I updated this to strlen(name)+1)

Iharob Al Asimi Over a year ago

@user3386109 abslutely right, I didn't notice that.

Iharob Al Asimi Over a year ago

@NirTzezana if you want you can contact me [email protected]

|

Weather Vane · Accepted Answer · 2015-01-20 20:48:07Z

4

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?

answered Jan 20, 2015 at 20:48

Weather Vane

34.8k7 gold badges43 silver badges63 bronze badges

6 Comments

Spencer D Over a year ago

Weather, I believe the OP has pasted pieces of the actual code rather than including all of the actual code. I would guess the variables which are commented as "global" are likely defined globally and the code following them is likely within some function which he did not include the declaration for. That said, the code is a tad confusing because of that.

Nir Tzezana Over a year ago

Are you talking about storage, it needs to be global beacuse its being used in many functions. You can see the full code here: pastebin.com/45grLUrs

Nir Tzezana Over a year ago

@SpencerDoak I thought it would be better to paste parts of the code instead the whole code, beacuse its shorter and more focused on the part that isn't working.

Weather Vane Over a year ago

@NirTzezana if people and phones are global why did you need to pass them to AddNewContact()?

Spencer D Over a year ago

@NirTzezana, yes only showing the relevant pieces of code is usually preferred on SO, but might I recommend for future questions that you include the function declarations (e.g. returnType functName(params) { ... }). This allows readers to notice separations between functions and global variables. That said, thank you for being conservative and not throwing all of your code into the question. It can be quite overwhelming to help someone who posts every line of their project.

|

Collectives™ on Stack Overflow

realloc an array of pointers is doing nothing

2 Answers 2

11 Comments

6 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

11 Comments

6 Comments

Your Answer

Sign up or log in

Post as a guest

Related