Segfault when popping a string from an empty stack

[j4james]

Been meaning to file this one for a while now. Basically any of the operations that take a string parameter will cause a segfault when operating on an empty stack. A simple test case is:

=@

This should attempt to "execute" an empty string, which on most platforms does nothing, but on FBBI this will generate a segfault because of a null pointer reference.

The fault occurs in the stack_pop_string function. On an empty stack, the initial stack_pop call on line 119 returns null, and the while loop is never executed, but since s->sp == -1 is true on an empty stack, the null that was assigned to x is dereferenced in the final condition on line 126, and thus we get a segfault.

I think the fault can be prevented with the following replacement code:

void stack_pop_string(stack * s, char * str)
{
  int i = 0;
  void * x = NULL;
  SDEBUG("Before Pop String", s);
  x = stack_pop(s);
  while ((x != NULL) && (*(char *)x != 0))
  {
    str[i++] = *(char *)x;
    x = stack_pop(s);
  }
  str[i] = (char)'\0';
}

And here's a more thorough test case that validates a couple of edge cases:

"llun ticilpmI ohce"=$ 0"llun ticilpxE ohce"=$ = a"kcats ytpmE"bk, @

In the program above, we're calling the execute command three times - first using a string with an implicit null terminatorr, then with an explicit null terminator, and finally using a completely empty stack. If all the calls are successful, the program should output:

Implicit null
Explicit null
Empty Stack

If we want to get more adversarial, though, there is still another potential segfault. The stack_pop_string function returns the popped string by copying it into a fixed length buffer (always 256 chars). So we can easily cause a segfault just by using a string longer than 256 characters. A simple test case:

'd3*k1 " ohce"= @

If you want to avoid this fault, you'd need to add a check in the stack_pop_string function to prevent it overflowing the buffer. Something like this should do the trick:

void stack_pop_string(stack * s, char * str)
{
  int i = 0;
  void * x = NULL;
  SDEBUG("Before Pop String", s);
  x = stack_pop(s);
  while ((x != NULL) && (*(char *)x != 0))
  {
    if (i+1 < 256) str[i++] = *(char *)x;
    x = stack_pop(s);
  }
  str[i] = (char)'\0';
}

Although ideally you shouldn't be hardcoding the size like that - it would be safer to pass that in as a parameter from the various calling locations.