0

I have this piece of code here:

    assert_ptr_equals(get_data(hm,key_three),NULL);
    assert_true((int*)get_data(hm,key_three)==NULL);

The get_data function returns a void pointer. The first assert is true but second one fails. Any idea why?

Here is the minimum reproducible code

#include<stdlib.h>
#include<stddef.h>
#include <string.h>
#include <stdlib.h>
#include <stdio.h>
#include <sys/wait.h>
#include <sys/types.h> 
#include <unistd.h> 

#define ASSERTION_FAILURE_EXITCODE 47

#define assert_true(x) assert_that(x)
#define assert_ptr_equals(x, y) assert_ptr_equals_internal(__FILE__, __extension__ __FUNCTION__, __LINE__, #x, x, #y, y)
#define assert_that(x) assert_that_internal(__FILE__, __extension__ __FUNCTION__, __LINE__, #x, x)

void assert_ptr_equals_internal(const char *file_name, const char *function_name, int line_number, const char *xname, void *x, const char *yname, void *y) {
    if (x != y) {
        fprintf(stderr, "%s:%s:%d: Expected <%s>:%p to be equal to <%s>:%p.\n", file_name, function_name, line_number, xname, x, yname, y);
        exit(ASSERTION_FAILURE_EXITCODE);
    }
}

void assert_that_internal(const char *file_name, const char *function_name, int line_number, const char *condition_name, int condition) {
    if (!condition) {
        fprintf(stderr, "%s:%s:%d: Expected '%s' to hold.\n", file_name, function_name, line_number, condition_name);
        exit(ASSERTION_FAILURE_EXITCODE);
    }
}




typedef void * Data;
typedef Data (* ResolveCollisionCallback)(Data oldData, Data newData);
typedef void (* CallbackIterate)(char * key, Data data);
typedef void (* DestroyDataCallback)(Data data);

typedef struct {
    Data * data;                //Data pointer to the data
    char * key;                 //char pointer to the string key
} HashMapItem;

typedef struct hashmap {
    HashMapItem ** items;       //items of the hashmaps
    size_t size;                //size of the hashmaps
    int count;                  //how many elements are in the hashmap
} HashMap;

unsigned int hash(const char * key){
    //sum of the charaters
    unsigned int sum = 0;
      
    for(int i=0; key[i] != '\0'; i++){
        sum += key[i];
    }

    return sum;
}

HashMap * create_hashmap(size_t key_space){
    if(key_space == 0)
        return NULL;
    
    HashMap * hm = (HashMap *) malloc(sizeof(HashMap));                         //allocate memory to store hashmap
    hm->items = (HashMapItem **) calloc(key_space, sizeof(HashMapItem**));      //allocate memory to store every item inside the map, null it
    hm->size = key_space;                                                       //set sitze of hashmap
    hm->count = 0;                                                              //empty at the begining


    return hm;
}

void insert_data(HashMap* hm, const char * key, const Data data, const ResolveCollisionCallback resolve_collison){
    if(key == NULL || hm == NULL || data == NULL){
        return;
    }
 
    //index where to put data
    unsigned int index = hash(key) % hm->size;
  
    if((hm->items)[index] != NULL){
           
        (hm->items)[index]->data = (Data *)malloc(sizeof(Data *));                          //allocate memory to store the address
        
        if(resolve_collison!=NULL)
            *(hm->items)[index]->data = resolve_collison((hm->items)[index]->data, data);   //copy address of data into the hashmap
        else
            *(hm->items)[index]->data = data;                                               //if there is no resolve collision and there is data stored
                                                                                            //store the data pointer as is
    }
    else {
        (hm->items)[index] = (HashMapItem *)malloc(sizeof(HashMapItem *));
        (hm->items)[index]->data = (Data *)malloc(sizeof(Data *));  
        *(hm->items)[index]->data = data; 
    }

    //allocate new space for the string key and copy it there
    (hm->items)[index]->key = strdup(key);
}   

Data get_data(HashMap* hm, char* key){
    if(key == NULL && hm == NULL)
        return NULL;
    

    unsigned int index = hash(key) % hm->size;
    if((hm->items)[index] == NULL){
        return NULL;
    }    
    return *(hm->items)[index]->data;
}

void remove_data(HashMap* hm, char * key, DestroyDataCallback destroy_data){
    if(hm == NULL || key == NULL)
        return;

    unsigned int index = hash(key) % hm->size;
   
    if((hm->items)[index] == NULL)
        return;
   
    if(destroy_data != NULL){
        destroy_data((Data) *(hm->items)[index]->data);
    }
    
    free((hm->items)[index]->data);
    free((hm->items)[index]->key);
    free((hm->items)[index]);
}


void test1() {
      HashMap *hm = create_hashmap(30);
    
    char *key ="jsart", *key_two = ":@-)", *key_three = "stonks";
    int x = 69, y = 420;
    void *placeholder = &x, *placeholder_two = &y;
    
    insert_data(hm, key_three, placeholder, NULL);
    assert_that(get_data(hm,key_three) == placeholder);
    remove_data(hm,key_three,NULL);

    assert_ptr_equals(get_data(hm,key_three),NULL);
    assert_true((int*)get_data(hm,key_three)==NULL);

    //delete_hashmap(hm, destroy);
}

int main(){
    test1();
    return 0;
}

Cristian Cutitei
  • 120
  • 3
  • 12
  • Comments are not for extended discussion; this conversation has been [moved to chat](https://chat.stackoverflow.com/rooms/250133/discussion-on-question-by-cristian-cutitei-what-happens-if-i-cast-a-pointer-that). – sideshowbarker Dec 04 '22 at 09:32

1 Answers1

3

At least these problems:

Wrong allocation leading to UB

(hm->items)[index]->data = (Data *)malloc(sizeof(Data *));      

I suspect OP wanted

(hm->items)[index]->data = (Data *)malloc(sizeof(Data));

Avoid size mistakes, allocate to the referenced object. Cast not needed.

(hm->items)[index]->data = malloc(sizeof (hm->items)[index]->data[0]);

Also in

    (hm->items)[index] = (HashMapItem *)malloc(sizeof(HashMapItem *));
    (hm->items)[index]->data = (Data *)malloc(sizeof(Data *));

 hm->items = (HashMapItem **) calloc(key_space, sizeof(HashMapItem**)); 

Dangling Pointers

@n. m. "there is an error in remove_data that leaves (hm->items)[index] dangling after free"

chux - Reinstate Monica
  • 143,097
  • 13
  • 135
  • 256
  • 1
    Maybe there should be a standard macro, `#define AllocateNItems(p, N) ((p) = malloc(N * sizeof *(p)))`. – Eric Postpischil Dec 04 '22 at 00:23
  • 1
    There are quite a few errors of this sort in this program. Then there is an error in `remove_data` that leaves `(hm->items)[index]` dangling after `free` (should reset to NULL). – n. m. could be an AI Dec 04 '22 at 00:27
  • @n.m. doesn't free() already set it to NULL? – Cristian Cutitei Dec 04 '22 at 00:31
  • @EricPostpischil that is indeed a huge problem which i have not addressed. You and chux were the best at helping me. I m new to C so Ill keep practicing. – Cristian Cutitei Dec 04 '22 at 00:33
  • 2
    @CristianCutitei It doesn't. It cannot. Even if it wanted to, this is physically impossible. *C has pass by value regime*. A function cannot change its argument. – n. m. could be an AI Dec 04 '22 at 00:36
  • 1
    @EricPostpischil Agree'd. Perhasp even better as `#define AllocateNItems(p, N) ((p) = malloc(sizeof *(p) * N))` or `#define AllocateNItems(p, N) ((p) = malloc((size_t)N) * sizeof *(p)))` to help encourage the multiplication is done with at least `size_t` math as with `AllocateNItems(p, 0x10000 * 0x10000)`. – chux - Reinstate Monica Dec 04 '22 at 00:40
  • @chux-ReinstateMonica: `sizeof *(p)` automatically gives `size_t`. But I should have written `(N)` instead of `N`, as standard macro practice. And then you cannot drive `(size_t)` into the expression given for `N`; that is up to the user. – Eric Postpischil Dec 04 '22 at 01:03
  • @EricPostpischil It is the case of preferred `sizeof *p * some_int * some_int` vs `some_int * some_int * sizeof *p` that I am trying to achieve. IAC, the `p = malloc(sizeof *p * N)` is a good idiom. – chux - Reinstate Monica Dec 04 '22 at 02:48
  • @chux-ReinstateMonica: `N` could be `3+4`. It needs to be parenthesized. – Eric Postpischil Dec 04 '22 at 02:51
  • @EricPostpischil Good point. Yet perhaps `((size_t)1*N)`? – chux - Reinstate Monica Dec 04 '22 at 02:56