3
\$\begingroup\$

Further to my previous post for a user login script I've been learning PDO in order to migrate from mysql_* functions. As before with mysql_*, I wanted to wrap the PDO code into wrapper functions, so this is what I've done here.

I've created two functions:

  1. db_connect(), which creates a connection and will be called at the start of each script
  2. db_single_prepared_query(), which will run a single, prepared query, supporting just the four CRUD operations. I'll create more functions for other uses as required.

Since I'm new to PDO I would like feedback on a) any errors in my general usage of PDO and b) the helper functions:

function db_connect() 
    {
    // Define connection as a static variable, to avoid connecting more than once
    static $dbh;

    // Try and connect to the database, if a connection has not been established yet
    if(!isset($dbh)) 
        {
        try 
            {
            $dsn = 'mysql:dbname=' . MYSQL_DBNAME . ';host=' . MYSQL_HOSTNAME . ';charset=utf8';
            $dbh = new PDO($dsn, MYSQL_USERNAME, MYSQL_PASSWORD);
            $dbh->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);//which tells PDO to disable emulated prepared statements and use real prepared statements. This makes sure the statement and the values aren't parsed by PHP before sending it to the MySQL server (giving a possible attacker no chance to inject malicious SQL).
            $dbh->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            } 
        catch(PDOException $e)
            {
            print "Cannot connect to database<P>";
            log_this("db_connect - " . $e->getMessage(), "error.log");
            exit();

            }
        }

    return $dbh;
    }

db_single_prepared_query() which calls db_connect(), which only connects once per script life:.

function db_single_prepared_query($query, $parameters=array())
    {
    // see what type of query this is, use this later to customise return values
    if (preg_match('/^ *(select|update|insert|delete)/i', $query, $matches)) 
        {
        $crud_op = strtoupper($matches[1]);
        }
    else
        {
        print "Database query failed<P>";
        log_this("db_single_prepared_query_exec() - could not determine CRUD op for " . $query . "\n"  , "error.log");
        exit();
        }


    // re-connect with static $dhb inside of db_connect(), thus allowing us to call this function [db_single_prepared_query()]
    // from within another function without having to worry about passing $dbh
    $dbh = db_connect();

    try 
        {

        $stmt = $dbh->prepare($query);

        for ($i=0;$i<sizeof($parameters);$i++) 
            {
            $stmt->bindValue($parameters[$i]['label'], $parameters[$i]['value'], $parameters[$i]['type']);
            }

        $stmt->execute();

        $ret = array();

        switch ($crud_op) 
            {
            case "SELECT":
                // rowCount not reliable for select http://php.net/manual/en/pdostatement.rowcount.php
                // but http://wiki.hashphp.org/PDO_Tutorial_for_MySQL_Developers says OK to use?? trying both for comparison
                $rows = $stmt->fetchAll(PDO::FETCH_ASSOC);
                $ret['ROWS'] = $rows;
                $ret['ROWCOUNT'] = sizeof($rows);
                $ret['ROWCOUNTvia_rowCount'] = $stmt->rowCount();
                break;
            case "INSERT":
                $ret['ROWCOUNT'] = $stmt->rowCount();
                $ret['LASTID'] = $dbh->lastInsertId();;
                break;
            case "UPDATE":
            case "DELETE":
                $ret['ROWCOUNT'] = $stmt->rowCount();
            }
        }
    catch(PDOException $e)
        {
        print "Database query failed<P>";
        log_this("db_single_prepared_query_exec() - " . $query . "\n" .  $e->getMessage() , "error.log");
        exit();
        }
    return $ret;
    }

log_this() is another wrapper which writes to a log file in a safe location.

Usage:

$query = " select username, ext_email from users where college_id=:cid and user_id<:maxuid";


$cid=4;
$maxuid=10000;

$parameters = array(
    array('label' => ':cid', 'value'=> $cid, 'type' => PDO::PARAM_INT),
    array('label' => ':maxuid', 'value'=> $maxuid, 'type' => PDO::PARAM_INT)
    );
$info = db_single_prepared_query($query, $parameters);

print "<pre>\n";
print_r($info);
print "</pre>\n";

Output of mysald log showing multiple queries only connect once:

150304 16:52:52       125 Connect     development@localhost on xxxxx
          125 Prepare     [1] select username, ext_email from users where college_id=? and user_id<?
          125 Execute     [1] select username, ext_email from users where college_id=4 and user_id<10000
          125 Prepare     [2] select username, ext_email from users where college_id=? and user_id<?
          125 Execute     [2] select username, ext_email from users where college_id=4 and user_id<10000
          125 Prepare     [3] select username, ext_email from users where college_id=? and user_id<?
          125 Execute     [3] select username, ext_email from users where college_id=4 and user_id<10000
          125 Prepare     [4] select username, ext_email from users where college_id=? and user_id<?
          125 Execute     [4] select username, ext_email from users where college_id=4 and user_id<10000
          125 Quit

Research links:

And of course the PHP docs

Questions

  1. In the bindValue() data_type such as PDO::PARAM_INT, if I supply that but bind a string to it like so:

    $cid="4we";
        $maxuid=10000;
    
        $parameters = array(
            array('label' => ':cid', 'value'=> $cid, 'type' => PDO::PARAM_INT),
            array('label' => ':maxuid', 'value'=> $maxuid, 'type' => PDO::PARAM_INT)
            );
        $info = db_single_prepared_query($query, $parameters);
    

    This is sent to the server and returns rows. Am I missing something? I would have thought the point of using prepared statements, and specifying data types to bind, is to reject things like passing a string in place of an int.

        150304 17:04:01     126 Connect     development@localhost on obrienApp
                126 Prepare     [1] select username, ext_email from users where college_id=? and user_id<?
                126 Execute     [1] select username, ext_email from users where college_id='4we' and user_id<10000
                126 Quit
    
  2. Is $stmt->rowCount(); reliable for MySQL?

  3. Do I really not need to worry about my incoming data? Is it really safe from injection?

\$\endgroup\$
  • \$\begingroup\$ Regarding your comment about SQL injection attacks. They are still possible if you use some less used charsets other than UTF-8. \$\endgroup\$ – AnotherGuy Mar 4 '15 at 21:09
  • \$\begingroup\$ That's what I thought. I will still validate inputs. \$\endgroup\$ – user602088 Mar 4 '15 at 22:07
2
\$\begingroup\$

Here are some of my observations. This is code review so the observations are about your code, but I will also answer your questions along the way.

1: Functions, not classes. Nothing wrong with that, just unexpected. Classes are better at compartiamentilizing names. They are the preferred choice for most programming tasks, but functions will do just fine in this case.

2: Wrapping means restricting yourself. But you restrict yourself unnecessarily. You can only ever have one database connection with these functions. This might seem fine now, but in the future you might want to connect to more than one database. Yes, you will. And defining the database access parameters as constants has the same problem.

3: In virtually all cases the third parameter of bindValue() is not needed. Given what you try to do, you could easily leave it out. This would simplify your code quite a lot. To change your example:

 $parameters = array(':cid'    => $cid, 
                     ':maxuid' => $maxuid);

 $info = db_single_prepared_query($query, $parameters);

and this will work just fine. I would also leave the : out of the parameter keys and add it later, but it can be argued it should be there.

4: I would not use rowCount() for SELECT, since it might not always give the correct result. You have count($rows), so why use it?

5: Errors will occur when accessing a database. For now it's fine to just through a wobbler and exit with an error message. This is not acceptable in a production environment. Errors should be handled properly, or at least your function should give the user the ability to do so.

6: All incoming data should be treated with the utmost suspicion. The fact that you bind might help to prevent SQL injection in these functions, but lots of other types of attacks exist. Hackers will try to change the input and see what happens. All input should therefore be filtered and clear restrictions on their values should always be imposed.

\$\endgroup\$
  • \$\begingroup\$ Thank you for your response. Re 1) I've not taken the step to OOP as yet, so will carry on with functions if that seems ok. 2) this is for a web site and I define the constants in a config file (not shown above) so each site has its own config and d/b, thus far in all the sites I've done I have not needed to access more than one d/b. 3) I'll play with leaving it out but still confused as to why it accepts wrong types, my Q1. Perhaps a questions for stackoverflow. 4) I'll look at that, thanks \$\endgroup\$ – user602088 Mar 4 '15 at 22:01
  • \$\begingroup\$ Cont. 5) my mysqld is on localhost and is rock solid, but yes problems can occur. I thought froma security POV one should not give too detailed errors to a user, and in any case how woould a web site visitor correct an error? Any errors are logged to a file and emailed to me. 6) I'm happy you confirmed my suspicion that prepared SQL is not the silver bullet for attacks, I will continue to filter (again not shown here for clarity) as I do with mysql_*. Thanks again! \$\endgroup\$ – user602088 Mar 4 '15 at 22:07
  • \$\begingroup\$ You're welcome. All documentation I can find on binding types states that it doesn't do much in PDO. I've never bothered with them. When a database error occurs, say a user tries to register, but the database is down, it would be nice if the user got the message: "Sorry we have a technical problem. We cannot process your registration.", instead of "Database query failed". That's what I mean by 'properly handled'. \$\endgroup\$ – KIKO Software Mar 4 '15 at 22:15
  • \$\begingroup\$ I've changed my error text to read "Sorry, there is temporary technical problem loading this page. We have been informed of this issue and will correct it ASAP. (DBQ)" where "DBQ" is a code that indicates to me where it failed, should somone email it to me. Re rowCount(), I do: $rows = $stmt->fetchAll(PDO::FETCH_ASSOC); then $ret['ROWCOUNT'] = sizeof($rows); - when you say count($rows) is this the same thing (if you mean PHP array functions, sizeof() is an alias of: count(). ) \$\endgroup\$ – user602088 Mar 5 '15 at 12:09
  • \$\begingroup\$ Re binding: leaving out the type results in -- where college_id='10' -- being run by mysqld, when I pass $college_id as a parameter which is an INT. It works OK, but just seems wrong to me that an int gets quoted by PDO \$\endgroup\$ – user602088 Mar 5 '15 at 12:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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