My user input gives a segmantation fault in C

Question

So I've been reading and practicing C for a few months now on and off, and I finally decided to start hacking away at some code. So I googled for some simple programs to try writing and the first one was a program to reverse whatever the user input is. Now I don't want any answers but if you could maybe clue me in if I'm using any functions wrong or something.

the code is: (ignore the printf of the strlen its for debugging)

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

int main(int argc, char *argv[]) {

    char command[100];

    printf("please enter string: ");
    fgets(command, 100, stdin);

    //printf("%d", strlen(command));

    int ha = strlen(command);

    while(1){
        if(!strcmp(command,"quit")) {
            for(int i = 0; i < ha; i++) {
                int str = strlen(command) - i;
                puts(command[str]);
            }
        } else {
            exit(0); 
        }

        printf("please enter string: ");
        fgets(command, 100, stdin);
    }
    return 0;
}

It compiles with no warnings or errors but when ran after entering some input it gives me a segmentation fault. any help is very much appreciated!

`fgets()` leaves the newline at the end of the string. So it will never match `"quit"`, you need to compare with `"quit\n"`. Or remove the newline. — Barmar, Nov 23 '15 at 23:10
`puts(command[str]);` is wrong. Did you mean to use `putc`?, like `putc(command[str]);` — R Sahu, Nov 23 '15 at 23:11
Check your compiler warnings. The argument to `puts()` should be a `char*`. `command[str]` is a `char`. — Barmar, Nov 23 '15 at 23:11
Consistent indentation would make this code *much* easier to read. You assign `ha` to the length of the *first* input line, and you never update it to represent the length of later lines. — Keith Thompson, Nov 23 '15 at 23:12
int str = strlen(command) - i; Never call an int 'str'. and you mean int j = ha - i; — pm100, Nov 23 '15 at 23:13
ok thanks I thought it would still work like that, but I fixed it according to how they show on the C library. — AppAttacker, Nov 23 '15 at 23:16
that was to pzarnger... i don't have a problem with a continuous loop but i'll fix the "quit/n" — AppAttacker, Nov 23 '15 at 23:19
To truncate the potential trailing `'\n'` after `fgets()`, use http://stackoverflow.com/a/28462221/2410359 — chux - Reinstate Monica, Nov 23 '15 at 23:19
You should be careful with `strlen()`. A string in c is just an array of bytes with a special `'\0'` value marking the end of the string. So `strlen()` looks for it iterating trhough the string every time you call it. The string length is not stored anywhere. You should always think of such things when calling a function. — Iharob Al Asimi, Nov 23 '15 at 23:46
`puts(command[str]);` is an error. If you really "compile with no warnings or errors" you must figure out how to invoke your compiler properly. — M.M, Nov 24 '15 at 00:12
Is it your intent that your "if" statement tests "true" if "command" is EQUAL to "quit"...??? — TonyB, Nov 24 '15 at 00:15
@Barmar it could match `"quit"` if the user typed `quit` and then indicated end-of-file. In fact that is the *only* way the program could do anything other than immediately `exit(0)`, so the occurrence of segfault suggests that either that happened, or this isn't the real code. (probably the latter) — M.M, Nov 24 '15 at 00:16
this line: "It compiles with no warnings or errors" is 100% wrong. when compiling always enable all the warnings, then fix those warnings. (in gcc, at a minimum use: `-Wall -Wextra -pedantic`) then you would see warnings about unused parameter `argc` and `argv[]`: suggest `int main( void )`. implicit definition of function `exit()`: add `#include `. parameter passed to `puts()` is char, it should be `char *` . These are rather basic things, that indicate either the warnings were ignored or the posted code never compiled. — user3629249, Nov 24 '15 at 00:23
always check the returned value from any user input, in this case from the call to `fgets()` to assure the operation was successful — user3629249, Nov 24 '15 at 00:26
please read/understand the man pages for those system functions used in a program. For instance, `strlen()` returns a `size_t` not a int and not a 'str' and this line: `puts(command[str]);` should be: `putc(command[str]);` — user3629249, Nov 24 '15 at 00:33
the posted code can be massively shortened/simplified by using `do{...}while();` rather that `while(){}` — user3629249, Nov 24 '15 at 00:37
the posted code contains 'magic' numbers. 'magic' numbers make the code much more difficult to understand, debug, maintain. the 'magic' number is 100. Suggest using #define to give the 'magic' number a meaningful name, then using that meaningful name throughout the code — user3629249, Nov 24 '15 at 00:39
the variable `ha` is only being calculated for the first input string, so is an invalid limit value for all subsequent input strings. — user3629249, Nov 24 '15 at 00:41
calling the function: `strlen()` is 'relatively' expensive in CPU cycles and elapsed time. suggest calling only once per input string, into a local variable, then using that local variable thereafter for that input string — user3629249, Nov 24 '15 at 00:43
@user3629249 modern compilers will hoist out repeated calls to `strlen` on the same string — M.M, Nov 24 '15 at 00:52
when calling the function:` fgets()` the trailing is also input into the buffer (unless the buffer is full first), then `fgets()` always appends a NUL byte to the buffer. to make good use of any of the string functions, that needs to be replaced with a NUL byte. (in windows, a is a multibyte entity so just using `inputbuffer[ strnlen(inputbuffer)-1] = '\0';` will not work.) Suggest using: `if( NULL != ( char *newline = strstr( inputbuffer, "\n" ) ) ) { *newline = '\0'; } so the first byte of the is set to NUL. — user3629249, Nov 24 '15 at 00:53
@M.M, depending on some hidden optimization in a compiler is not a good idea and leaves the reader of the code (me for instance) thinking of the repeated calls to strlen() as a runtime expense that can be easily avoided (and the result is much easier to read/understand code. — user3629249, Nov 24 '15 at 00:57
IMO the simplest code is to have strlen in the loop since that's exactly what we are looping until; premature optimization makes the code messier — M.M, Nov 24 '15 at 01:34
@user3629249 Your comments about windows newlines are misplaced. `\r\n` occurs in the file but this is invisible to the C program; the implementation translates whatever the operating system's like-ending indicator is, into `\n`. This way, portable code can be written which assumes lines end in `\n`. — M.M, Nov 24 '15 at 01:34
Technically the return value of `fgets` should be checked; if it fails (e.g. eof occurs with no characters being read) the buffer is indeterminate. — M.M, Nov 24 '15 at 01:36
@M.M, That would be nice IF the call to `fgets()` did not input the however, `fgets()` does input the right into the buffer. so it is in the buffer. In windows, that is a 2 byte string. — user3629249, Nov 24 '15 at 01:47

score 1 · Answer 1 · answered Nov 24 '15 at 00:25

The segmentation fault is from the puts function. The argument should be char* (puts (command)).

The if condition wont't be true, because fgets read "quit\n", you can delete the character with strtok(command,"\n") or you can add the \n to the strcmp

I would try to compile the code with more warnings, I always use -ansi -pedantic -Wall too.

My user input gives a segmantation fault in C

1 Answers1