Efficiency with strcpy, strcat and malloc

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) {

char *query2 = "' LIMIT 1";

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

strncpy(querystring,query1,strlen(query1));
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);

if (comparison == 0) {
} else {
}

}
else {
printf("No such user... Password is invalid");
}
free(querystring);
}


Output of program:

Checking password for test@blah.com - pw: 5f4dcc3b5aa765d61d8327deb882cf99
Query string: SELECT password FROM logins WHERE email = 'test@blah.com' LIMIT 1
Query returned 1 results with 1 fields

-
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. –  Alexis Wilke Aug 30 '14 at 0:02

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?

-

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);
);

const char *query_tail = "' LIMIT 1";

/* 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.)

-
I made the same "mistake," but in his output the password looks like a hash... –  Alexis Wilke Aug 29 '14 at 23:17
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); ??? –  Jonny Whatshisface Aug 29 '14 at 23:31
The 2 * strlen(input) + 1 requirement is from the mysql_real_escape_string() documentation. –  200_success Aug 29 '14 at 23:53

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

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...

-
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 –  vnp Aug 29 '14 at 23:04
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 Aug 29 '14 at 23:14
That being said, it looks like sizeof(char) remains 1 even if the byte is not exactly 8 bits. –  Alexis Wilke Aug 29 '14 at 23:16
It doesn't look like, it is. –  vnp Aug 29 '14 at 23:18
@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. –  syb0rg Aug 30 '14 at 0:40

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");
}

-

(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";
// ...
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];
// ...
// no need anymore!
//free(query);

-
I didn't even realize I could use format strings in that manner... –  Jonny Whatshisface Aug 29 '14 at 23:45

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;

What is bind? Is it supposed to be param? Also, how can you dereference it??? –  Alexis Wilke Aug 30 '14 at 19:50
@AlexisWilke yeah mistake, and I meant & –  ratchet freak Aug 30 '14 at 20:04