Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

share|improve this question
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
share|improve this answer

A few points of interest:

  • This dead statement, even though probably eliminated by the compiler, worries me:

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

    int a is declared inside the if scope. But you might have the idea from a quick glance at the code that it is a local variable. This is one reason why always adding curly braces can improve readability.

    Also, you are assigning NULL to an integer. This will compile in C++ because NULL is just a #define for zero, but it looks very strange in the code, since NULL is associated with pointers.

  • MySQL_Database_Connection__global should be a function parameter. This would make you code more reusable and also easier to debug. If you write a standalone string escaping function that doesn't rely on a specific global name, you can reuse it in other projects that have the same need.

  • If you are going to return at the end of an if block, don't add the corresponding else. This just complicates logic for no good reason.

  • You should prefer using std::string for text manipulation in C++. Since your escapeSQL() seems to be a C++ wrapper to a lower-level function that deals with C strings, it should receive and return a C++ string.

  • return is not a function call, so it's parameter shouldn't be enclosed by parenthesis.

  • As was pointed out by a previous review, this:

    std::string to(((dataInLen * 2) + 1), '\0');

    will produce a string with length() always equal to dataInLen * 2 + 1, even when mysql_real_escape_string() writes less chars than that to the string. To fix this, I would create a temporary char buffer and pass it to the function, then return it. There might be an even cleaner solution, but can't think of one right now.


This is how I would refactor your function with the changes above applied to it:

std::string escapeSQL(SQLConnection & connection, const std::string & dataIn)
{
    // This might be better as an assertion or exception, if an empty string
    // is considered an error. Depends on your requirements for this function.
    if (dataIn.empty())
    {
        return "";
    }

    const std::size_t dataInLen = dataIn.length();
    std::vector<char> temp((dataInLen * 2) + 1, '\0'); 
    mysql_real_escape_string(&connection, temp.data(), dataIn.c_str(), dataInLen);

    return temp.data();
    // Will create a new string but the compiler is likely 
    // to optimize this to a cheap move operation (C++11).
}

Note: I don't know what type MySQL_Database_Connection__global has, so I have mocked it with a fake SQLConnection type.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.