0

for some reason my code works fin on my system running ubuntu 16.04 but when I run it on my schools computer (also running Ubuntu 16.04) I get a segmentation fault. The line when I run debugger claims it is coming from the print loop at the end which makes no sense as a int should not trigger a segmentation fault. 

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdbool.h>
#include <ctype.h>

int RETURN = 0; //return value to change if error found

// check if entered value is a number
bool isNumericChar(char x)
{
    return (x >= '0' && x <= '9')? true: false;
}

// Recreated atoi to check for non ints
int myAtoi(char *str)
{
  if (*str == '\0'){
    return 0;
  }

  int res = 0; 
  int sign = 1;
  int i = 0;  

  // If number is negative, then update sign
  if (str[0] == '-'){
    sign = -1;
    i++;  
  }

  // Iterate through all digits of input string and update result
  for (; str[i] != '\0'; ++i){
    if (isNumericChar(str[i]) == false){
      RETURN = 1;
      fprintf(stderr, "must be integer\n");
      break;
      return 0;
    }
    res = res*10 + str[i] - '0';
  }

    // Return result with sign
  return sign*res;
}

// this is main struct of a node of the linked list
struct LL {
  int number;
  struct LL *next;
};

// this function allocates memory for a node and returns the
// allcoated node item
struct LL *makeNode(int num) {
  struct LL *node = (struct LL *) malloc( sizeof(struct LL) );
  node->number = num;
  return node;
}

int main() {
  char input[10];   
  struct LL * stack_head; 
  bool wasPush = false; 
  int num = 0; 
  while(EOF != scanf("%s", input )) {
    if( 0 == strcmp(input, "push") ) {   // if the word is push, then next item may be a number
      wasPush = true;
    } 
    else if( 0 == strcmp( input, "pop" ) ) { 
      wasPush = false; 
      if( stack_head != NULL ) {
        stack_head = stack_head->next;   
      }
    } 
    else{
      // probably a number
      if( !wasPush ){
        continue;   // and last word was a push
      }
      int number = myAtoi(input);   //convert this to a number

      if( stack_head == NULL ) {   //an empty linked list then this is the first node
        stack_head = makeNode(number);
        stack_head->next = NULL;
      }
      else{   // list and can have a next item in it
        struct LL * prev_head = stack_head;
        stack_head = makeNode(number);
        stack_head->next = prev_head;
      }
      wasPush = false;
    }
  }

  // we print the items on the stack now by iterating
  // from the top to the bottom of the stack
  while( stack_head != NULL ) {
    num = stack_head->number;
    printf("%d\n", num );
    stack_head = stack_head->next;
  }
  return RETURN;
}
ender554
  • 47
  • 1
  • 7
  • 5
    `makeNode` should set `node->next = NULL` also initialize `stack_head` to `NULL` in your main. – cleblanc Jul 10 '18 at 20:34
  • 2
    Your use of scanf is quite unsafe. For a short discussion see this thread for example: https://stackoverflow.com/questions/1621394/how-to-prevent-scanf-causing-a-buffer-overflow-in-c – hetepeperfan Jul 10 '18 at 20:39
  • 1
    C has `isdigit` so you don't need to write `isNumericChar`. You already have the necessary header that defines it. – lurker Jul 10 '18 at 21:04
  • Thanks all! just needed to initialize it in main. As far as scanf I know it is unsafe, unfortunately it was part of the requirement for this assignment. – ender554 Jul 11 '18 at 04:14
  • regarding: `break; return 0;` in function: `myAtoi()`, the 'return' statement will never be executed because the 'break' exits the 'for()' loop so instead the statement: `return sign*res;` will be executed – user3629249 Jul 11 '18 at 14:47
  • OT: when calling any of the heap allocation functions: `malloc` `calloc` `realloc`: 1) the returned type is `void*` which can be assigned to any other pointer. Casting just clutters the code, making it more difficult to understand, debug, etc. 2) always check (!=NULL) the returned value to assure the operation was successful. – user3629249 Jul 11 '18 at 14:51
  • OT: for ease of readability and understanding: 1) separate code blocks: `for` `if` `else` `while` `do...while` `switch` `case` `default` via a single blank line. – user3629249 Jul 11 '18 at 14:54
  • regarding: `while(EOF != scanf("%s", input )) {` there are other reasons a call to `scanf()` can fail besides EOF. strongly suggest (in this case) to use: `while( 1 == scanf("%s", input )) {` Also, when using the '%s' and/or '%[...]' input format specifiers, always include a MAX CHARACTERS modifier that is one less than the length of the input buffer to avoid any chance of a buffer overflow (and the attendant undefined behavior) – user3629249 Jul 11 '18 at 14:56
  • the posted code contains some 'magic' numbers. 'magic' numbers are numbers with no basis. I.E. 10. 'magic' numbers make the code much more difficult to understand, debug, etc. Suggest giving those 'magic' numbers meaningful names via a `enum` statement or `#define` statement, then using those meaningful names throughout the code – user3629249 Jul 11 '18 at 14:59
  • the posted code contains a major memory leak. This is because many memory allocations (via call to malloc()` ) were made, but the resulting memory was never returned to the heap (via calls to `free()` – user3629249 Jul 11 '18 at 15:04
  • regarding: ` while( stack_head != NULL ) { num = stack_head->number; printf("%d\n", num ); stack_head = stack_head->next; }` This destroys the pointer to the head of the linked list, This results in two problems. 1) the list cannot ever be referenced again 2) the pointer cannot be used to initiate passing the memory back to the heap. – user3629249 Jul 11 '18 at 15:08
  • it is a poor programming practice to `#include` header files those contents are not being used. Suggest removing: `#include ` – user3629249 Jul 11 '18 at 15:11

1 Answers1

4

Try initializing stack_head to NULL. Local variables are not guaranteed to be initialized, so your stack_head == NULL check may fail.

meat
  • 609
  • 3
  • 8