C Segmentation Fault in strcmp for-loop

Question

Novice programmer learning C, I'm encountering this 'segmentation fault (core dumped)' error while trying to run a for-loop with strcmp. I have seen questions on similar issues with strcmp, but they don't seem to address my problem. Here's the program I've written.

#include<stdio.h>
#include<string.h>

int main() {
  char ftpstring[15];
  printf("\nEnter valid ftp command > ");
  fgets(ftpstring,15,stdin);

  const char* ftp[] = { "ascii", "recv", "send", "rmdir", "mkdir" , "pwd", "ls", "cd", "status", "quit" };

  for ( int i = 0; i <= 10; i++ ) {
    int comparison;
    comparison = strcmp(ftpstring, ftp[i]);
    if (comparison == 0 ) {
      printf("%s is a valid ftp command.", ftpstring);
      break;
    }
    if(i == 10) {
      printf("%s is NOT a valid ftp command.", ftpstring);
    }
  }
}

As you can see, this program tries to read user input to determine if it matches one of the predefined valid ftp commands, then return whether or not it does.

`for ( int i = 0; i <= 10; i++ )` should be `for ( int i = 0; i < 10; i++ )` — Alex Lop., Apr 17 '17 at 15:45

Alex Lop. · Answer 1 · 2017-04-17T16:46:12.327

1

for ( int i = 0; i <= 10; i++ ) should be for ( int i = 0; i < 10; i++ )

The ftp array contains 10 strings, so the loop should be from 0 to 9 including.

More general solution could be

for ( int i = 0; i < sizeof(ftp)/sizeof(ftp[0]); i++ )

But it is better to define a macro

#define FTP_NUM_OF_COMMANDS 10

and define the ftp array as following:

const char* ftp[FTP_NUM_OF_COMMANDS] = { "ascii", "recv", "send", "rmdir", "mkdir" , "pwd", "ls", "cd", "status", "quit" };

In this case the compiler will also verify that you don't initialize it (by mistake) with more than 10 values. The for loop will look like this:

for ( int i = 0; i < FTP_NUM_OF_COMMANDS; i++ )

Also note that the following code should be moved outside the for loop

if(i == FTP_NUM_OF_COMMANDS) {
  printf("%s is NOT a valid ftp command.", ftpstring);
}

i==FTP_NUM_OF_COMMANDS will never occur inside the loop itself, if that condition is true, the for loop should break. Make sure that you define i outside the for loop scope so it would be available after the for loop breaks.

edited Apr 17 '17 at 16:46

answered Apr 17 '17 at 15:46

Alex Lop.

6,810
1
26
45

Even better would be to do `sizeof(ftp)/sizeof(ftp[0])` -- because then you can change the size or type of elements in `ftp` without causing a bug if you don't notice this line. – gilez Apr 17 '17 at 15:54
@gilez Agree! Edited. Thanks! – Alex Lop. Apr 17 '17 at 15:55
Just to say, i rather write `for (int i = 0; ftp[i] != NULL; i++)` – albttx Apr 17 '17 at 15:57
1

@ale-batt It would be a mistake. Nothing guarantees that `ftp [10]` will always be 0 (`NULL`). – Alex Lop. Apr 17 '17 at 16:00
You're right, it's just i used to NULL-ended my array – albttx Apr 17 '17 at 16:01

score 0 · Answer 2 · edited May 23 '17 at 11:54

You are making a comparison past the end of the array: the for loop should stop at 9, while yours goes past the end of the array.

Using 10 as a "magic number" is not a good choice, either: it is much better to have the compiler compute the size for you. Finally, it is better to use index after the loop to decide if the command has been found or not:

int index = -1;
for ( int i = 0 ; i != sizeof(ftp) / sizeof(*ftp) ; i++ ) {
    if (!strcmp(ftpstring, ftp[i])) {
        index = i;
        break;
    }
}
if (index == -1) {
    printf("%s is NOT a valid ftp command.", ftpstring);
}

C Segmentation Fault in strcmp for-loop

2 Answers2