Why does a certain part of the code output Segmentation Fault?

Question

I'm making this console application where the user has to enter inputs. However, when the code gets to the part where the user inputs a file name which then gets added into a file path using strcat, it outputs Segmentation Fault.. Here is the full code:

int main(int argc, char *argv[])
{   
    char theFilePath[512];
    char theIP[20];
    char theFile[100];
    char password[1];
    char username[10];

printf("Username: ");
scanf("%s" , &username);

printf("Enter password: ");
scanf("%s", &password);

printf("Enter IP: ");
scanf("%d" , &theIP);

printf("Please specify the file: ");
scanf("%s" , &theFile);

strcat(theFilePath, "./passfiles/");
strcat(theFilePath, theFile);
strcat(theFilePath,".pf");
sprintf(theFilePath,"%s",theFilePath);


if (!(file_exist (theFilePath)))
{
    printf("The file cannot be found in the path %s", theFilePath);
    exit(EXIT_FAILURE);
} else 
{
    printf("The file exists!");
}
}

Any ideas why it does that?

Thanks in advance!

Standard error. `strcat` requires the first param to be a NUL terminated string. You don't have that. `theFilePath` is uninitialized and can contain random garbage. — kaylum, Feb 06 '17 at 01:05
@DaveNewton a zero-character password, allowing for the null terminator — M.M, Feb 06 '17 at 01:10
Maybe the error is from the function 'file_exist'? `int file_exist (char *filename) { struct stat *buffer; return (stat (filename, &buffer) == 0); }` — GeorgeDG, Feb 07 '17 at 12:38

score 1 · Answer 1 · edited May 23 '17 at 12:33

You have (at least) a couple of issues.

The first is the fact that you generally shouldn't be using an unbounded %s is scanf since it does not protect against buffer overflows (very similar to gets, which was deprecated in C99 and removed from C11).

This is doubly important when your buffers are pitifully small such as char password[1] which means, with the null terminator, all your passwords will have to be zero characters long. Now I'm no world-famous security researcher but I'm reasonably confident there's a security hole there in that scheme somewhere :-)

There are far safer options when needing user input, such as the function that can be found here, one that protects against buffer overflow and provides information on (and recovers from) the user trying to enter too much data.

The second is relying on uninitialised memory. The theFilePath variable is not initialised so may contain arbitrary data in it, yet your use of strcat expects it to contain a num-terminated string. That's a bad assumption to make and can be fixed by simply making the first strcat into a strcpy or, since it's always being set to the same initial value, doing that as part of the variable declaration itself:

char theFilePath[512] = "./passfiles/";

And, as an aside, assuming you're using a modern C compiler, you would be better declaring the variables at the point where you need them, rather than all at the top of the function, meaning this declaration should go where the current (errant) strcat currently is. Localising your declaration and use greatly aids readability.

You may also want to think about the usefulness of the statement:

sprintf(theFilePath,"%s",theFilePath);

Even if it works, it's effectively a no-op since all it does is copy some data to exactly the same place as it currently exists. In any case, it's not guaranteed to work since the standard quite clearly states in C11 7.21.6.6./2:

If copying takes place between objects that overlap, the behavior is undefined.

So I've made the first `strcat` into a `strcpy` and also declared the `theFilePath` variable like so: `char theFilePath[512] = "";` but it still outputs Segmentation Fault... — GeorgeDG, Feb 06 '17 at 18:47

score 0 · Answer 2 · answered Feb 06 '17 at 01:12

The character array

char theFilePath[512];

was not initialized by a string. So you may not use the standard C function strcat that concatenates strings. As result the program has undefined behavior.

You could initialize it for example either like

char theFilePath[512] = "";

or like

char theFilePath[512] = { 0 };

or like

char theFilePath[512];
theFilePath[0] = '\0';

Take into account that the scanf function is not safe. It is better to use function fgets. Also you have to write

printf("Username: ");
scanf("%s" , username);
             ^^^^^^^

instead of

printf("Username: ");
scanf("%s" , &username);
             ^^^^^^^^^

Arrays are implicitly converted (with very rare exceptions) to pointers to their first elements in expressions.

score 0 · Answer 3 · answered Feb 06 '17 at 01:52

While the above answers are all helpful and correct, I would like to offer a slightly different perspective.

scanf is safe if used safely. For example,

char username[10];
scanf("%10s" , username);

is safe. Unfortunately it's not possible to pass sizeof username to scanf.

For filenames, Posix defines PATH_MAX in limits.h as the maximum valid length of a filename. If your system doesn't supply that definition, er, I recommend switching to one that does. In a crunch, MAX_PATH is available on Windows.

I also suggest looking at the man page for getopt(3). While it's useful to learn how to use scanf, it's good practice to accept command-line options for user-provided information that is needed only at startup time.

Finally, see what you can do to crank up your warning level. My recent version of clang, for example, flags your incorrect use,

$ cc -std=c11     addr.c   -o addr
addr.c:9:16: warning: format specifies type 'char *' but the argument 
has type 'char (*)[10]' [-Wformat]
  scanf("%s" , &username);
         ~~    ^~~~~~~~~
1 warning generated.

Such warnings provide feedback quicker even than questions on SO. ;-)

Why does a certain part of the code output Segmentation Fault?

3 Answers3