Efficiency with strcpy, strcat and malloc

Question

I'm still learning C, but I'm trying to make sure I've got a decent grasp on working with "strings" and data structures.

If possible, I'd like a little input on how I'm handling this and see if

1. it could be done more efficiently
2. I'm setting myself up for unforeseen negative consequences
3. I'm using the functions appropriately

void test(char *username, char *password) {
 printf("Checking password for %s - pw: %s\n",username,password);
 char *query1 = "SELECT password FROM logins WHERE email = '";
 char *query2 = "' LIMIT 1";
 char *querystring = malloc(strlen(query1) + strlen(username) + strlen(query2) * sizeof(char));
 strncpy(querystring,query1,strlen(query1));
 strncat(querystring,username,strlen(username));
 strncat(querystring,query2,strlen(query2));
 printf("Query string: %s\n",querystring);
 mysql_query(mysql_con,querystring);
 MYSQL_RES *result = mysql_store_result(mysql_con);
 int num_fields = mysql_num_fields(result);
 int num_rows = mysql_num_rows(result);
 if (num_rows != 0) {
 MYSQL_ROW row;
 printf("Query returned %i results with %i fields\n",num_rows,num_fields);
 row = mysql_fetch_row(result);
 printf("Password returned: %s\n",row[0]);
 int comparison = strncmp(password, row[0], strlen(password));
 if (comparison == 0) {
 printf("Passwords match!\n");
 } else {
 printf("Passwords do NOT match!\n");
 }
 }
 else {
 printf("No such user... Password is invalid");
 }
 free(querystring);
}

Output of program:

Checking password for [email protected] - pw: 5f4dcc3b5aa765d61d8327deb882cf99
Query string: SELECT password FROM logins WHERE email = '[email protected]' LIMIT 1
Query returned 1 results with 1 fields
Password returned: 5f4dcc3b5aa765d61d8327deb882cf99
Passwords match!

• Alexis Wilke: As a side note, if you do not need to read the password, you could also add it to your WHERE ... clause, then the compare is done by the SQL. Although some people like to know whether the account exists (email found in table) versus invalid password... depends whether you'd like to distinguish between both.

Answer 1

Fairly good C. Few notes:

• sizeof(char) is by 1 definition. It doesn't hurt to multiply by it, but still totally unnecessary.

• malloc return value better be tested against NULL.

• Calling functions of strn (emphasis on n) family in this context is kinda paranoid: you've already allocated enough memory; no need for extra safety. In fact, I'd rather go for sprintf("%s%s%s", querystring, query1, username, query2).

• int comparison is just noise. It's OK to directly test results of strcmp(). BTW, strncmp in this context looks paranoid again.

• I am not familiar with mysql. Shouldn't you release row somehow?

Answer 2

You have an SQL injection vulnerability, due to the way you composed the SQL query by incorporating the value of user without escaping it first.

size_t username_len = strlen(username);
char *escaped_username = malloc(2 * username_len + 1);
unsigned long escaped_username_len = mysql_real_escape_string(
 mysql_con, escaped_username, username, username_len
);
const char *query_head = "SELECT password FROM logins WHERE email = '";
const char *query_tail = "' LIMIT 1";
char *query = malloc(strlen(query_head) + strlen(query_tail) + escaped_username_len + 1);
sprintf(query, "%s%s%s", query_head, escaped_username, query_tail);
free(escaped_username);
/* Do query here... */
free(query);

In addition, storing plaintext passwords is poor security practice. You should be storing salted password hashes. (If you are already using hashes, then the function parameter should be renamed to pw_hash or something.)

• Alexis Wilke: I made the same "mistake," but in his output the password looks like a hash...
• Jonny Whatshisface: Thank you for the input. I completely overlooked that concept... Also, I changed up a bit and ended up discovering snprintf() which I used to also determine the amount of memory to malloc... Out of curiosity, I noticed that you malloc 2 * the length + 1. I know the +1 is to account for the nul at the end, but is the multiplier of 2 actually required consider the char *escaped_username indicates you're malloc'ing a char? Or would I actually be better off doing a malloc(sizeof(char) * username_len +1); ???
• 200_success: The 2 * strlen(input) + 1 requirement is from the mysql_real_escape_string() documentation.

Answer 3

You are using strncat() the wrong way. It is a very treacherous function. I have no clue why it was done that way, but you probably won't even believe what I tell you here:

The size is NOT the total size of the string, but the space left!

From the manual page:

If src contains n or more bytes, strncat() writes n+1 bytes to dest (n from src plus the terminating null byte). Therefore, the size of dest must be at least strlen(dest)+n+1.

If you are to deal with many strings and do copies, concatenate, etc. I would suggest you create a small structure representing a string and functions that handle your strings. That allows you to put all the copy code in one place and avoid potential problems littering your entire program.

---

In the following test, you use strncmp():

int comparison = strncmp(password, row[0], strlen(password));

If the length of password is shorter than the length of row[0], then your comparison is wrong. Assuming your password are a SHA512 (it should at least use that encryption) then all encrypted passwords would have the same size. However, I do not see any encryption of the password, so I would imagine that the length can change. You should have:

int comparison = strcmp(password, row[0]);

Assuming that the string in row[0] is NUL terminated.

---

(削除) Side note about vnp statement "sizeof(char) is 1" is wrong. Some processor have a sizeof(char) of 2. Not very many and for sure not the main Desktop computers, but you cannot expect that size of always be 1. (削除ここまで) — see comments below

Your math is wrong:

char *querystring = malloc(strlen(query1) + strlen(username) + strlen(query2) * sizeof(char));

Here you say:

a + b + c ×ばつ d

What you meant is:

(a + b + c) ×ばつ d

Since d is 1 you do not see any difference, of course...

• vnp: Chapter and verse allowing sizeof(char) to be not 1, please. My version of Standard has 6.5.3.4:4, which says When sizeof is applied to an operand that has type char, unsigned char ,or signed char, (or a qualified version thereof) the result is 1
• Alexis Wilke: If you assume that his C compiler is 100% C99 compliant. And people who see a compiler that is very similar to C as not a C compiler unless proven to be 100% C99, then you are correct. However, most sensible people who write in C do not care much whether the compiler is 100% compliant and there are some flavor with strange sizes because the architecture is strange... stackoverflow.com/questions/2215445/… (see answer #2 about PDP).
• Alexis Wilke: That being said, it looks like sizeof(char) remains 1 even if the byte is not exactly 8 bits.
• vnp: It doesn't look like, it is.
• syb0rg: @AlexisWilke Actually, even GCC isn't 100% C99 compliant; but you should develop C in concordance with the standards despite that. If a feature isn't supported, then a bug report should be filed. The the request isn't honored, then you should be using a different compiler with more apt developers.

Answer 4

To add to other peoples input:

1. You could do without mysql_num_fields and mysql_num_rows since your SQL is always set for 1 field and 0 to 1 rows
2. You should be making sure there is no error with mysql_errno

3. Just like with PHP, you could also be using prepared statements (heres an example) to stop SQL injection attacks (I'm not going to code that for you)

    mysql_query(mysql_con,querystring);
    if (mysql_errno() != 0) {
    printf("Error #%d occurred trying to call mysql_query().\n", mysql_errno());
    return;
    }
    MYSQL_RES *result = mysql_store_result(mysql_con);
    if (mysql_errno() != 0) {
    printf("Error #%d occurred trying to call mysql_store_result().\n", mysql_errno());
    return;
    }
    MYSQL_ROW row;
    if ((row = mysql_fetch_row(result))) {
    printf("Query returned 1 results with 1 fields\n");
    // ...
    } else {
    printf("No such user... Password is invalid");
    }
   

Answer 5

(On top of the many excellent points in other answers)

Instead of piecing together the query string like this:

const char *query_head = "SELECT password FROM logins WHERE email = '";
const char *query_tail = "' LIMIT 1";
char *query = malloc(strlen(query_head) + strlen(query_tail) + escaped_username_len + 1);
sprintf(query, "%s%s%s", query_head, escaped_username, query_tail);
// ...
free(query);

How about using a format string + snprintf + a simple char[...], like this:

const char *format = "SELECT password FROM logins WHERE email = '%s' LIMIT 1";
int query_len = strlen(format) + escaped_username_len;
char query[query_len];
snprintf(query, query_len, format, escaped_username);
// ...
// no need anymore!
//free(query);

• Jonny Whatshisface: I didn't even realize I could use format strings in that manner...

Answer 6

• you should free the result when you are done with it mysql_free_result(result);

• concatenating for queries smell, instead use prepared statements and parameterized query

for example:

MYSQL_STMT *statement = mysql_stmt_init(*mysql);
char *query = "SELECT password FROM logins WHERE email = ? LIMIT 1";
mysql_stmt_prepare(statement, query, strlen(query));
MYSQL_BIND param;
memset(&param, 0, sizeof(param));
param.buffer_type = MYSQL_TYPE_STRING;
param.buffer = username;
param.length = strlen(username);
mysql_stmt_bind_param(statement, &param);
//and so on
mysql_stmt_close(statement);

• Alexis Wilke: What is bind? Is it supposed to be param? Also, how can you dereference it???
• ratchet freak: @AlexisWilke yeah mistake, and I meant &