Invalid read in hash table

Question

I'm using uthash.h in order to store my application's configuration. As the config comes from a file that is read at runtime, the keys and values within the hash are both dynamically allocated char *'s:

typedef struct config_entry {
    char *name;
    char *value;
    UT_hash_handle hh;
} CONFIG_ENTRY;

As explained in the user guide, I implemented my own function to add keys to the config-hash that ensures uniqueness. Here it is:

void cfg_put( char *name, char *value, FREE_FLAGS flags ) {

    CONFIG_ENTRY *entry;

    //first, check if the key is already in the hash
    HASH_FIND_STR( config_, name, entry );
    if( entry == NULL ) {
        //key doesn't exist yet => create new one
        entry = (CONFIG_ENTRY *)malloc( sizeof( CONFIG_ENTRY ) );
        entry->name = name;
        HASH_ADD_KEYPTR( hh, config_, entry->name, strlen(entry->name), entry );
    } else {
        //key exists => possibly free existing pointers before setting value

        if( (flags & FREE_NAME) == FREE_NAME ) {        //
            free( entry->name );                        // these lines seem to be
        }                                               // problematic.
        entry->name = name;                             //

        if( (flags & FREE_VALUE) == FREE_VALUE ) {
            free( entry->value );
        }
    }

    //Finally, set the value
    entry->value = value;
}

I also wrote up some testcases for checking my implementation, and they seem to run just fine. However, if I run the tests using valgrind to check for memleaks, I always get the following:

==2561== Invalid read of size 1
==2561==    at 0x4026097: bcmp (mc_replace_strmem.c:541)
==2561==    by 0x804ADF5: cfg_get (in /home/gj/..../test/config_test)
==2561==    by 0x804B2C7: test_config1 (in /home/..../test/config_test)
==2561==    by 0x402E446: run_single_test (in /usr/local/lib/libcunit.so.1.0.1)
[...]
==2561==  Address 0x4194210 is 0 bytes inside a block of size 4 free'd
==2561==    at 0x4023B6A: free (vg_replace_malloc.c:366)
==2561==    by 0x804A872: cfg_put (in /home/..../test/config_test)
==2561==    by 0x804B27D: test_config1 (in /home/..../test/config_test)
==2561==    by 0x402E446: run_single_test (in /usr/local/lib/libcunit.so.1.0.1)
[...]

Here's the test case and the implementation of cfg_get for completeness:

void test_config1( void ) {

    cfg_clear( FREE_ALL );

    cfg_put( strdup("foo"), "bar", FREE_NONE );
    CU_ASSERT_EQUAL( cfg_count(), 1 );
    CU_ASSERT_STRING_EQUAL( cfg_get("foo"), "bar" );

    cfg_dump();

    cfg_put( "foo", "baz", FREE_NAME );
    CU_ASSERT_EQUAL( cfg_count(), 2 );
    CU_ASSERT_STRING_EQUAL( cfg_get("foo"), "baz" );

    cfg_clear( FREE_NONE );

    cfg_dump();
}

cfg_get:

char *cfg_get( const char *name ) {

    CONFIG_ENTRY *entry = NULL;
    HASH_FIND_STR( config_, name, entry );

    if( entry ) {
        return entry->value;
    } else {
        return NULL;
    }
}

Somehow, it seems I'm accessing the old name-pointer in cfg_get after I've overwritten it in cfg_put. The problem only occurs for the name, not for the value. I'm too stupid to figure it out, thx for any advice.

Answer 1

You'll have to provide the complete program — that is, a complete minimal example that reproduces the valgrind issue. The code you've posted in your question looks fine, so the bug must be hiding somewhere else; e.g. in the code of cfg_clear() or cfg_count().

(Originally I thought that cfg_count() must be return HASH_COUNT(config_); — but that implementation wouldn't pass your test case, so you must be doing something weirder. Which means cfg_count is probably the wrong name for that function anyway.)

---

Stylistically, you might find your code easier to debug if you avoided the use of global variables (config_), and definitely you'd find it easier if you stored the "necessity to free this value" bits directly alongside the "value" bits, instead of requiring the user to keep track of FREE_NAME, FREE_VALUE, etc., on his own. That is, instead of

typedef struct config_entry {
    char *name;
    char *value;
    UT_hash_handle hh;
} CONFIG_ENTRY;

void cfg_put(char *name, char *value, FREE_FLAGS flags);
void cfg_clear(FREE_FLAGS flags);

you should provide merely

typedef struct config_entry {
    char *name;
    char *value;
    UT_hash_handle hh;
    bool must_free_name;
    bool must_free_value;
} CONFIG_ENTRY;

void cfg_put(char *name, char *value, FREE_FLAGS flags);
void cfg_clear(void);

at which point your test case becomes the more manageable

void test_config1()
{
    cfg_clear();  // use the stored bits to figure out what needs freeing
    cfg_put(strdup("foo"), "bar", FREE_NAME);  // name is alloc'ed, so name must be freed later
    CU_ASSERT_EQUAL( cfg_count(), 1 );
    CU_ASSERT_STRING_EQUAL( cfg_get("foo"), "bar" );

    cfg_put("foo", "baz", FREE_NONE);  // neither name nor value is alloc'ed
    CU_ASSERT_EQUAL( cfg_count(), 2 );
    CU_ASSERT_STRING_EQUAL( cfg_get("foo"), "baz" );
}

Answer 2

There is a problem in your config_put() function: it modifies the key of an item already inserted in to the hash. You are not supposed to do this. It may be OK to change the name pointer to one that points to the same string, but it may be not, the implementation of uthash.h is a bit obscure.

I suggest you change the API so config_put() does all the string management, letting the config_ hash own all the strings, and no longer call strdup() in test_config1. This is much simpler and avoids the potentially complicated and error prone tracking of the life cycle of string values outside of the hash structure:

void cfg_put(const char *name, const char *value) {

    CONFIG_ENTRY *entry;

    //first, check if the key is already in the hash
    HASH_FIND_STR(config_, name, entry);
    if (entry == NULL) {
        //key doesn't exist yet => create new one
        entry = malloc(sizeof(*entry));
        entry->name = strdup(name);
        HASH_ADD_KEYPTR(hh, config_, entry->name, strlen(entry->name), entry );
    } else {
        //key exists => free existing value pointer if any
        free(entry->value);
    }

    //Finally, set the value
    entry->value = value ? strdup(value) : NULL;
}