CODEWITHSUNDEEP
carraysstructdynamic-allocation
Mr Tsjolder from codidact
Mr Tsjolder from codidact

Reputation: 2876

How to store data in a dynamic array of structs?

I have these structs with which I would like to implement a map

typedef struct {
    const char *name;
    int number;
}   Entry;

typedef struct {
    int available;
    int guard;
    Entry *entries;
} Map;

and code to work to initialise and put elements in it:

Map *map_init() {
    Map *res = (Map *) malloc(sizeof(Map));

    res->available = 4;
    res->guard = 0;
    res->entries = (Entry *) malloc(4 * sizeof(Entry));

    return res;
}

int map_put(Map *map, const char *name, int nr) {
    Entry entry;
    int i = 0;

    for (i = 0; i < map->guard; ++i) {
        entry = map->entries[i];
        printf("entry (  x , %u) at %p (%p)\n", entry.number, &entry, entry.name);

        if (!strcmp(entry.name, name))        // Segmentation fault here
            return 0;
    }

    entry = map->entries[map->guard++];
    entry.name = name;
    entry.number = nr;

    printf("entry (%s, %u) at %p (%p)\n", entry.name, entry.number, &entry, entry.name);

    return 1;
}

when I run my main method

int main(int argc, char **argv) {
    printf("Initialising...\n");
    Map *map = map_init();

    printf("Putting...\n");
    map_put(map, "test", 2);
    map_put(map, "some", 1);

    // ...

    free(map->entries);
    free(map);
    return 0;
}

I get as output

Initialising...
Putting...
entry (test, 2) at 0x7fff50b32a90 (0x10f0cdf77)
entry (  x , 0) at 0x7fff50b32a90 (0x5000000000000000)
Segmentation fault: 11

from which I could derive that the segmentation fault is due to the fact that entry.name does not point to a string anymore (also the number is lost, but this does not lead to unauthorised memory access). After I set the data in the first invocation of map_put, everything seems to be stored in the correct places.

Anyone an idea where these entries could be overwritten or why the values are not stored?

Upvotes: 2

Views: 746

Answers (3)

Serge Ballesta
Serge Ballesta

Reputation: 148910

You have a major problem in map_put. You use a local Entry, in which you copy entries from the map. But when you later assign value to the local copy, the original entries from the map are left unchanged.

So when you later try to compare the new name to the existing entry, you are comparing it to uninitialized values, which is Undefined Behaviour.

You should use an Entry * instead: 

int map_put(Map *map, const char *name, int nr) {
    Entry *entry;
    int i = 0;

    for (i = 0; i < map->guard; ++i) {
        entry = map->entries + i;
        printf("entry (  x , %u) at %p (%p)\n", entry->number, entry, entry->name);

        if (!strcmp(entry->name, name))        // Segmentation fault here
            return 0;
    }

    entry = &map->entries[map->guard++];
    entry->name = name;
    entry->number = nr;

    printf("entry (%s, %u) at %p (%p)\n", entry->name, entry->number, entry, entry->name);

    return 1;
}

But that is not all. You just store the address of a string in name. It is fine in this example, because you are actually passing string litteral constants. But if you read strings from standard input or a file, the content of the buffer will be overwritten with each new value. As you only store the address, you will end with all entries pointing to same value: the last one.

IMHO you should comtemplate using strdup to store copies of the string - and free them at the end. BTW, as you have an init function to initialize you Map, you should build a cleanup one to have all necessary free in one single place.

Upvotes: 1

user3386109
user3386109

Reputation: 34829

The problem is that variable entry in map_put is not a pointer. It is a structure. So the code

entry = map->entries[map->guard++];
entry.name = name;
entry.number = nr;

copies the contents of map->entries[map->guard] into entry. Then you update the fields in entry and return from the function.

The correct code looks like this

int map_put(Map *map, const char *name, int nr) {
    Entry *entry;  // <-- entry is a pointer
    int i = 0;

    for (i = 0; i < map->guard; ++i) {
        entry = &map->entries[i];
        printf("entry (  x , %u) at %p (%p)\n", entry->number, (void *)entry, (void *)entry->name);

        if (!strcmp(entry->name, name))
            return 0;
    }

    entry = &map->entries[map->guard++];
    entry->name = name;
    entry->number = nr;

    printf("entry (%s, %u) at %p (%p)\n", entry->name, entry->number, (void *)entry, (void *)entry->name);

    return 1;
}

Upvotes: 1

Some programmer dude
Some programmer dude

Reputation: 409176

The problem is this:

entry = map->entries[map->guard++];

Here you copy the data from the array into the entry structure instance. Then you modify the data of entry and discard those modifications. The (original) structure data in the array is still unmodified.

That will of course lead to undefined behavior when you in the next call to map_put use the uninitialized structures in the array.

Either modify the array structure instance directly and increase map->guard separately. Or make entry a pointer and make it point to the array element.

Upvotes: 3

Related Questions