0

So here is my issue, I have been trying to figure this out for the last 5 hours, I have a header file, a tester file, and a c source file. I would really like to understand what is happening and why so I can avoid the issue in the future. The header file declares the struct but does not define it:

typedef struct Stack *StackP;

and in my source file, Stack.c I have defined the stack:

struct Stack
{
  int top;
  int capacity;
  int count;
  ItemT items;
};

where ItemT is defined as char *

in the tester file, the call goes:

StackP stackPtr = newStack();

and what I have for my newStack function located in the c source file is:

StackP newStack(void) {
  struct Stack stack1;
  StackP stackPtr = &stack1;
  (stackPtr->items) = (ItemT)malloc(DEFAULT_CAPACITY*sizeof(ItemT));        

  (stackPtr->top) = -1;
  (stackPtr->capacity) = DEFAULT_CAPACITY;
  (stackPtr->count) = 0;    
  fprintf(stderr, "\nSuccesfully allocated memory to items...\n");

  return stackPtr;
}

now, my push function is:

void pushStack(StackP stackPtr, ItemT item) {           
  if ((stackPtr->count) == (stackPtr->capacity)) {
    fprintf(stderr, "\nERROR: Full stack.\n");
  }
  else {
    stackPtr->items = item;
    fprintf(stderr, "\nSuccessfully pushed %s on to the stack...\n", stackPtr->items);
    (stackPtr->items)++;
    (stackPtr->top)++;
    (stackPtr->count)++;
  }
}

My question is this: Have I don't something wrong in any of these blocks of code.

If I call a function that says:

return (stackPtr->count);

it will return a random set of numbers instead of 0, or 1. For instance, if I push 2 strings to the stack, instead of count being 2, count is 479622 or some other random long number. Why is this happening?

Again, I would like to know what I'm doing wrong and not just correct syntax because I really HAVE to understand this.

  • 1
    `return stackPtr;` do you realize you're returning the address of an automatic variable (`struct Stack stack1;`), resulting in a dangling pointer that invokes *undefined behavior* to utilize? – WhozCraig Jan 22 '15 at 21:05
  • 1
    In `newStack` you return a local variable as pointer, which is no good (you should malloc that) - Site note: please remove the C++ tag –  Jan 22 '15 at 21:05
  • You should move the definition of the `Stack` structure to a header file so it can be shared by other source files. Sometimes, the compiler needs to know more details than a forward declaration provides. – Thomas Matthews Jan 22 '15 at 21:09

1 Answers1

7

The program has undefined behaviour as it is returning the address of a local variable from a function:

StackP newStack(void) {
  struct Stack stack1;
  StackP stackPtr = &stack1;

  return stackPtr;
}

stack1 no longer exists when newStack exits. stackPtr must point to dynamically allocated memory if it is to exist beyond the scope of the function:

StackP newStack(void) {
  struct Stack stack1;
  StackP stackPtr = malloc(sizeof(*stackPtr));
  if (stackPtr)
  {
  }

  return stackPtr;
}

See Do I cast the result of malloc?

Community
  • 1
  • 1
hmjd
  • 120,187
  • 20
  • 207
  • 252
  • 1
    @FrankPalmasani keep in mind, you do *not* need to dynamically allocate your `Stack` if you change the return type of your function to `struct Stack` rather than `StackP`. In other words, returning `stack1` and having the function return a `struct Stack` (*not* a pointer-to) would be sufficient, so long as you also account for this in the free-logic. – WhozCraig Jan 22 '15 at 21:16
  • @hmjd yeah, it may be an unstated outer requirement the structure itself be dynamic, but from what I see, there is currently no reason it *must* be so. It definitely makes the `newStack()` implementation shorter (two lines). – WhozCraig Jan 22 '15 at 21:21