Take the 2-minute tour ×
Game Development Stack Exchange is a question and answer site for professional and independent game developers. It's 100% free, no registration required.

I have been learning to code in C from this amazing resource http://c.learncodethehardway.org/book/

I am on exercise 17. Basically creating your own simple database using Malloc. I modified the exercise to come up with my own arbitrary size database. So far I have been on the right track. But I have had to deal with this segmentation fault nightmare and I don't know where and why its happening in my code.

If some of you C geniuses could kindly look into my code and point out what I am doing wrong, it would be greatly appreciated!

Particularly in the function Database_Set and Database_ExpandSize. Inside Database_Set, I keep track of rows count to see if it exceeds maxrows and call Database_Expand to reallocate a new memory space of bigger size. Before this I shift everything from old address to a temporary address using memcpy.

Please look at my code below. The code is running fine till I set 8 rows but when I try to set a 9th row in my database. The size increases from 8 to 16 but and causes a "segmentation fault". I am definitely doing something absurd in Database_Expand but I am don't know what. You can also try running the entire code in your C compiler. I am running it on my Mac Terminal by using Make.

Also after running the code through valgrind, showed me some memory leaks loss of some 16k bytes in Expand_Size. How do I correctly free the old memory space while reallocating?

#include <stdio.h>
#include <assert.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>

#define MAX_DATA 512
#define MAX_ROWS 2

struct Address
{
    int pid;
    int set;
    char name[MAX_DATA];
    char email[MAX_DATA];
};

struct Database
{
    //for arbitrary size database, we need to allocate it on the Heap'
    int maxrows;
    int rowCount;
    struct Address* addressRows;
};

struct Connection
{
    struct Database* db;
    FILE* file;
};

void Database_Load(struct Connection* conn)
{
    int rc = fread(conn->db,sizeof(struct Database),1,conn->file);
    if(rc!=1)printf(" could not load the database ");
}

// This is the first step. You first open the database by allocating space for connection and database structures
// Next you check for the mode type and open the file assigining it to connection
struct Connection* Database_Open(const char* filename)
{
    struct Connection* conn = malloc(sizeof(struct Connection));
    if(!conn)printf(" memory error ");

    conn->db = malloc(sizeof(struct Database));
    if(conn->db == NULL)printf(" mem error ");

    //setting up arbitrary size database
    conn->db->maxrows = MAX_ROWS;
    conn->db->rowCount = 0;
    conn->db->addressRows = malloc(sizeof(struct Address) * conn->db->maxrows);
    conn->file = fopen(filename,"w");
    /*else
    {
        conn->file = fopen(filename,"r+");

        if(conn->file)
            printf(" loading the database.. ");
            Database_Load(conn);
    }*/
    if(!conn->file)
    {
        printf(" failed to open the file ");
        return NULL;
    }
    return conn;
}

// Initialize all the pids of address structures in the database
void Database_Create(struct Connection* conn)
{
    int i = 0;
    for(i = 0; i < conn->db->maxrows; i++)
    {
        struct Address* addr = conn->db->addressRows + ( sizeof(struct Address) * i );
        addr->set = 0;
        addr->pid = i;
    }
}

int Database_ExpandSize(struct Database* db)
{
    /*
     allocate a temp address row stuct
     */
    struct Address* tempAddrRows = malloc(sizeof(struct Address) * db->maxrows);
    struct Address* startAddress = tempAddrRows;  //to bring back tempAddressRows back to the start, keep a track of its starting address
    struct Address* incrementalAddress = tempAddrRows;
    struct Address* dbAddrRowTemp = NULL;

    /*
     copy everything from original addressRows to temporary incremental address which points to tempAddressRows
     */
    int i = 0;
    for(i = 0; i < db->maxrows; i++)
    {
        dbAddrRowTemp = db->addressRows + (sizeof(struct Address) * i);
        memcpy(incrementalAddress,dbAddrRowTemp,sizeof(struct Address));
        incrementalAddress += sizeof(struct Address);
    }

    //sanity check, all looks fine
    for(i = 0; i < db->maxrows; i++)
    {
        struct Address* check = tempAddrRows + (sizeof(struct Address) * i);
        printf(" \n for temp addr row, pid, set, name, email %d %d %s %s \n ",check->pid,check->set,check->name,check->email);
    }

    //reallocate temp address to double size and bring it back to start address
    tempAddrRows = realloc(tempAddrRows,db->maxrows * 2 * sizeof(struct Address)); //realloc frees the previous memory block ie previous allocations of tempAddrRows is freed!
    tempAddrRows = startAddress;

    //make the max rows double the size and assigne dbAddressRows to point to newly allocated tempAddressRows
    db->maxrows *= 2;
    db->addressRows = tempAddrRows;
    return 1;
}

void Database_Write(struct Connection* conn)
{
    rewind(conn->file);
    int rc = fwrite(conn->db,sizeof(struct Database),1,conn->file);
    if(rc!=1)printf(" memory error in database write ");
    rc = fflush(conn->file);
    if(rc == -1)printf(" could not be flushed ");
}

void Database_Set(struct Connection* conn,int id,char* name,char* email)
{
    conn->db->rowCount++;
    if(conn->db->rowCount > conn->db->maxrows)
    {
        printf(" row increased max row size, expanding the array from %d to %d \n ",conn->db->maxrows,conn->db->maxrows*2);
        if(Database_ExpandSize(conn->db) == 0)
        {
            printf(" failed to resize the array ");
            return;
        }
    }

    struct Address* addr = conn->db->addressRows + ( sizeof(struct Address) * id );

    if(!addr)
    {
        printf(" addr is null in Set ");
        return;
    }

    if(addr->set == 1)
    {
        printf(" already set the id \n ");
        return;
    }

    addr->set = 1;
    addr->pid = id;

    printf(" setting for %s with address %p \n",name,(void*)addr);

    char* res = strncpy(addr->name,name,MAX_DATA);
    if(!res)printf(" could not set the name ");


    res = strncpy(addr->email,email,MAX_DATA);
    if(!res)printf(" could not set the email ");
}

void Database_Get(struct Connection* conn,int id)
{
    struct Address* addr = conn->db->addressRows + (sizeof(struct Address) * id );

    if(!addr)
    {
        printf(" addr is null in Get ");
        return;
    }
    printf(" for id %d ",id);
    printf(" \n name %s",addr->name);
    printf(" \n email %s",addr->email);
}

void Database_Delete(struct Connection* conn,int id)
{
    struct Address* addr = conn->db->addressRows + (sizeof(struct Address) * id );

    if(!addr)
    {
        printf(" addr is null in Get ");
        return;
    }
    addr->set = 0;
}

void Database_List(struct Connection* conn)
{
    int i =0;
    for(i = 0; i < conn->db->maxrows; i++)
    {
        struct Address* addr = conn->db->addressRows + (sizeof(struct Address) * i );
        printf(" \n inside list address for id %d -> %p, setId %d \n ",i,(void*)addr,addr->set);
        if(addr->set == 1)
        {
            printf(" id %d \n",addr->pid);
            printf(" name %s \n",addr->name);
            printf(" email %s \n",addr->email);
        }
    }
}

void Database_Close(struct Connection* conn)
{
    if(conn)
    {
        if(conn->file)fclose(conn->file);
        if(conn->db)free(conn->db);
        free(conn);
    }
}

int main(int argc, char *argv[])
{
    if(argc < 3) printf(" enter correct parameters in form ./HeapDatabase db.dat c ");

    char* filename = argv[1];
    struct Connection* conn = Database_Open(filename);
    Database_Create(conn);
    Database_Write(conn);

    Database_Set(conn,0,"j","b");
    Database_Set(conn,1,"p","k");
    Database_Set(conn,2,"fd","ewew");
    Database_Set(conn,3,"ew","as");
    Database_Set(conn,4,"ewq","ewe");
    Database_Set(conn,5,"bn","we");
    Database_Set(conn,6,"de","de");
    Database_Set(conn,7,"m","a");
    //Database_Set(conn,8,"wee","qweqwe");
    Database_List(conn);
    Database_Close(conn);
    return 0;
}

Thank you!

share|improve this question

closed as off-topic by congusbongus, Byte56 Jul 6 at 15:08

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Programming questions that aren't specific to game development are off-topic here, but can be asked on Stack Overflow. A good rule of thumb is to ask yourself "would a professional game developer give me a better/different/more specific answer to this question than other programmers?"" – congusbongus, Byte56
If this question can be reworded to fit the rules in the help center, please edit the question.

1 Answer 1

In your Database_ExpandSize() function:

struct Address* startAddress = tempAddrRows;
...
//reallocate temp address to double size and bring it back to start address
tempAddrRows = realloc(tempAddrRows,db->maxrows * 2 * sizeof(struct Address)); 
tempAddrRows = startAddress;

You first reallocate your data with twice the size but then you overwrite the pointer it with the old (now invalid, because realloc frees the previous memory block ie previous allocations of tempAddrRows is freed!) pointer. This leaks the newly allocated block (as there is no pointer to it) and makes tempAddrRows a dangling pointer. Any access to the memory pointed to by it results in undefined behaviour.

To fix both issues remove the assignment tempAddrRows = startAddress;.

You should also check whether realloc returns NULL to handle any error during memory allocation. Note that when realloc indeed does return NULL the original memory block is not freed.

share|improve this answer
    
Thanks for the feedback. That doesn't solve the segmentation fault error. –  ckzilla Jul 8 at 1:43

Not the answer you're looking for? Browse other questions tagged or ask your own question.