HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

My method of escaping MySQL strings in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
mysqlstringsescapingmethod

Problem

This is how I'm currently escaping MySQL strings in C++:

std::string escapeSQL(const char* dataIn)
{
   if (!MySQL_Database_Connection__global->host)
      int a = NULL;

   if (dataIn)
   {
      std::size_t dataInLen = strlen(dataIn);
      std::string to(((dataInLen * 2) + 1), '\0');
      mysql_real_escape_string(MySQL_Database_Connection__global, &to[0], dataIn, dataInLen);
      return (to);
   }
   else
   {
      return ("");
   }
}

char query[1024] = {'\0'};
snprintf(query, sizeof(query), "SELECT * FROM database.table WHERE column = '%s';", escapeSQL(userTypedArgument).c_str());


It seems to be doing the job, but I'm curious if I can get a review of my escapeSQL() function for any pitfalls I may have overlooked.

Solution

if (!MySQL_Database_Connection__global->host)
    int a = NULL;


What is the purpose of this if statement? It only contains one unused declaration.

if (dataIn) ... return ("");


I try to avoid adding defensive behavior like this. Returning empty string for NULL argument seems like a good idea and might even save your program from crashing. But, NULL argument indicates your program is already in invalid state and it's unlikely to function properly. Assertion or exception with clear indication what's wrong would save you a lot of debugging.

std::size_t dataInLen = strlen(dataIn);
escapeSQL(userTypedArgument).c_str()


Consider using just std::string or just C strings. Or at least minimize those awkward conversions back and forth. They can only lead to bugs like this one:

mysql_real_escape_string(MySQL_Database_Connection__global, &to[0], dataIn, dataInLen);


You are creating null-terminated C string inside std::string. std::string ignores null characters. to.length() will always return dataInLen*2+1. Any method other than c_str() will result in unexpected behavior.

char query[1024] = {'\0'};
snprintf(query, sizeof(query), "SELECT * FROM database.table WHERE column = '%s';", escapeSQL(userTypedArgument).c_str());


This is exploitable. You should check if the query exceeds 1023 characters and fail in a safe way (or just use std::string). Currently user can remove part of your query by entering a long string. For example:

"DELETE * FROM table WHERE name='long string ...' AND owner=currentUser"
                                                  ^ 1024th character

Code Snippets

if (!MySQL_Database_Connection__global->host)
    int a = NULL;
if (dataIn) ... return ("");
std::size_t dataInLen = strlen(dataIn);
escapeSQL(userTypedArgument).c_str()
mysql_real_escape_string(MySQL_Database_Connection__global, &to[0], dataIn, dataInLen);
char query[1024] = {'\0'};
snprintf(query, sizeof(query), "SELECT * FROM database.table WHERE column = '%s';", escapeSQL(userTypedArgument).c_str());

Context

StackExchange Code Review Q#71932, answer score: 4

Revisions (0)

No revisions yet.