Beefy Boxes and Bandwidth Generously Provided by pair Networks vroom
No such thing as a small change
 
PerlMonks  

drawbacks to 'eval' parameters/placeholders/binding in DBI calls to mysql database

by nextguru (Scribe)
on Aug 20, 2009 at 21:36 UTC ( #790190=perlquestion: print w/ replies, xml ) Need Help??
nextguru has asked for the wisdom of the Perl Monks concerning the following question:

Background story:
I have created a working test case that will lead to a generic update routine for a mysql database having multiple tables. The tables have different structures and being Lazy I didn't want to hard code the column names so I could reuse the code for all tables. This would allow passing the eventual subroutine a table name and hash of values to update a single record. The subroutine is intended for use in a web based application, so I am using parameter binding to avoid SQL injection hacks. On a lark I tried 'eval' of a string containing the names of the variables to insert for the parameters and to my pleasant surprise it worked.

Question: Are there any drawbacks to this method of binding? If so, what techniques should I use instead?

thanks for any pointers
- nextguru (that's NeXT guru, not the next guru)

#!/usr/local/bin/perl -w use strict; use DBI; my $username = 'xxxxxx'; my $password = 'xxxxxx'; my $host = 'localhost'; my $database = 'xxxxxx_innoTest'; my $tableName = 'members'; my %memberRecord; $memberRecord{'nameLogin'} = 'testloginname; DROP TABLE members;'; $memberRecord{'password'} = 'testpwd'; $memberRecord{'nameFirst'} = 'testfirstname'; $memberRecord{'nameLast'} = 'testlastname'; $memberRecord{'email'} = 'testemail@mycompany.com'; my $columnNames; my $columnBindings; my $ToBeEvaled; foreach my $keyName ( keys %memberRecord ) { if (not $columnNames) { $columnNames = $keyName; $columnBindings = '?'; $ToBeEvaled = "\$memberRecord{'".$keyName."'}"; } else { $columnNames = join( ', ', $columnNames, $keyName ); $columnBindings = join( ', ', $columnBindings, '?' ); $ToBeEvaled = join(', ', $ToBeEvaled, "\$memberRecord{'".$keyN +ame."'}" ); } } my $sqlStatement = sprintf("INSERT INTO %s (%s) VALUES (%s);", $tableN +ame, $columnNames, $columnBindings); my $dbh = DBI->connect( "DBI:mysql:database=$database;host=$host", $us +ername, $password ) or die $DBI::errstr; my $sth = $dbh->prepare( $sqlStatement ) or die $DBI::errstr; $sth->execute( eval $ToBeEvaled ) or die $DBI::errstr; $sth->finish(); $dbh->disconnect() or warn "Disconnection failed: $DBI::errstr\n"; exit;

Comment on drawbacks to 'eval' parameters/placeholders/binding in DBI calls to mysql database
Download Code
Re: drawbacks to 'eval' parameters/placeholders/binding in DBI calls to mysql database
by JavaFan (Canon) on Aug 20, 2009 at 21:49 UTC
    Seems overly complicated.

    Why not something like:

    my %memberRecord; ... Populate %memberRecord ...; my @columns = keys %memberRecord; my @values = values %memberRecord; my $sqlStatement = do { local $" = ", "; my @qms = ('?') x @columns; "INSERT INTO $tableName (@columns) VALUES (@qms)"; }; ... my $sth = $dbh->prepare($sqlStatement); $sth->execute(@values);
      Yes! I knew there was an easier way to do it in a generic way. Thanks.
Re: drawbacks to 'eval' parameters/placeholders/binding in DBI calls to mysql database
by Tanktalus (Canon) on Aug 20, 2009 at 21:59 UTC

    Drawback: ever having a special character in the key name. Heaven forbid you end up with a key name with a single quote in it. (Which then leads to Bobby Tables problems.) Granted, that might cause other SQL problems, but in the general case, it's not unreasonable.

    Just push each value into an array, and pass that in. Cheaper, easier to read, easier to maintain, easier to modify (grep, map, whatever), faster, fewer exploits/hard-to-find bugs. There's a reason why we say eval STRING is evil. It's rare that it's the right tool for the job, but it so easily can fit into so many jobs.

      The solution is the appropriate use of quotemeta (aka \Q..\E) for eval and $dbh->quote_identifier for prepare.

      The OP's code is needless complicated as JavaFan pointed out, so I'll apply the solution to his code:

      my %memberRecord = ...; my @cols = keys %memberRecord; my @vals = values %memberRecord; my $stmt = do { local $" = ", "; my @q_cols = map $dbh->quote_identifier($_), @cols; my @params = ('?') x @cols; "INSERT INTO $tableName (@q_cols) VALUES (@params)" }; my $sth = $dbh->prepare($stmt); $sth->execute(@vals);
        Thanks. A quick test confirms that at least a single quote in a column name is covered with the proposed fix. I will investigate more, but I think I'm on the right track now thanks to all.
      Thanks for the info. I was uniformed about the 'eval STRING' being evil, will have to research a bit more.
Re: drawbacks to 'eval' parameters/placeholders/binding in DBI calls to mysql database
by ikegami (Pope) on Aug 20, 2009 at 22:41 UTC

    I didn't realize how bad your code was when I wrote my earlier reply. I'm not trying to rude. I think you'll agree with me after you see what follows.

    Your bad and buggy way of building an array:

    sub to_literal { "'$_[0]'" } my $x; for (...) { if (!$x) { $x = to_literal($_); } else { $x = join(', ', to_literal($_)); } } my @a = eval $x;

    Still bad, but bugs removed:

    sub to_literal { "'\Q$_[0]\E'" } <--- my $x; for (...) { if (!defined($x)) { <--- $x = to_literal($_); } else { $x = join(', ', to_literal($_)); } } my @a = eval $x;

    A good way of building an array:

    my @a; for (...) { push @a, $_; }

    Bonus: It doesn't stringify everything.

    Useful functions: push, pop, shift, unshift, splice

    Update: I initially had the loops flattened, but I thought that would cause confusion.
    Update: Added links.

      I coded the test routine very late last night and couldn't seem to write this in a simple way (think of forest/trees analogy). This is a much clearer (and now quite obvious) method. Thanks for the help.
Re: drawbacks to 'eval' parameters/placeholders/binding in DBI calls to mysql database
by Util (Priest) on Aug 22, 2009 at 04:01 UTC

    Here is my solution using placeholders. I added framing code to make it easier for other monks to test; all you need is SQLite, the cross-platform DB in a single executable.

    Working, tested code:

    #!/usr/bin/perl use strict; use warnings; use DBI; my $username = 'xxxxxx'; my $password = 'xxxxxx'; my $host = 'localhost'; my $database = 'xxxxxx_innoTest'; my $tableName = 'members'; my $data_source = "DBI:mysql:database=$database;host=$host"; # Overriding for local testing with SQLite. my $db_file = 'pm790190.sqlite'; sub sqlite { system( sqlite3 => '-line', $db_file, @_ )==0 or warn; } { unlink $db_file if -e $db_file; sqlite(<<'END_OF_SQL'); CREATE TABLE members ( nameLogin char(50), password char( 8), nameFirst char(20), nameLast char(20), email char(30) ); END_OF_SQL $data_source = "dbi:SQLite:dbname=$db_file"; } my %memberRecord = ( nameLogin => 'testloginname; DROP TABLE members;', password => 'testpwd', nameFirst => 'testfirstname', nameLast => 'testlastname', email => 'testemail@mycompany.com', ); my @column_names = sort keys %memberRecord; my @column_values = map { $memberRecord{$_} } @column_names; my @placeholders = map { '?' } @column_names; my $column_name_list = join ', ', @column_names; my $placeholder_list = join ', ', @placeholders; my $sqlStatement = <<"END_OF_SQL"; INSERT INTO $tableName ($column_name_list) VALUES ($placeholder_list) END_OF_SQL my $dbh = DBI->connect( $data_source, $username, $password ) or die $DBI::errstr; my $sth = $dbh->prepare( $sqlStatement ) or die $dbh->errstr; $sth->execute(@column_values) or die $dbh->errstr; $sth->finish(); undef $sth; $dbh->disconnect() or warn "Disconnection failed: $DBI::errstr\n"; # Checking results of local testing with SQLite. sqlite('SELECT * FROM members');
    Output:
    nameLogin = testloginname; DROP TABLE members; password = testpwd nameFirst = testfirstname nameLast = testlastname email = testemail@mycompany.com

Log In?
Username:
Password:

What's my password?
Create A New User
Node Status?
node history
Node Type: perlquestion [id://790190]
Front-paged by Arunbear
help
Chatterbox?
and the web crawler heard nothing...

How do I use this? | Other CB clients
Other Users?
Others imbibing at the Monastery: (6)
As of 2014-04-20 20:04 GMT
Sections?
Information?
Find Nodes?
Leftovers?
    Voting Booth?

    April first is:







    Results (487 votes), past polls