Segmentation Fault while freeing memory in C

1
votes

So I'm running the program to overcome integer overflow and while i'm debugging it runs through the first command hugePrint(p = parseString("12345")); without an issue. It runs the code after, hugeDestroyer(p); and frees the memory without a problem. Once I run the next line of code in Ubuntu i'm getting a seg fault and it won't even print the numbers but on windows it prints "354913546879519843519843548943513179" and then gives me a seg fault after it runs hugeDestroyer(p); again. Also Having problems running a NULL value through my parseString() function. Any help is greatly appreciated to shed light on the seg faults.

// main
#include <stdio.h>
#include <stdlib.h>
#include <limits.h>
#include "Fibonacci.h"

// print a HugeInteger (followed by a newline character)
void hugePrint(HugeInteger *p)
{
    int i;

    if (p == NULL || p->digits == NULL)
    {
        printf("(null pointer)\n");
        return;
    }

    for (i = p->length - 1; i >= 0; i--)
        printf("%d", p->digits[i]);
    printf("\n");
}

int main(void)
{
    HugeInteger *p;

    hugePrint(p = parseString("12345"));
    hugeDestroyer(p);

    hugePrint(p = parseString("354913546879519843519843548943513179"));
    hugeDestroyer(p);

    hugePrint(p = parseString(NULL));
    hugeDestroyer(p);

    hugePrint(p = parseInt(246810));
    hugeDestroyer(p);

    hugePrint(p = parseInt(0));
    hugeDestroyer(p);

    hugePrint(p = parseInt(INT_MAX));
    hugeDestroyer(p);

    hugePrint(p = parseInt(UINT_MAX));
    hugeDestroyer(p);

    return 0;
}

//fibonacci.c
#include <stdio.h>
#include <stdlib.h>
#include <limits.h>
#include "Fibonacci.h"


// Functional Prototypes
/*
HugeInteger *hugeAdd(HugeInteger *p, HugeInteger *q)
{

}
*/


HugeInteger *hugeDestroyer(HugeInteger *p)
{
    int i;
    if ( p == NULL )
        return NULL;

    if( p->length != NULL)
        free(p->length);

    if ( p->digits != NULL )
    {
        for ( i = 0; i < p->length; i++)
            free( p->digits[i] );
        free( p->digits );
    }
    free( p );

    return NULL;

}

HugeInteger *parseString(char *str)
{

    int counter, reverseCount = 0;
    HugeInteger *numArray = malloc(sizeof(HugeInteger));

    if ( str == NULL)
    {
        return NULL;
    }
//checks for NULL pointer
    if ( numArray == NULL)
    {
        return NULL;
    }

// Dynamically allocate memory for array of numbers
    numArray->length = strlen(str);
    numArray->digits = malloc( sizeof(int) * numArray->length+1);

    if ( numArray->digits == NULL)
        return NULL;

    for( counter = numArray->length - 1 ; counter >= 0 ; counter--)
    {
        numArray->digits[reverseCount] = str[counter];

// Handles conversion from ASCII to integer format
        switch(numArray->digits[reverseCount])
        {
        case 48:
            numArray->digits[reverseCount] = 0;
            break;
        case 49:
            numArray->digits[reverseCount] = 1;
            break;
        case 50:
            numArray->digits[reverseCount] = 2;
            break;
        case 51:
            numArray->digits[reverseCount] = 3;
            break;
        case 52:
            numArray->digits[reverseCount] = 4;
            break;
        case 53:
            numArray->digits[reverseCount] = 5;
            break;
        case 54:
            numArray->digits[reverseCount] = 6;
            break;
        case 55:
            numArray->digits[reverseCount] = 7;
            break;
        case 56:
            numArray->digits[reverseCount] = 8;
            break;
        case 57:
            numArray->digits[reverseCount] = 9;
            break;
        default:
            numArray->digits[reverseCount] = 0;
            break;
        }
        reverseCount++;
    }

    return numArray;

}

HugeInteger *parseInt(unsigned int n)
{
    HugeInteger *number = malloc(sizeof(HugeInteger));
    if( number == NULL )
        return NULL;

    if(n > UINT_MAX )
        return NULL;

    number = n;

    return number;

}
unsigned int *toUnsignedInt(HugeInteger *p)
{
    unsigned int *newInteger = malloc(sizeof(unsigned int));

    if( newInteger == NULL)
    {
        return NULL;
    }

    if (p == NULL || p > UINT_MAX )
    {
        return NULL;
    }

    newInteger = p;

    return newInteger;

}
/*
HugeInteger *fib(int n)
{
    // base cases: F(0) = 0, F(1) = 1
    if (n < 2)
        return n;
// definition of Fibonacci: F(n) = F(n – 1) + F(n - 2)
    return fib(n – 1) + fib(n – 2);
}
*/

//fibonacci.h
#ifndef __FIBONACCI_H
#define __FIBONACCI_H

typedef struct HugeInteger
{
    // a dynamically allocated array to hold the digits of a huge integer
    int *digits;

    // the number of digits in the huge integer (approx. equal to array length)
    int length;
} HugeInteger;


// Functional Prototypes

HugeInteger *hugeAdd(HugeInteger *p, HugeInteger *q);

HugeInteger *hugeDestroyer(HugeInteger *p);

HugeInteger *parseString(char *str);

HugeInteger *parseInt(unsigned int n);

unsigned int *toUnsignedInt(HugeInteger *p);

HugeInteger *fib(int n);


#endif
3
cpointersmemoryintegerfree
I've removed the 'free(p->length)' thank you for pointing that out. I've noticed that at run time the program is not freeing past 10 indexes of p->digits[i] within the for loop which is bringing up my seg fault - Chris.r.Torres
Check my edit to see why it does not go pass 10 indexes. - Eric Fortin
You are illegally interchanging pointers and integers all over the place. The compiler should be spewing warnings at you. Pay attention to them! Also, how can an unsigned int ever be greater than UINT_MAX? Also also, why use a switch to implement a linear function? Try d = (c >= '0' && c <= '9') ? c - '0' : 0 - pat
I will definitely try that ternary, looks much more simple. The purpose of the (n > UINT_MAX) is to not allow the user to input a number larger than the max, instead I'm simply to return null - Chris.r.Torres
But the argument is an unsigned int, so by definition, it cannot hold a value greater than UINT_MAX, since UINT_MAX is defined to be the largest value that can be held in an unsigned int. - pat

3 Answers

1
votes

You have multiple problems in your code.

When you malloc to get a HugeInteger, you don't initialize the memory returned to 0. Since you have early return in your method when str == NULL, members of the struct aren't initialized so when you call hugeDestroyer on it, you get undefined behavior because you don't know what memory looks like in your struct. More than likely p->digits is not null and it tries to free memory from it.


In hugeDestroyer you have this code

if( p->length != NULL)
    free(p->length);

length is an int and is not dynamically allocated with malloc so calling free on this interprets the int as an address. Undefined behavior again.


Also in hugeDestroyer, you have this code

for ( i = 0; i < p->length; i++)
    free( p->digits[i] );
free( p->digits );

This is also wrong, p->digits is allocated once with the right size. You only need to call free(p->digits) since c runtime remember the size of allocation. Your seg fault is again caused by free interpreting values in digits array as memory addresses to free.

1
votes

Assuming your huge integer representation is going to use decimal digits, ParseInt is going to need to break the argument down into individual decimal digits. Give this a try:

#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include <limits.h>
#include "Fibonacci.h"

void hugeDestroyer(HugeInteger *p)
{
    if ( p != NULL ) {
        free( p->digits );
        free( p );
    }
}

HugeInteger *parseString(char *str)
{
    HugeInteger *huge;
    int i;

    if (str == NULL)
        goto error1;

    if ((huge = malloc(sizeof *huge)) == NULL)
        goto error1;

    huge->length = strlen(str);

    if ((huge->digits = malloc(huge->length * sizeof *huge->digits)) == NULL)
        goto error2;

    for (i = 0; i < huge->length; i++) {
        huge->digits[huge->length-i-1] =
            (str[i] >= '0' && str[i] <= '9') ? str[i] - '0' : 0;
    }

    return huge;

//error3:
//    free(huge->digits);
error2:
    free(huge);
error1:
    return NULL;
}

HugeInteger *parseInt(unsigned int n)
{
    HugeInteger *huge;
    int digits[10]; // largest 32-bit unsigned integer has 10 digits in base 10
    int length;

    if ((huge = malloc(sizeof *huge)) == NULL)
        goto error1;

    length = 0;
    do {
        digits[length++] = n % 10;
        n /= 10;
    }
    while (n);

    huge->length = length;

    if ((huge->digits = malloc(huge->length * sizeof *huge->digits)) == NULL)
        goto error2;

    memcpy(huge->digits, digits, huge->length * sizeof *huge->digits);

    return huge;

//error3:
//    free(huge->digits);
error2:
    free(huge);
error1:
    return NULL;
}
0
votes

I just looked at your HugeInteger struct and notice that length is not a pointer. Do not free it!

As @EricFortin mentioned, you do not initialize your struct after calling malloc():

HugeInteger *numArray = malloc(sizeof(HugeInteger));
numArray->length = 0 ; // add me
numArray->digits = NULL ; // add me

Same thing here:

HugeInteger *number = malloc(sizeof(HugeInteger));
numArray->length = 0 ; // add me
numArray->digits = NULL ; // add me

Every time you return early from any of your ParseInt() or ParseString() functions, you have a memory leak because you do not free your HugeInteger pointer.

Do this (I was wrong the first time, another thanks to @EricFortin):

    for ( i = 0; i < p->length; i++) // remove me
        free( p->digits[i] ); // remove me
    free( p->digits ); // keep me