InsertQuery_pgsql uses PDO's bindParam when using a fromQuery with parameters, the code looks like:

    if (!empty($this->fromQuery)) {
      foreach ($this->fromQuery->getArguments() as $key => $value) {
        $stmt->bindParam($key, $value);
      }
    }

The problem is that the second parameter to bindParam is passed by reference and the foreach reuses the reference for $value. The same reference is therefore stored for every bound parameter, leaving them all with the same value at execution time.

I believe that this is a critical bug since it breaks the INSERT FROM functionality of DBTNG (unfortunately on Postgres specifically).

The attached patch reworks the foreach so that the reference issued to bindParam is specific to each value.

CommentFileSizeAuthor
#10 845846-include-test.patch3.91 KBStevel
#9 845846-include-test.patch5.06 KBStevel
#8 845846-use-direct-reference.patch2.77 KBStevel
#6 845846-arguments-variable.patch2.58 KBStevel
#4 845846-give-reference-to-bindParam.patch2.55 KBStevel
pgsql.InsertForm.patch697 bytesbellHead
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

Crell CreditAttribution: Crell commented
Component: database system » postgresql database

Refiling for the Postgres team to look into.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

This is indeed a bug. Nice catch, but we need to go a few steps further on this:

  • Keep the array on the same line, there is no need for the intermediary variable (PDO will not override the value, and even if it does, getArguments() return a value.
  • We need an inline comment before the construct to explain why the pass-by-reference.
  • There are at least two cases where we are using the same faulty pattern.
  • foreach($array as $k=>&$v) {} is dangerous because it leaks the reference out of the loop. I would prefer using $array[$k] directly instead of messing up with references even more.

For the record: PDO--

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Title: Last bound parameter value overwrites others when using a fromQuery in InsertQuery_pgsql » PDOStatement::bindParam() bind to a reference even for OUT parameters

How, and I would like to know which tests (if any) fails because of that. If none, we need more tests.

Stevel’s picture

Stevel CreditAttribution: Stevel commented
FileSize
845846-give-reference-to-bindParam.patch2.55 KB

Here's a new patch:

Comments for #2:
1) No extra variable introduced
2) An inline comment is provided the first time the foreach with reference is used
3) I only found one extra case where this is used.
4) The foreach ($array as $k =>&$v) is used several times in the pgsql query.inc, so I don't think it's that big a problem.

As tests are concerned, there's a modified Insert test that should fail on pgsql now without the rest of the patch. I'm not sure how to test the second instance though.

Status: Needs review » Needs work

The last submitted patch, 845846-give-reference-to-bindParam.patch, failed testing.

Stevel’s picture

Stevel CreditAttribution: Stevel commented
Status: Needs work » Needs review
FileSize
845846-arguments-variable.patch2.58 KB

Reroll, it appears we need to store the arguments in a separate variable after all: above patch gives 

PHP Fatal error:  Cannot create references to elements of a temporary array expression in ./includes/database/pgsql/query.inc on line 51
Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

Let's please use direct references instead of pass-by-reference. I'm very uncomfortable with it, because:

$array = array(1, 2, 3);

foreach ($array as &$value) {
  $value += 1;
}
// $array contains array(2, 3, 4)

foreach ($array as $value) {
  $value = 1;
}
// $array contains array(2, 3, 1)

So let's use an intermediary variable and direct reference to the element:

$values = ...;
foreach ($values as $column => $value) {
  $this->bindParam($column, $values[$column]);
}
Stevel’s picture

Stevel CreditAttribution: Stevel commented
FileSize
845846-use-direct-reference.patch2.77 KB

This patch removes all occurences of the pass by reference using &$variable

Stevel’s picture

Stevel CreditAttribution: Stevel commented
FileSize
845846-include-test.patch5.06 KB

Forgot to add the modified test in the latest patches.

Stevel’s picture

Stevel CreditAttribution: Stevel commented
FileSize
845846-include-test.patch3.91 KB

d.o does not like the ANSI coloring (which makes the diff show colored in the terminal), so removing it :)

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Needs review » Reviewed & tested by the community

Let's get this in.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Looks good to me. Committed to CVS HEAD.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.