views:

159

answers:

3

Hi there. I have some code to add fractions.

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

struct frac
{
    int enumerator;
    int denominator;
};
typedef struct frac frac_t;


frac_t *Add(frac_t *b1, frac_t *b2)
{
  frac_t rfrac;
  frac_t *p;
  p = &rfrac;
 (*p).enumerator= ((*b1).enumerator* (*b2).denominator) + ((*b2).enumerator* (*b1).denominator);
 (*p).denominator= ((*b1).denominator* (*b2).denominator);
  return p;
}

int main(void)
{
  frac_t b1 = {2,4};
  frac_t b2 = {1,7};
  frac_t *add = Add(&b1, &b2);
  printf("%i %i\n", add->enumerator, add->denominator);
  system("pause");
  return 0;
}

This works perfectly fine. The result is: 3 5, as it is supposed to be.

If I add a "printf" it totally messes up my result:

int main(void)
{
 frac_t b1 = {2,4};
 frac_t b2 = {1,7};
 frac_t *add = Add(&b1, &b2);
 printf("addition:\n"); 
 printf("%i %i\n", add->enumerator, add->denominator);
 system("pause");
 return 0;
}

The result is:

addition:

2008958704 -1

What went wrong?

+10  A: 

Your function Add is returning a pointer to a temporary variable created within that function. Once that function has returned, the program can use that memory in any way it wishes; you should not be accessing it any longer. You got lucky the first time -- the program happened to leave that area of memory alone, and your result was preserved. Adding in the second printf caused additional modifications to the stack memory, and this overwrote your original values and exposed the bug.

You should pass a pointer to frac_t to the function Add instead to get your result. E.g.:

void Add(frac_t *result, frac_t *b1, frac_t *b2) {
    // modify result here
}
int3
+1 for a precise reason.
nthrgeek
A: 

You are returning the address of an object that is local to the function Add. This means that once you leave the function the address is no longer valid, the object has been destroyed.

If you try to access the object, sometimes it might work (as in your first example), but much of the time it won't and you cannot rely on what the program might do.

You need to change to function either to return a struct by value and not a pointer to a local struct, or to take in a pointer to a struct to which it should write a result, or to dynamically allocate memory for the result and return a pointer to this memory. In the last case the caller would have to be responsible for freeing that memory.

Charles Bailey
+1  A: 

In

frac_t *Add(frac_t *b1, frac_t *b2)
{
  frac_t rfrac;
  frac_t *p;
  p = &rfrac;
 (*p).enumerator= ((*b1).enumerator* (*b2).denominator) + ((*b2).enumerator* (*b1).denominator);
 (*p).denominator= ((*b1).denominator* (*b2).denominator);
  return p;
}

you return the address of a local variable, rfrac. This gives you undefined behaviour when you come to use it. The printf() call simply caused the UB to manifest itself.

anon