2

I'm having trouble with this snippet of code, and can't find any errors:

$query = "CREATE TABLE ? (? INT NOT NULL AUTO_INCREMENT, PRIMARY KEY(?), ? VARCHAR(30), ? VARCHAR(50), ? TIMESTAMP, ? VARCHAR(50), ? DECIMAL(15, 2), ? DECIMAL(3, 2), ? VARCHAR(255))";
$array = array($table_name, $id, $id, $a_title, $c_title, $date_updated_title, $s_title, $ds_title, $ps_title, $u_title);

    try {
        $results = db_query($db, $query, $array); // db_query() is my PDO function to query the database. This function works fine elsewhere.
        echo($table_name . " create successfully!");
    } catch (PDOException $e) {
        echo('<br />Could not create table "' . $table_name . '".');
        return false;
        error($e); //error() is my function to write errors to my log, and works fine elsewhere.
    }

When I run this in my browser, it returns my caught exception 'Could not create table "name".' However, I don't see any error in my log, so I don't know if it's a syntax issue, or what.

When I take the query itself, and replace the question marks with the actual values, and dump it in PHPMyAdmin, it creates the table fine. I'm not really sure what the issue is here. I've had reasonable success with PDO on another site, but I'm still relatively new. Any ideas?

Thanks for the help!

[Edit] I've since tried using this query:

"CREATE TABLE $a_title (? INT NOT NULL AUTO_INCREMENT, PRIMARY KEY(?), ? VARCHAR(30), ? VARCHAR(50), ? TIMESTAMP, ? VARCHAR(50), ? DECIMAL(15, 2), ? DECIMAL(3, 2), ? VARCHAR(255))";

I've tried with both single and double quotes. I also removed the $table_name variable from the array. Still getting a syntax error, and not sure why.

aikorei
  • 570
  • 1
  • 7
  • 23
  • 5
    `CREATE TABLE ?` <= uh-uh. Give it a name or use a variable. You can't bind tables. – Funk Forty Niner Jun 26 '14 at 17:34
  • 3
    Rule of thumb: would this query work if you replaced the placeholder with a literal *string* `'hello'` or *number* `42`? If not, then you cannot use a placeholder there. (And keep in mind, a *string* `'hello'` is a completely different beast than an *identifier* (table name, column name, index name, etc...)) – DCoder Jun 26 '14 at 17:35
  • 1
    Add `$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);` right after the connection is opened, if you're not already doing so. – Funk Forty Niner Jun 26 '14 at 17:36
  • 1
    Writing a wrapper around PDO for the sake of convenience is usually a bad idea. PDO itself isn't very complicated so I'm not sure why you'd need to abstract that away, especially when it probably limits how you can use PDO. – tadman Jun 26 '14 at 17:38
  • 1
    Assuming that this is part of a method or function, you will not see any errors in your log as you have a `return` statement right before you try to log it. – jeroen Jun 26 '14 at 17:38
  • 1
    *...and that,* is your PDO lesson for the day ;-) – Funk Forty Niner Jun 26 '14 at 17:40
  • Wow, thanks for the feedback, everyone (and for reminding me I'm still new at this). @jeroen I should have seen that (duh...no wonder no errors logged). – aikorei Jun 26 '14 at 17:42
  • @tadman I don't think I understand what you mean. Any chance you could explain a bit more? – aikorei Jun 26 '14 at 17:45
  • @fred -ii- thanks for explaining that. I didn't realize....still not working as expected, though (same issues, though now I'm actually getting a syntax error for the query). – aikorei Jun 26 '14 at 17:45
  • You're welcome. Update your question with your newly-tried attempt. – Funk Forty Niner Jun 26 '14 at 17:47
  • @aikorei You're still trying to use placeholders for identifiers, here column names and such. Have a look at Bill Karwins answer. – VMai Jun 26 '14 at 18:22
  • @aikorei Just use PDO directly, keep how it's used exposed and obvious. If you want a cleaner approach, use a [development framework](http://codegeekz.com/best-php-frameworks-for-developers/) like [Laravel](http://laravel.com/) which abstracts this for you in a way that's tested and field-proven. – tadman Jun 26 '14 at 18:38
  • As per your edit `CREATE TABLE '$a_title'` you would need to remove the quotes, or use backticks around the variable. – Funk Forty Niner Jun 26 '14 at 18:43

1 Answers1

3

Comment from @DCoder is correct. You can use a query parameter only in place where you could normally put a single string literal, date literal, or numeric literal.

You can't use a query parameter for:

  • Table names

    WRONG: SELECT * FROM ?

  • Column names

    WRONG: SELECT * FROM table WHERE ? = 1234

  • Lists of values

    WRONG: SELECT * FROM table WHERE column IN (?)

    Though you could use IN() with a list of parameter placeholders, one for each scalar value.

  • SQL operators, expressions, or keywords

    WRONG: SELECT * FROM table WHERE column ? 'value' AND ? ORDER BY column ?

For those cases, if you want dynamic content to become part of your query, the content must be part of the query before you call prepare().

But this means that you're back to interpolating variables into SQL query strings, which we are told is a no-no for its SQL injection risk.

The solution is to use filtering and whitelisting to make sure that the content doesn't contain some unsafe content. For example, if it's a dynamic table name, strip out anything but characters you know you want to keep, and then also delimit the table name just in case someone names their table a reserved word like "table" or "order" or something.

$table = preg_replace("/[^\w]/", "", $table);
$sql = "CREATE TABLE `{$table}` ( ... )";

Re your comment:

Yes, column names are off limits as well. As I said at the top, parameters are only for scalar values.

You also need to learn the appropriate usage of the three different types of quote marks.

Community
  • 1
  • 1
Bill Karwin
  • 538,548
  • 86
  • 673
  • 828
  • thanks, that helps a lot. Apparently column names are off limits as well (I didn't realize that at first). – aikorei Jun 26 '14 at 18:31