Skip to content

Race Condition in Connection::Replace() #5440

@Mnkras

Description

@Mnkras

@KorvinSzanto brought this to my attention, there are a few ways to solve this,

        $num = $this->query($qb->getSql())->fetchColumn();
        // race happens right here :/
        if ($num < 1) {
            $this->insert($table, $fieldArray);
        } else {
            $this->update($table, $fieldArray, $updateKeys);
        }

I currently have a hack way that executes a raw, MYSQL only query (using ON DUPLICATE KEY UPDATE).

The ideal solution is to migrate away from Connection::Replace() and use the EntityManager ($em->merge()) which handles this, but this code is there for BC with ADODB, and migrating everything is gonna be annoying. My current hack is below, its just basic query building (in a hacky way)

Looking for input from anyone on a better way to solve

diff --git a/concrete/src/Database/Connection/Connection.php b/concrete/src/Database/Connection/Connection.php
index ce98c0422c..9602590c2c 100644
--- a/concrete/src/Database/Connection/Connection.php
+++ b/concrete/src/Database/Connection/Connection.php
@@ -181,8 +181,14 @@ public function MetaColumnNames($table)
     public function Replace($table, $fieldArray, $keyCol, $autoQuote = true)
     {
         $qb = $this->createQueryBuilder();
-        $qb->select('count(*)')->from($table, 't');
-        $where = $qb->expr()->andX();
+        $qb->insert($table);
+        if ($autoQuote) {
+            foreach ($fieldArray as $key => $value) {
+                $fieldArray[$key] = $this->quote($value);
+            }
+        }
+        $qb->values($fieldArray);
+        $sql = $qb->getSQL();
         $updateKeys = array();
         if (!is_array($keyCol)) {
             $keyCol = array($keyCol);
@@ -194,18 +200,17 @@ public function Replace($table, $fieldArray, $keyCol, $autoQuote = true)
                 $field = null;
             }
             $updateKeys[$key] = $field;
-            if ($autoQuote) {
-                $field = $qb->expr()->literal($field);
-            }
-            $where->add($qb->expr()->eq($key, $field));
         }
-        $qb->where($where);
-        $num = $this->query($qb->getSql())->fetchColumn();
-        if ($num < 1) {
-            $this->insert($table, $fieldArray);
-        } else {
-            $this->update($table, $fieldArray, $updateKeys);
+        $on_dupe = ' ON DUPLICATE KEY UPDATE';
+        $dup_keys = [];
+        foreach ($updateKeys as $key => $value) {
+
+            $dup_keys[] = sprintf(' %s=%s', $key, $value);
         }
+        $on_dupe .= join(',', $dup_keys);
+        $sql .= $on_dupe;
+        $this->executeQuery($sql);
+
     }

     /**

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type:BugExisting functionality not performing as expected.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions