Why does this recursive factorial function hit segmentation fault?

Question

This is the recursive function I'm using to calculate the factorial of an int. It compiles fine but when I run it, the program hits segmentation fault.

The function receives an int taken from the user. The function call is fact(&n).

GDB is not helping because when I "step" into the fact function it hits segmentation fault on the first pass through it.

Also the code is very simple and it is adjusted to the pseudo code from the walkthroughs/shorts.

Where is this code going so bad?

int fact (int* b)

int main (void)
{
    int a;
    do
    {
        a = GetInt();
    }
    while (a < 1);

    fact(&a);
    printf("Factorial: %i\n", a);
}

int fact (int* b)
{
     if (*b == 1)
     {
        return *b;
     }
     else
     {
         *b = *b * (*b - 1);
         fact(b);
     }
     return *b;
}

Answer 1

The fact() function is a recursive function that takes a pointer to an int as input. However, the internal recursive call, return *a * fact(a - 1); is not passing a valid pointer to an int. It's clear that you want to pass the next smaller integer, but you are actually passing the contents stored at the pointer minus 1, instead of a pointer address. This eventually leads to an attempt to access unauthorized or invalid memory.

If you need to pass an address pointer, then you need to be consistent. If not, it would be much simpler to just pass and return integers across the board instead of pointers.

[EDIT- post comments]

Hmmm, looks like I wasn't precise enough. My bad. In your recursive call, because of the way the function parameters are designed, you need to pass an address of the memory containing the number to be multiplied. Instead, you are passing the address of a minus 1, meaning that you are subtracting 1 from the address, not the value stored at the address. While writing this as an exercise to understand pointers(a good thing), it has problems. You have two options - either correct the recursive call or change it to eliminate all the pointers.

If you want to keep all the pointers, the recursive call needs to be changed. First, you need to subtract 1 from the value stored in a somehow, probably with an intermediate variable. Next, you need to execute the recursive call using that new value, while preserving a for the return part of the statement. You can't simply subtract 1 from an address because this would access a different area of memory than what you want.

Also, this means that when you initially call the function from main, you have to pass it the address of the data, not the actual data.

Or, you could simplify everything. Get rid of all the pointers and just pass by copy:

int fact (int a)
{
    if (a == 1)
    {
        return a;
    }
    else
    {
        return a * fact(a - 1);
    }
}

This also means that you would update your calling code with something like this:

int n = 4;
int result = fact(n);  // this would store 24 in result.

It completely eliminates the pointers and corrects the recursion address problem.

In your comment, you said that " I'm trying to get this function to get passed the value from an int variable in Main,..." If you are passing an int variable, then you need to do the latter. If you pass it an int var when it is expecting a pointer to an int var , then it is going to fail.

[EDIT 2: post comments]

The new function in your edit creates an infinite loop. Next, it isn't doing anything with any of the return values. The remaining issue is how a function is called. Here is the simplified method of doing this without the complexity of pointers, mallocs, etc. You need to master this first.

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

int fact (int b);

int main (void)
{
    int a;
    do
    {
        a = GetInt();
    }
    while (a < 1);

    int result = fact(a);
    printf("Factorial: %i\n", result);
}



int fact (int b)
{
    if (b == 1)
    {
        return b;
    }
    else
    {
        return b * fact(b - 1);
    }
}

If this answers your question, please click on the check mark to accept. Let's keep up on forum maintenance. ;-)