cmallocfree

Trying to understand a `pointer being freed was not allocated` error in C


So the error is self explanatory, but I don't understand how I'm getting it. I made the malloc and now the free complains.

I'm trying to build a map/hashtable for myself. I'm newish to C but not programming so I figured it would be a nice start. I'm also using SDL so that why I'm using those functions rather than straight malloc/free. But that shouldn't change anything...

So I can create the base map and I can add a key/value to it. I store the free function along with it so the map knows how to free its own items. The 1st set works. When I set again with the same key, it should free the current value and store the new one. But this is where the error happens.

Where are all my mistakes / what am I doing wrong?

Output

DEBUG: test/src/util/map.c:53:Map_Create(): Map created 0 of 10 used.
DEBUG: test/src/util/map.c:101:Map_Set(): Add item 3 of 10.
DEBUG: test/src/util/map.c:89:Map_Set(): Free item 3 of 10.
sdl_test(19543,0x1ffee1e00) malloc: *** error for object 0x10249bf8e: pointer being freed was not allocated

main.c

#include <SDL3/SDL.h>
#include <SDL3/SDL_main.h>
#include "util/map.h"

int main(int argc, char *argv[]) {

  char *itemA = SDL_malloc(sizeof(char) * 12);
  char *itemB = SDL_malloc(sizeof(char) * 11);
  itemA = "Hello World";
  itemB = "What's Up?";

  Map map = Map_Create(10);

  Map_Set(map, "test", itemA, SDL_free);
  Map_Set(map, "test", itemB, SDL_free);

  Map_Destroy(map);

  SDL_Quit();
}

map.c

#include <SDL3/SDL.h>
#include <SDL3/SDL_assert.h>
#include "util/map.h"
#include "debug.h"

struct Map_Item {
  struct Map_Item *next;
  void (*free)(void*);
  void *value;
  char *key;
} Map_Item;

struct Map {
  struct Map_Item **items;
  int capacity;
  int size;
};

/**
 * Internal functions
 */

static unsigned int _Map_Hash(const char *key) {
  unsigned int hash = -1;
  while (*key) {
    hash *= 31;
    hash ^= (unsigned char) *key;
    key += 1;
  }
  return hash;
}

/**
 * Public functions
 */

Map Map_Create(int capacity) {
  Map map = SDL_malloc(sizeof (Map));
  SDL_assert(map != NULL);

  // Create space for the initial items
  map->items = SDL_calloc(capacity, sizeof (struct Map_Item *));
  SDL_assert(map->items != NULL);

  map->capacity = capacity;
  map->size = 0;

  // Clear each location as there is currently nothing in any of them
  for (int i = 0; i < map->capacity; i += 1) {
    map->items[i] = NULL;
  }

  DEBUG_PRINT("Map created %d of %d used.\n", map->size, map->capacity);

  return map;
}

void Map_Destroy(Map map) {
  DEBUG_PRINT("Map cleanup %d of %d used.\n", map->size, map->capacity);

  // Loop over each item and free it;
  for (int i = 0; i < map->capacity; i += 1) {
    struct Map_Item *curr = map->items[i];
    while (curr != NULL) {
      DEBUG_PRINT("Free item %d of %d.\n", i, map->capacity);
      struct Map_Item *next = curr->next;
      if (curr->free != NULL) {
        curr->free(curr->value);
      }
      SDL_free(curr->key);
      SDL_free(curr);
      curr = next;
    }
  }

  SDL_free(map->items);
  SDL_free(map);
}


void Map_Set(Map map, const char *key, void *value, void (*free)(void*)) {
  int b = _Map_Hash(key) % map->capacity;

  // Look if the key is in use, if it is then free the old value and store the
  // new one in its place.
  for (struct Map_Item *curr = map->items[b]; curr != NULL; curr = curr->next) {
    if (SDL_strcmp(curr->key, key) == 0) {
      if (curr->free != NULL) {
        DEBUG_PRINT("Free item %d of %d.\n", b, map->capacity);
        curr->free(curr->value);
      }
      DEBUG_PRINT("Replace item %d of %d.\n", b, map->capacity);
      curr->value = value;
      curr->free = free;
      return;
    }
  }

  // No existing key was found, so insert it as a new entry at the head of the
  // list.
  DEBUG_PRINT("Add item %d of %d.\n", b, map->capacity);
  struct Map_Item *new = SDL_malloc(sizeof (struct Map_Item));
  new->key = SDL_malloc(sizeof(char) * SDL_strlen(key) + 1);
  new->next = map->items[b];
  new->value = value;
  new->free = free;
  SDL_strlcpy(new->key, key, SDL_strlen(key) + 1);
  map->items[b] = new;
  map->size += 1;
}

Solution

  • Here ...

      char *itemA = SDL_malloc(sizeof(char) * 12);
      char *itemB = SDL_malloc(sizeof(char) * 11);
    

    ... you allocate memory for two arrays of char, and store pointers to those allocated spaces in itemA and itemB.

    Here ...

      itemA = "Hello World";
      itemB = "What's Up?";
    

    ... you replace the pointers to the allocated spaces with pointers to string literals (leaking the allocated memory). Those indeed are not dynamically allocated. When you try to free one of them, the system is perfectly justified in complaining.

    Possibly you want to copy data into the allocated blocks instead of pointing the items at different objects. That would be:

      strcpy(itemA, "Hello World");
      strcpy(itemB, "What's Up?");