Program crashing using malloc() and free()

Question

I appear to be crashing my program when trying to free memory sections. The following is the overall structure of my linked lists:

typedef struct {
    char                            *dataitem;
    struct listelement              *link;
    int16_t                         wordSize;
    int16_t                         (*libWord)[Q];
    char                            gpioValue;
    struct listelement              *syllables;
}listelement;

The program crashes when calling this function:

recordedWordsPointer = RemoveItem(recordedWordsPointer);                            // get rid of any junk stored in the recorded buffer

Where:

volatile listelement *recordedWordsPointer;

has stored values in libWord and points to next link if there is another, otherwise NULL. The following shows what happens when entering the function:

listelement * RemoveItem (listelement * listpointer) {
    cpu_irq_disable();
    listelement * tempp = listpointer;

    while( listpointer->syllables != NULL ){
        RemoveSyllable(listpointer->syllables);
    }
    if( listpointer != NULL ){
        tempp = listpointer -> link;
        free (listpointer->dataitem);
        free (listpointer->libWord);
        free (listpointer);
    }
    cpu_irq_enable();   
    return tempp;
}

void RemoveSyllable (listelement * listpointer) {

    while( listpointer->syllables != NULL ){
        RemoveSyllable(listpointer->syllables);
    }
    free (listpointer->dataitem);
    free (listpointer->libWord);
    free (listpointer);
    listpointer = NULL;
    return;
}

I was wondering if I am doing something wrong to cause a memory crash?

Thanks!

EDIT:

I was asked to show how I construct the memory locations to help. I use the following two functions:

listelement * AddItem (listelement * listpointer, char* name, int16_t size, int16_t wordLength, int16_t (*words)[Q]) {
    // returns listPointer at the beginning of list
    listelement * lp = listpointer;
    listelement * listPointerTemp;
    char ErrorHandler = NULL;
    // are we at the end of the list?
    if (listpointer != NULL) {
        // move down to the end of the list
        while (listpointer -> link != NULL)
        listpointer = listpointer -> link;
        listPointerTemp = listpointer;
        listpointer -> link = (struct listelement  *) malloc (sizeof (listelement));
        // on fail end links becomes NULL already above
        if(listpointer -> link != NULL){
            listpointer = listpointer -> link;
            listpointer -> link = NULL;
            listpointer -> wordSize = wordLength;
            listpointer -> syllables = NULL;

            listpointer -> dataitem = (char*) malloc ((size + 1)*sizeof(char));
            if(listpointer -> dataitem != NULL){
                for(int i=0; i<size ; i++){
                    listpointer -> dataitem[i] = name[i];
                }
                listpointer -> dataitem[size] = NULL;

                listpointer -> libWord =  (int16_t(*)[Q])malloc(wordLength*Q*sizeof(int16_t));
                if(listpointer -> libWord != NULL){
                    for (int16_t row=0 ; row < wordLength ; row++){
                        for (int col=0 ; col < Q ; col++){
                            listpointer -> libWord[row][col]  = words[row][col];
                        }
                    }
                    ErrorHandler = 1;
                }else{
                    free(listpointer->dataitem);
                    free(listpointer);
                    listPointerTemp -> link = NULL;
                }
            }else{
                free(listpointer);
                listPointerTemp -> link = NULL;
            }
        }
        if(ErrorHandler == NULL){
            //failure
            usart_write_line(&AVR32_USART0,"\r\n--------------------------------------------\r\n");
            usart_write_line(&AVR32_USART0,"Ran out of Memory!  Word not created.\r\n");
            usart_write_line(&AVR32_USART0,"\r\n--------------------------------------------\r\n");
        }
        return lp;
    } else {
        listpointer = (struct listelement  *) malloc (sizeof (listelement));

        if(listpointer != NULL){
            listpointer -> link = NULL;
            listpointer -> wordSize = wordLength;
            listpointer -> syllables = NULL;

            listpointer -> dataitem = (char*) malloc ((size + 1)*sizeof(char));
            if(listpointer -> dataitem != NULL){
                for(int16_t i=0; i<size ; i++){
                    listpointer -> dataitem[i] = name[i];
                }
                listpointer -> dataitem[size] = NULL;

                listpointer -> libWord =  (int16_t(*)[Q])malloc(wordLength*Q*sizeof(int16_t));
                if(listpointer -> libWord != NULL){
                    for (int16_t row=0 ; row < wordLength ; row++){
                        for (int col=0 ; col < Q ; col++){
                            listpointer -> libWord[row][col]  = words[row][col];
                        }
                    }
                    ErrorHandler = 1;
                }else{
                    free(listpointer->dataitem);
                    free(listpointer);
                    listPointerTemp -> link = NULL;
                }
            }else{
                free(listpointer);
                listPointerTemp -> link = NULL;
            }
        }
        if(ErrorHandler == NULL){
            //failure
            usart_write_line(&AVR32_USART0,"\r\n--------------------------------------------\r\n");
            usart_write_line(&AVR32_USART0,"Ran out of Memory!  Word not created.\r\n");
            usart_write_line(&AVR32_USART0,"\r\n--------------------------------------------\r\n");
        }
        return listpointer;
    }
}

listelement* AddSyllable (listelement * listpointer, char* name, int16_t size, int16_t wordLength, int16_t (*words)[Q]) {
    // returns listPointer at the beginning of list
    listelement * lp = listpointer;
    listelement * listPointerTemp;
    char ErrorHandler = NULL;
    // are we at the end of the list?
    if (listpointer != NULL) {
        // move down to the end of the list
        while (listpointer -> syllables != NULL)
        listpointer = listpointer -> syllables;
        listPointerTemp = listpointer;
        listpointer -> syllables = (struct listelement  *) malloc (sizeof (listelement));
        // on fail end links becomes NULL already above
        if(listpointer -> syllables != NULL){
            listpointer = listpointer -> syllables;
            listpointer -> link = NULL;
            listpointer -> wordSize = wordLength;

            listpointer -> dataitem = (char*) malloc ((size + 1)*sizeof(char));
            if(listpointer -> dataitem != NULL){
                for(int i=0; i<size ; i++){
                    listpointer -> dataitem[i] = name[i];
                }
                listpointer -> dataitem[size] = NULL;

                listpointer -> libWord =  (int16_t(*)[Q])malloc(wordLength*Q*sizeof(int16_t));
                if(listpointer -> libWord != NULL){
                    for (int16_t row=0 ; row < wordLength ; row++){
                        for (int col=0 ; col < Q ; col++){
                            listpointer -> libWord[row][col]  = words[row][col];
                        }
                    }
                    ErrorHandler = 1;
                }else{
                    free(listpointer->dataitem);
                    free(listpointer);
                    listPointerTemp -> syllables = NULL;
                }
            }else{
                free(listpointer);
                listPointerTemp -> syllables = NULL;
            }
        }
        if(ErrorHandler == NULL){
            //failure
            usart_write_line(&AVR32_USART0,"\r\n--------------------------------------------\r\n");
            usart_write_line(&AVR32_USART0,"Ran out of Memory!  Word not created.\r\n");
            usart_write_line(&AVR32_USART0,"\r\n--------------------------------------------\r\n");
        }
        return lp;
    } else {
        listpointer = (struct listelement  *) malloc (sizeof (listelement));

        if(listpointer != NULL){
            listpointer -> link = NULL;
            listpointer -> wordSize = wordLength;

            listpointer -> dataitem = (char*) malloc ((size + 1)*sizeof(char));
            if(listpointer -> dataitem != NULL){
                for(int16_t i=0; i<size ; i++){
                    listpointer -> dataitem[i] = name[i];
                }
                listpointer -> dataitem[size] = NULL;

                listpointer -> libWord =  (int16_t(*)[Q])malloc(wordLength*Q*sizeof(int16_t));
                if(listpointer -> libWord != NULL){
                    for (int16_t row=0 ; row < wordLength ; row++){
                        for (int col=0 ; col < Q ; col++){
                            listpointer -> libWord[row][col]  = words[row][col];
                        }
                    }
                    ErrorHandler = 1;
                }else{
                    free(listpointer->dataitem);
                    free(listpointer);
                    listPointerTemp -> syllables = NULL;
                }
            }else{
                free(listpointer);
                listPointerTemp -> syllables = NULL;
            }
        }
        if(ErrorHandler == NULL){
            //failure
            usart_write_line(&AVR32_USART0,"\r\n--------------------------------------------\r\n");
            usart_write_line(&AVR32_USART0,"Ran out of Memory!  Word not created.\r\n");
            usart_write_line(&AVR32_USART0,"\r\n--------------------------------------------\r\n");
        }
        return listpointer;
    }
}

Why is the variable marked volatile? Yes, you must be doing something wrong. You've not shown how the memory is allocated, so we can't tell how you're freeing it wrong. The pointer to an array of Q int16_t looks suspicious; what is the value of Q, and why is it a good idea to make that a pointer to an array rather than simply an array? — Jonathan Leffler
– Jonathan Leffler, Commented Jul 21, 2013 at 18:24
The volatile is used because I have ISR's that use the same memory locations and don't want them to change those locations via registers to prevent data loss. I disable interrupts to deconstruct memory to prevent anything weird from happening (constructing onto half deconstructed linked list or anything else I cannot think of) — user2368363
– user2368363, Commented Jul 22, 2013 at 1:18
Are you running this code in the kernel? What is an ISR (AcronymFinder didn't help me)? I know of TSR (terminate-and-stay-resident), but I thought that was a 1990s technology. Where are you running this code? Any chance of you producing an SSCCE (Short, Self-Contained, Correct Example) so we can see exactly what you're doing? Without seeing a version of the code that's failing, we aren't going to be able to help much. Are you on Linux? If so, have you used valgrind? — Jonathan Leffler
– Jonathan Leffler, Commented Jul 22, 2013 at 1:25
Oh, I see you've added a ream of code for the allocation. Ouch! What environment are you actually running in? USART suggests some sort of embedded environment. The code you show is not an SSCCE. I expect people will look at it even so, but you need to work on an SSCCE. — Jonathan Leffler
– Jonathan Leffler, Commented Jul 22, 2013 at 1:29

Jongware · Accepted Answer · 2013-07-21 19:41:56Z

2

Your RemoveSyllable function doesn't actually set the syllables member to NULL. You think it does, inside the routine, but actually you are only changing its value inside the local variable.

answered Jul 21, 2013 at 19:41

Jongware

22.6k8 gold badges56 silver badges104 bronze badges

Sign up to request clarification or add additional context in comments.

1 Comment

user2368363 Over a year ago

That is a very good observation. Thanks! FYI-I don't think that the function goes into RemoveSyllable() when it crashes when using JTAG. Any additional thoughts?

Jonathan Leffler · Accepted Answer · 2013-07-22 03:23:35Z

Initial observations

This is more a partial deconstruction of your code than a full-on answer.

Do you not have your compiler warnings turned on? If not, why not?

typedef struct {
    char                            *dataitem;
    struct listelement              *link;
    int16_t                         wordSize;
    int16_t                         (*libWord)[Q];
    char                            gpioValue;
    struct listelement              *syllables;
} listelement

Note that the link and syllables members do not point at the anonymous struct that you typedef as listelement. They point to some other wholly unrelated structure. The compiler complains bitterly about this — with good reason. The fix is simple: change typedef struct { to typedef struct listelement {. But we should not have to deal with such a mess in your code.

You also sometimes misuse NULL. For example, char ErrorHandler = NULL; generates a warning that a pointer is being converted to an integer of a different size. NULL is not necessarily 0; it is at least sometimes ((void *)0) or something similar. You also use NULL in place of '\0' to set the end byte of a string to the null byte — that elicits a compiler warning too. These are noisy rather than dreadfully serious, but you should aim for code that compiles silently with a minimum of casting (that is, don't simply slap casts everywhere to shut up the compiler warnings).

You write:

    if(listpointer -> link != NULL){

Ignoring the placement of the brace (that's subject to legitimate debate), you should have a space after the if and you should not have any spaces around ->. Similar comments about spaces apply to while; it is also conventional to use no space between the function name and the open parenthesis in both calls and definitions.

This warning is serious:

usart.c:297:5: warning: passing argument 1 of ‘RemoveItem’ discards ‘volatile’ qualifier from pointer target type [enabled by default]
usart.c:253:14: note: expected ‘struct listelement *’ but argument is of type ‘volatile struct listelement *’

Basically, you remove the volatility of recordedWordsPointer when you call RemoveItem, which makes the presence of volatile superfluous. If it is volatile, you have to make sure that everywhere that uses it knows that. Or, more simply, remove the volatile qualifier.

There appears to be massive code duplication in each of AddItem() and AddSyllable(). You should aim to eliminate about half of each function.

Now I've got to work our how you might really use the functions so that I can see what goes wrong. That's difficult, not least because of the arguments to the AddItem() and AddSyllable() functions include a mysterious int16_t (*words)[Q]. It would help if you could show a simple main() program that invokes these functions with appropriate arguments (which should probably be initialized data structures rather than values read from file).

Refactored code

One key refactoring to reduce the volume of code is to create a function that makes a listelement given a name, size, number of words, and words. The dupstr() function below is not identical to strdup(); it takes a length and duplicates that many characters of a possibly longer string.

#include <stdlib.h>
#include <stdio.h>
#include <stdint.h>
#include <string.h>

enum { Q = 16 };

typedef struct listelement
{
    char                *dataitem;
    struct listelement  *link;
    int16_t              wordSize;
    int16_t            (*libWord)[Q];
    struct listelement  *syllables;
} listelement;

static int AVR32_USART0 = 0;

static void cpu_irq_disable(void);
static void cpu_irq_enable(void);   
static void usart_write_line(int *ptr, char *msg);
extern void RemoveSyllable(listelement *listpointer);
extern listelement *RemoveItem(listelement *listpointer);
extern listelement *AddSyllable(listelement *listpointer, char *name, int16_t size, int16_t wordLength, int16_t (*words)[Q]);
extern listelement *AddItem(listelement *listpointer, char *name, int16_t size, int16_t wordLength, int16_t (*words)[Q]);

/* Duplicate string - or use POSIX strdup() */
static char *dupstr(const char *name, int16_t size)
{
    char *str = malloc(size+1);
    if (str != NULL)
    {
        memmove(str, name, size);
        str[size] = '\0';
    }
    return str;
}

static listelement *makeElement(const char *name, int16_t size, int16_t wordLength, int16_t (*words)[Q])
{
    listelement *listpointer = (listelement *)malloc(sizeof(listelement));
    // on fail end links becomes NULL already above
    if (listpointer != NULL)
    {
        listpointer->dataitem = dupstr(name, size);
        listpointer->libWord =  (int16_t(*)[Q])malloc(wordLength*Q*sizeof(int16_t));
        if (listpointer->dataitem == NULL || listpointer->libWord == 0)
        {
            free(listpointer->dataitem);
            free(listpointer->libWord);
            free(listpointer);
            listpointer = NULL;
        }
        else
        {
            listpointer->link = NULL;
            listpointer->wordSize = wordLength;
            listpointer->syllables = NULL;
            for (int16_t row=0; row < wordLength; row++)
            {
                for (int col=0; col < Q; col++)
                {
                    listpointer->libWord[row][col] = words[row][col];
                }
            }
        }
    }
    return listpointer;
}

static void reportError(void)
{
    usart_write_line(&AVR32_USART0,"\r\n--------------------------------------------\r\n");
    usart_write_line(&AVR32_USART0,"Ran out of Memory!  Word not created.\r\n");
    usart_write_line(&AVR32_USART0,"\r\n--------------------------------------------\r\n");
}

listelement *AddItem(listelement *listpointer, char *name, int16_t size, int16_t wordLength, int16_t (*words)[Q])
{
    listelement *lp = listpointer;
    char ErrorHandler = 0;
    if (listpointer != NULL)
    {
        while (listpointer->link != NULL)
            listpointer = listpointer->link;
        listpointer->link = makeElement(name, size, wordLength, words);
        if (listpointer->link != NULL)
        {
            listpointer = listpointer->link;
            ErrorHandler = 1;
        }
    }
    else
    {
        listpointer = makeElement(name, size, wordLength, words);
        if (listpointer != NULL)
            ErrorHandler = 1;
        lp = listpointer;
    }
    if (ErrorHandler == 0)
        reportError();
    return lp;
}

listelement *AddSyllable(listelement *listpointer, char *name, int16_t size, int16_t wordLength, int16_t (*words)[Q])
{
    listelement *lp = listpointer;
    char ErrorHandler = 0;
    if (listpointer != NULL)
    {
        while (listpointer->syllables != NULL)
            listpointer = listpointer->syllables;
        listpointer->syllables = makeElement(name, size, wordLength, words);
        if (listpointer->syllables != NULL)
            ErrorHandler = 1;
    }
    else 
    {
        listpointer = makeElement(name, size, wordLength, words);
        if (listpointer != NULL)
            ErrorHandler = 1;
        lp = listpointer;
    }
    if (ErrorHandler == 0)
        reportError();
    return lp;
}

listelement *RemoveItem(listelement *listpointer)
{
    cpu_irq_disable();
    listelement * tempp = listpointer;

    while (listpointer->syllables != NULL)
    {
        RemoveSyllable(listpointer->syllables);
    }
    if (listpointer != NULL)
    {
        tempp = listpointer->link;
        free (listpointer->dataitem);
        free (listpointer->libWord);
        free (listpointer);
    }
    cpu_irq_enable();   
    return tempp;
}

void RemoveSyllable(listelement *listpointer)
{
    while (listpointer->syllables != NULL)
    {
        RemoveSyllable(listpointer->syllables);
    }
    free(listpointer->dataitem);
    free(listpointer->libWord);
    free(listpointer);
    listpointer = NULL;
    return;
}

static void cpu_irq_disable(void) { AVR32_USART0 = 0; }
static void cpu_irq_enable(void)  { AVR32_USART0 = 1; }   
static void usart_write_line(int *ptr, char *msg)
{
    *ptr = !*ptr;
    fprintf(stderr, "%s", msg);
}

int main(void)
{
    listelement *recordedWordsPointer = 0;
    recordedWordsPointer = RemoveItem(recordedWordsPointer);
}

I've not proven that every single change in there is sound, but I am sure that the addSyllable() and addItem() functions are a whole heap easier to read with the makeElement() function doing most of the repeated work. There is undoubtedly room for improvement. The code needs a working main() that really exercises the functions (and the code above does not qualify).

I have reservations about how the pointer to the array is being handled, but I have not taken the code to a machine where I can usefully run valgrind (Mac OS X 10.8.x unfortunately is not supported by valgrind, yet). I have not proved that it is wrong; I am suspicious that it is wrong. It would help me no end to see the calling code, and the definition of the variable passed as a pointer to an array.

Collectives™ on Stack Overflow

Program crashing using malloc() and free()

2 Answers 2

1 Comment

Initial observations

Refactored code

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

Initial observations

Refactored code

Comments

Your Answer

Sign up or log in

Post as a guest

Related