Local variables, free() and segfaults

[shrublet]

Hello there,

I am a Java programmer by trade and am VERY new to C so please excuse any blatant pointer ignorance, if possible. I am having a problem with the concept of returning something but also freeing it afterwards within a method.

I am implementing a stack program and for pop, I want it to return the name of the just-popped item but also free the struct that name lives in. The code is below- it will better illustrate what I am trying to say.

#define STACK_NAME_SIZE 100

struct StackElement {
  char   name[STACK_NAME_SIZE];
  struct StackElement *next;
};struct StackElement *stackTop = 0;

void push (char* aName) {
  struct StackElement* e = (struct StackElement*) malloc (sizeof (struct StackElement));
  strncpy (e->name, aName, STACK_NAME_SIZE);
  e->next  = stackTop;
  stackTop = e;
}


char* pop () {
  struct StackElement *e = stackTop;
  stackTop = e->next;
  char name1[STACK_NAME_SIZE];
  strcpy(name1,e->name);
  free (e);
  return *name1;
  }

As it stands now, I am getting a segfault. I think the problem is because I am not using malloc to initialize name1...? However, I really really want to avoid using malloc if possible because I have no opportunity to free name1 later (since we are given a bunch of test suites that we cannot alter). Ideally, I would like name1 to somehow be a local variable that the compiler will clean up after I return the name value.

I apologize if strcpy is not the right way to do it... I have tried straight assignment, char*s instead of char[]s, strncpy.... everything. I am at a loss. I would love any and all input (though it is a school assignment so I am not asking for a solution in code ready made, of course... I'd really like to understand my mistake).

Thank you!
-Shrublet

[Narue]

>As it stands now, I am getting a segfault.
I'm not surprised. You have the right idea in making a copy of the name to return, but you're attempting to return a pointer to a local variable, which is a bad idea. When a function returns, the local variables are conceptually destroyed. Thus, you can't use a pointer to the destroyed data.

If you don't want to use malloc, you really only have the choice of using an externally defined array to hold the name. For example:

char *pop ( char name[], size_t size )
{
  struct StackElement *save = stackTop;

  if ( strlen ( save->name ) >= size )
    return NULL;

  strcpy ( name, save->name );
  stackTop = save->next;
  free ( save );

  return name; /* Note: no indirection here */
}

[shrublet]

Oops! I should add that I know that the pop I posted is absolutely messed up as far as mixing up return values and pointers and stuff go. I meant to post my earlier version using malloc, which I'll copy here (if that's ok):

#define STACK_NAME_SIZE 100

struct StackElement {
  char   name[STACK_NAME_SIZE];
  struct StackElement *next;
};struct StackElement *stackTop = 0;

void push (char* aName) {
  struct StackElement* e = (struct StackElement*) malloc (sizeof (struct StackElement));
  strncpy (e->name, aName, STACK_NAME_SIZE);
  e->next  = stackTop;
  stackTop = e;
}


char* pop () {
  struct StackElement *e = stackTop;
  stackTop = e->next;
  char *name = (char*) malloc (sizeof (STACK_NAME_SIZE));
  strncpy(name, e->name, STACK_NAME_SIZE);
  free (e);
  return name;
  }

[shrublet]

Hi Narue!

Thank you for your reply. I do understand what you mean... I guess what I was trying to do before is, well, undoable! The only concern I have with storing the name to be returned in an external array is with a test like this (which is part of the assignment and the problem we are trying to fix):

void test6 () {
  printf ("test6:\n");
  char *x[2];
  push ("Zero");
  push ("One");
  x[0] = pop (popname);
  x[1] = pop (popname);
  push("Two");
  push("Three");
  printf ("%s\n", x[0]);
  printf ("%s\n", x[1]);
}

In this case, both x[0] and x[1] are pointing to the same value (the value in the external array which is being overwritten with each pop() call). Is there any way to get around this? Before, I was considering some sort of helper method but I am not sure if that would solve the problem or simply compound it.

My new pop() code is here:

#define STACK_NAME_SIZE 100

struct StackElement {
  char   name[STACK_NAME_SIZE];
  struct StackElement *next;
};

struct StackElement *stackTop = 0;

char  popname[STACK_NAME_SIZE];

void push (char* aName) {
  struct StackElement* e = (struct StackElement*) malloc (sizeof (struct StackElement));
  strncpy (e->name, aName, STACK_NAME_SIZE);
  e->next  = stackTop;
  stackTop = e;
}


char *pop ( char popname[] ) {

  struct StackElement *e = stackTop;
  stackTop = e->next;
  strcpy(popname, e->name);
  free (e);
  return popname;

  }

Again, thanks for your help. It is very much appreciated!
-Shrublet

[Narue]

>Is there any way to get around this?
Sure, don't use a pointer for the element type of the x array:

void test6 () {
  printf ("test6:\n");
  char x[2][STACK_NAME_SIZE];
  push ("Zero");
  push ("One");
  pop (x[0], STACK_NAME_SIZE);
  pop (x[1], STACK_NAME_SIZE);
  push("Two");
  push("Three");
  printf ("%s\n", x[0]);
  printf ("%s\n", x[1]);
}

[shrublet]

Ohhhh I see now! It didn't even occur to me to think of directly passing x[0]. Thank you again, Narue! You saved my bacon. :)

[rajdesire]

It is also advisible, if you are new to c..pl go through some good programming book. Richie 's programming book is an example of this.