Limit all DB drivers to executing single statements by checking for delimiter

I am going take a stab at tracking down what is going wrong here.

Some CREATE TABLE commands have have semicolons in the COMMENT. Let's see if a full test run reveals any problems.

Wuh? My install script and

php core/scripts/run-tests.sh --url http://localhost/drupal-8.0.x --all --verbose

is running just fine locally?

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -452,7 +452,7 @@ public function makeComment($comments) {
        // Flatten the array of comments.
   -    $comment = implode('; ', $comments);
   +    $comment = implode('. ', $comments);
   

   LOL. That is such a Drupal bug to have. I'm tempted to be a grammar pedant and ask if there's something other than periods that make sense, but...

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -490,6 +490,8 @@ public function makeComment($comments) {
      protected function filterComment($comment = '') {
   +    // Remove semicolons to avoid triggering multi-statement check.
   +    $comment = strtr($comment, array(';' => '.'));
        return preg_replace('/(\/\*\s*)|(\s*\*\/)/', '', $comment);
      }
   

   This.

3. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -456,7 +456,8 @@ public function prepareComment($comment, $length = NULL) {
   
   @@ -456,7 +456,8 @@ public function prepareComment($comment, $length = NULL) {
          // Add table prefixes before truncating.
          $comment = Unicode::truncate($this->connection->prefixTables($comment), $length, TRUE, TRUE);
        }
   -
   +    // Remove semicolons to avoid triggering multi-statement check.
   +    $comment = strtr($comment, array(';' => '.'));
        return $this->connection->quote($comment);
   

   And this. Seems redundant that we need to do it in both places, no?

#15 2 and 3: one is for the COMMENT in the schema install (ie, the CREATE TABLE) and the other is for the comment() method on the query class.

mpdonadio: Ah, OK. Silly DBTNG. Who wrote this thing?

Then I think it's fine. Thanks, pwolanin!

Do we need to request a manual test run against PostgreSQL?

Both PostgreSQL and SQLite. Is there a doc page somewhere that explains how we do that now? (It's kinda sad that I don't know that, being DB maintainer and all...)

+++ b/core/tests/Drupal/Tests/Core/Database/ConnectionTest.php
@@ -254,11 +254,11 @@ public function providerMakeComments() {
-        '/* Exploit * / DROP TABLE node; -- */ ',
+        '/* Exploit * / DROP TABLE node. -- */ ',
...
       array(
-        '/* Exploit DROP TABLE node; --; another comment */ ',
+        '/* Exploit DROP TABLE node. --. another comment */ ',

@@ -286,8 +286,8 @@ public function testMakeComments($expected, $comment_array) {
-      array('Exploit * / DROP TABLE node; --', 'Exploit * / DROP TABLE node; --'),
-      array('Exploit DROP TABLE node; --', 'Exploit */ DROP TABLE node; --'),
+      array('Exploit * / DROP TABLE node. --', 'Exploit * / DROP TABLE node; --'),
+      array('Exploit DROP TABLE node. --', 'Exploit */ DROP TABLE node; --'),

This looks weird - should we be testing that the exploit protection works here instead?

Running the Database group of test. SQLite is fine with this patch... Postgres is not.

Before patch

Test summary
------------

Drupal\system\Tests\Database\AlterTest                        34 passes
Drupal\system\Tests\Database\BasicSyntaxTest                  23 passes
Drupal\system\Tests\Database\CaseSensitivityTest               4 passes
Drupal\system\Tests\Database\ConnectionTest                   24 passes   1 fails
Drupal\system\Tests\Database\ConnectionUnitTest                7 passes
Drupal\system\Tests\Database\DatabaseExceptionWrapperTest      2 passes
Drupal\system\Tests\Database\DeleteTruncateTest               12 passes
Drupal\system\Tests\Database\FetchTest                        41 passes
Drupal\system\Tests\Database\InsertDefaultsTest               10 passes
Drupal\system\Tests\Database\InsertLobTest                     7 passes
Drupal\system\Tests\Database\InsertTest                       30 passes
Drupal\system\Tests\Database\InvalidDataTest                   4 passes
Drupal\system\Tests\Database\LoggingTest                      24 passes
Drupal\system\Tests\Database\MergeTest                        52 passes
Drupal\system\Tests\Database\NextIdTest                        4 passes
Drupal\system\Tests\Database\QueryTest                        15 passes
Drupal\system\Tests\Database\RangeQueryTest                    4 passes
Drupal\system\Tests\Database\RegressionTest                   15 passes
Drupal\system\Tests\Database\SchemaTest                      748 passes
Drupal\system\Tests\Database\SelectCloneTest                   4 passes
Drupal\system\Tests\Database\SelectComplexTest                94 passes
Drupal\system\Tests\Database\SelectOrderedTest                17 passes
Drupal\system\Tests\Database\SelectPagerDefaultTest           22 passes
Drupal\system\Tests\Database\SelectSubqueryTest               19 passes
Drupal\system\Tests\Database\SelectTableSortDefaultTest       32 passes
Drupal\system\Tests\Database\SelectTest                      106 passes
Drupal\system\Tests\Database\SerializeQueryTest                3 passes
Drupal\system\Tests\Database\TaggingTest                      28 passes
Drupal\system\Tests\Database\TemporaryQueryTest                7 passes
Drupal\system\Tests\Database\TransactionTest                  55 passes
Drupal\system\Tests\Database\UpdateComplexTest                35 passes
Drupal\system\Tests\Database\UpdateLobTest                     7 passes
Drupal\system\Tests\Database\UpdateTest                       35 passes

After patch

Test summary
------------

Drupal\system\Tests\Database\AlterTest                        34 passes
Drupal\system\Tests\Database\BasicSyntaxTest                  23 passes
Drupal\system\Tests\Database\CaseSensitivityTest               4 passes
Drupal\system\Tests\Database\ConnectionTest                   25 passes
Drupal\system\Tests\Database\ConnectionUnitTest                7 passes
Drupal\system\Tests\Database\DatabaseExceptionWrapperTest      2 passes
Drupal\system\Tests\Database\DeleteTruncateTest               12 passes
Drupal\system\Tests\Database\FetchTest                        41 passes
Drupal\system\Tests\Database\InsertDefaultsTest               10 passes
Drupal\system\Tests\Database\InsertLobTest                     7 passes
Drupal\system\Tests\Database\InsertTest                       30 passes
Drupal\system\Tests\Database\InvalidDataTest                   4 passes
Drupal\system\Tests\Database\LoggingTest                      24 passes
Drupal\system\Tests\Database\MergeTest                        52 passes
Drupal\system\Tests\Database\NextIdTest                        4 passes
Drupal\system\Tests\Database\QueryTest                        15 passes
Drupal\system\Tests\Database\RangeQueryTest                    4 passes
Drupal\system\Tests\Database\RegressionTest                   15 passes
Drupal\system\Tests\Database\SchemaTest                      748 passes
Drupal\system\Tests\Database\SelectCloneTest                   4 passes
Drupal\system\Tests\Database\SelectComplexTest                94 passes
Drupal\system\Tests\Database\SelectOrderedTest                17 passes
Drupal\system\Tests\Database\SelectPagerDefaultTest            0 passes             1 exceptions
Drupal\system\Tests\Database\SelectSubqueryTest               19 passes
Drupal\system\Tests\Database\SelectTableSortDefaultTest        0 passes             1 exceptions
Drupal\system\Tests\Database\SelectTest                      106 passes
Drupal\system\Tests\Database\SerializeQueryTest                3 passes
Drupal\system\Tests\Database\TaggingTest                      28 passes
Drupal\system\Tests\Database\TemporaryQueryTest                0 passes             1 exceptions
Drupal\system\Tests\Database\TransactionTest                  55 passes
Drupal\system\Tests\Database\UpdateComplexTest                35 passes
Drupal\system\Tests\Database\UpdateLobTest                     7 passes
Drupal\system\Tests\Database\UpdateTest                       35 passes

To run tests with postgres just pass run-tests.sh a valid postgres database connection url.

Isn't this change adding extra code for no reason now we've gone for #2388255: (followup) Limit PDO MySQL to executing single statements if PHP supports it and doesn't Postgres have something similar to protect a user from running mulitple sql statements?

The PDO single-statement flag was only added in a super-recent version of PHP, so there are still a lot of users that won't have that check. This is an extra check for users that don't have that flag. I cannot speak to Postgres', as I've never used it for more than 10 minutes.

Adding #2509296: PostgreSQL: Re-think current PDO::ATTR_EMULATE_PREPARES setting as related.

@pwolanin, PostgreSQL PDO limits to single statements by default, but we explicit allow multiple statements by enabling emulated prepares ;)

@pwolanin yes, you have to re-upload your patch again to enable PG testing. Unfortunately there are still many random exceptions with PG, all related to some kind of already installed exceptions that we need to fix first before getting meaningfully results ;)

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -529,7 +534,8 @@ public function makeComment($comments) {
-    return strtr($comment, ['*' => ' * ']);
+    // Change semicolons to period to avoid triggering multi-statement check.
+    return strtr($comment, ['*' => ' * ', ';' => '.']);

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -540,7 +540,8 @@ public function prepareComment($comment, $length = NULL) {
+    // Remove semicolons to avoid triggering multi-statement check.
+    $comment = strtr($comment, array(';' => '.'));

Is there any particular reason why we don't use str_replace?

The flag makes sense here, I think. Nice work. A minor nit, but otherwise I'm happy with the latest patch. (I don't know what our postgres policy is this week on whether this is RTBCable or not.)

1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -540,7 +540,8 @@ public function prepareComment($comment, $length = NULL) {
   +    // Remove semicolons to avoid triggering multi-statement check.
   +    $comment = strtr($comment, array(';' => '.'));
   

   New-style arrays, please.

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
   @@ -192,28 +192,40 @@ function initializeDatabase() {
   +      $connection = Database::getConnection();
          // Create functions.
   -      db_query('CREATE OR REPLACE FUNCTION "greatest"(numeric, numeric) RETURNS numeric AS
   +      $connection->query('CREATE OR REPLACE FUNCTION "greatest"(numeric, numeric) RETURNS numeric AS
   

   Thank you! :-)

I picked some nits.

1. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -593,6 +599,15 @@ public function query($query, array $args = array(), $options = array()) {
   +        // allowed unless the option is set.  This is needed for special cases
   

   The word "This" feels ambiguous. How about "Semicolons are needed..."

2. +++ b/core/lib/Drupal/Core/Database/Connection.php
   @@ -593,6 +599,15 @@ public function query($query, array $args = array(), $options = array()) {
   +          throw new \InvalidArgumentException('; is not supported in SQL strings.  Use only one statement at a time');
   

   Missing a closing period on the second sentence.

3. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
   @@ -192,28 +192,40 @@ function initializeDatabase() {
   +    // During tasks we may need to run complex SQL like creating functions
   

   This sentence feels incomplete and disjointed from the previous paragraph.

Per #42 this is probably as much of a Postgres review as we're going to get, so let's move on.

@crell since we have a green testbot (since some hours) let's do another retest to make sure everything works as expected before setting to RTBC.

Rerolled.

Looking at the fails I have to wonder. They are about creating the GREATEST and CONCAT functions, but we removed them (because obsolete in PostgreSQL > 9.1) in #2552769: Remove outdated custom functions for greatest() and concat() ;)

Adding #2561121: Installer fails on postgresql due to uncaught exception as related...

This seems to revert #2552769: Remove outdated custom functions for greatest() and concat(), probably a merge error or something. Marking needs work for that.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -239,6 +239,10 @@ public function destroy() {
+   * - allow_delimiter_in_query: By default, queries which have the ; delimiter
+   *   any place in them will cause an exception. In rare cases, such as

Should we document *why* this is the case or is that obvious? (I'm genuinely asking, I thought about this myself and am not sure.)

RTBC, looks great to me!

Fixing title

I'm having a hard time interpreting the conflicting DrupalCI results in #60, so reuploading that patch to get fresh, uncluttered, results.

Is that SQLite failure expected? I notice that #60 didn't have any SQLite test running?

Per #70.

I committed #2573289: Core NodeAdminTest test failing on sqlite due to unspecified node ordering so let's try this again.

Adding credit to @bzrudi71 and @Crell for multiple reviews and investigations into related issues.

Removing credit from myself. I only re-uploaded existing patches, didn't contribute anything significant of my own.

I like how this patch hardens Drupal's Database API for users without a database engine/configuration that does it at that layer. Also, this was RTBC for over a week between #68 and #69 without anyone raising a concern with doing this. I also reviewed the patch myself and no concerns jumped out at me. Therefore, committed and pushed to 8.0.x.

I think we could use a change record for module developers who create SQL functions, like the below, so "needs work" for that.

+++ b/core/lib/Drupal/Core/Database/Connection.php
@@ -239,6 +239,12 @@ public function destroy() {
+   *   additional queries (such as inserting new user accounts). In rare cases,
+   *   such as creating an SQL function, a ; is needed and can be allowed by
+   *   changing this option to TRUE.
...
+++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Install/Tasks.php
@@ -250,18 +250,23 @@ function initializeDatabase() {
-      if (!db_query("SELECT COUNT(*) FROM pg_proc WHERE proname = 'rand'")->fetchField()) {
-        db_query('CREATE OR REPLACE FUNCTION "rand"() RETURNS float AS
+      if (!$connection->query("SELECT COUNT(*) FROM pg_proc WHERE proname = 'rand'")->fetchField()) {
+        $connection->query('CREATE OR REPLACE FUNCTION "rand"() RETURNS float AS
           \'SELECT random();\'
-          LANGUAGE \'sql\''
+          LANGUAGE \'sql\'',
+          [],
+          [ 'allow_delimiter_in_query' => TRUE ]
         );

Change record looks good to me - looking forward to this in core

Thanks. I published the CR, so moving this to the 7.x queue.

Just bumping this as this would be good to get into 7.x

Regarding Drupal 8. Warning, this blows up a statement like:

\Drupal::database()->query('SELECT n.nid FROM {node} as n LEFT JOIN {key_value} as kv ON n.nid=kv.name WHERE kv.collection="pathauto_state.node" AND kv.value="i:0;"')

NOTICE: PHP message: Uncaught PHP Exception PDOException: "SQLSTATE[HY093]: Invalid parameter number: no parameters were bound" at /var/www/html/web/core/lib/Drupal/Core/Database/Statement.php

And while that's easy enough to work around by adding placeholders even though the offending query parameters aren't based on user input:

$nids = \Drupal::database()->query("SELECT n.nid FROM {node} as n LEFT JOIN {key_value} as kv ON n.nid=kv.name WHERE kv.collection='pathauto_state.node' AND kv.value=:val",
  [':val' => 'i:0;'])->fetchCol(); 

If you're trying to do an UPDATE query with a join i think you're straight out of luck.

This was committed.

Restoring credits.