strcat() and fgets() in C not working

Question

For one reason or another, I can not get two instances of fgets() and strcat() to work.

This is my code:

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

int main()
{
  char str[150];

  char userName[8]; char password[8];
  printf("Enter username: ");fgets(userName, 9, stdin);
  printf("Enter password: ");scanf("%s", password);

  strcpy(str, "INSERT INTO USERS (ID,USERNAME,PASSWORD,IP) "  \
         "VALUES (1, '");
  strcat(str, userName);
  strcat(str, "', '");
  strcat(str, password);
  strcat(str, "', '120.132.12.1');");

  puts(str);
  return 0;
}

When getting the string for "password", I can not use fgets() or it throws the error "Abort trap: 6". Specifically I am using:

fgets(password, 9, stdin);

9 elements because I read you need to account for the null terminating char in your total.

I don't understand this because being that there are as many elements in the char array as userName, I thought the code to get the string should be virtually the same other than the char array name.

Also, strcat() seems to not be working when trying to append "str" with userName. My output looks something like this:

Enter username: beckah12
Enter password: beckah12
INSERT INTO USERS (ID,USERNAME,PASSWORD,IP) VALUES (1, '', 'beckah12', '120.132.12.1')

Why is it skipping over the "strcpy(str, userName)"?

Update

There is some memory overlap when I declare these two strings. When I declared a random integer between the two strings, it took retained the input for both. How do I fix this memory overlap with a more permanent solution?

Answer 1

For your fgets() you correctly stated:

9 elements because I read you need to account for the null terminating char in your total.

But in your variable declarations you forgot it:

char userName[8]; char password[8];

Make them char ....[9] and your program shouldn't abort any more.

But, as @chux mentioned:

if you read complete lines of text you will also have to deal with the new line character, which makes your code something like that:

char userName[10]; char password[10];
...
fgets( userName, sizeof userName, stdin );
if( userName[strlen(userName)-1] == '\n' ) {
    //truncate new line character
    userName[strlen(userName)-1] = '\0';
}

... and still you will have a problem if someone types a name or a password of more than 8 characters. So I would suggest:

#define L_USER 8

char userName[256]; char password[256];
...
fgets( userName, sizeof userName, stdin );
if( strlen(userName) > L_USER ) {
    //truncate name if too long
    userName[L_USER] = '\0';
}    
if( userName[strlen(userName)-1] == '\n' ) {
    //truncate new line character
    userName[strlen(userName)-1] = '\0';
}

Answer 2

It's a radical rewrite, but it is also effective. This is more along the lines of what I'd use:

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

static int prompt_for_data(const char *prompt, const char *what, char *buffer, size_t buflen)
{
    char line[256];
    assert(prompt != 0 && *prompt != '\0');
    assert(what != 0 && *what != '\0');
    assert(buffer != 0);
    assert(buflen > 1 && buflen < sizeof(line));

    printf("%s", prompt);
    if (fgets(line, sizeof(line), stdin) == 0)
    {
        fprintf(stderr, "EOF\n");
        return EOF;
    }
    size_t len = strlen(line);
    if (len == 0)
    {
        fprintf(stderr, "Zero length string? You can't use Control-@ in a %s\n",
                what);
        return EOF;
    }
    if (line[len - 1] != '\n')
    {
        fprintf(stderr, "Line far too long (%zu bytes)\n", len);
        return EOF;
    }
    line[--len] = '\0';
    /* Optionally chop leading and trailing white space - omitted */
    if (len > buflen - 1)
    {
        fprintf(stderr, "Maximum length of %s is %zu (you entered %zu characters)\n",
                what, buflen - 1, len);
        return EOF;
    }
    if (len == 0)
    {
        fprintf(stderr, "You're supposed to enter a value for the %s\n", what);
        return EOF;
    }
    strcpy(buffer, line);
    return 0;
}

int main(void)
{
    char userName[9];
    char password[9];

    if (prompt_for_data("Enter username: ", "username", userName, sizeof(userName)) == 0 &&
        prompt_for_data("Enter password: ", "password", password, sizeof(password)) == 0)
    {
        char str[150];
        snprintf(str, sizeof(str),
                 "INSERT INTO USERS (ID, USERNAME, PASSWORD, IP) VALUES"
                 "(%d, '%s', '%s', '%s');",
                 1, userName, password, "120.132.12.1");
        puts(str);
    }
    return 0;
}

The sequence of 'prompt for value, read a line, check its length, copy it to the desired variable' is encapsulated in the prompt_for_data() function. It is then used twice (but I'd probably write it as a function even if it was called just once) to get the username and password. Then, rather than using strcat(), I use snprintf() to create the output string.

You should also note that if you want to change the size of your user name and password from the quaintly archaic 8 characters plus terminating null to 32, the only lines of code that change are the declarations of userName and password. This is because I've carefully used sizeof(userName) or sizeof(password) when I needed to describe the size. Similarly with the other variables; for example, the fgets() line doesn't change if I change the size of the line variable. It is usually good practice to minimize the number of necessary changes when you change the size of a variable.

Note that you still have SQL injection issues. If I type "O'Reilly" as the user name, the SQL is invalid. Remember Little Bobby Tables! One good thing about short user names; it makes SQL injection harder.

Sample runs

$ ./up
Enter username: beckah12
Enter password: beckah12
INSERT INTO USERS (ID, USERNAME, PASSWORD, IP) VALUES(1, 'beckah12', 'beckah12', '120.132.12.1');
$ ./up
Enter username: absolom12
Maximum length of username is 8 (you entered 9 characters)
$ ./up
Enter username: beckah12
Enter password: absolom12
Maximum length of password is 8 (you entered 9 characters)
$ ./up
Enter username: 
You're supposed to enter a value for the username
$ ./up
Enter username: beckah12
Enter password: 
You're supposed to enter a value for the password
$ ./up
Enter username: EOF
$ ./up
Enter username: beckah12
Enter password: EOF
$ ./up
Enter username: nnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnn
Line far too long (255 bytes)
$

Answer 3

Change The lines

char userName[8]  to  char userName[9]

char password[8]  to  char password[9]

because that is what you have specified in fgets(userName, 9, stdin); et. al..