Convert Phabricator to handle "%s" / "%B" properly

Summary:
Ref T1191. I believe we only have three meaningful binary fields across all applications:

  - The general cache may contain gzipped content.
  - The file storage blob may contain arbitrary binary content.
  - The Passphrase secret can store arbitrary binary data (although it currently never does).

This adds Lisk config for binary fields, and uses `%B` where necessary.

Test Plan:
  - Added and executed unit tests.
  - Forced file uploads to use MySQL, uploaded binaries.
  - Disabled the CONFIG_BINARY on the file storage blob and tried again, got an appropraite failure.
  - Tried to register with an account containing a G-Clef, and was stopped before the insert.

Reviewers: btrahan, arice

Reviewed By: arice

CC: arice, chad, aran

Maniphest Tasks: T1191

Differential Revision: https://secure.phabricator.com/D8316

src/aphront/console/DarkConsoleCore.php

@@ -74,9 +74,18 @@ final class DarkConsoleCore {
     $cache = new PhutilKeyValueCacheProfiler($cache);
     $cache->setProfiler(PhutilServiceProfiler::getInstance());
 
+    // This encoding may fail if there are, e.g., database queries which
+    // include binary data. It would be a little cleaner to try to strip these,
+    // but just do something non-broken here if we end up with unrepresentable
+    // data.
+    $json = @json_encode($storage);
+    if (!$json) {
+      $json = '{}';
+    }
+
     $cache->setKeys(
       array(
-        'darkconsole:'.$key => json_encode($storage),
+        'darkconsole:'.$key => $json,
       ),
       $ttl = (60 * 60 * 6));
 

src/applications/cache/PhabricatorKeyValueDatabaseCache.php

@@ -19,7 +19,7 @@ final class PhabricatorKeyValueDatabaseCache
 
         $sql[] = qsprintf(
           $conn_w,
-          '(%s, %s, %s, %s, %d, %nd)',
+          '(%s, %s, %s, %B, %d, %nd)',
           $hash,
           $key,
           $format,

src/applications/files/storage/PhabricatorFileStorageBlob.php

@@ -2,46 +2,17 @@
 
 /**
  * Simple blob store DAO for @{class:PhabricatorMySQLFileStorageEngine}.
- *
- * @group file
  */
 final class PhabricatorFileStorageBlob extends PhabricatorFileDAO {
-  // max_allowed_packet defaults to 1 MiB, escaping can make the data twice
-  // longer, query fits in the rest.
-  const CHUNK_SIZE = 5e5;
 
   protected $data;
 
-  private $fullData;
-
-  protected function willWriteData(array &$data) {
-    parent::willWriteData($data);
-
-    $this->fullData = $data['data'];
-    if (strlen($data['data']) > self::CHUNK_SIZE) {
-      $data['data'] = substr($data['data'], 0, self::CHUNK_SIZE);
-      $this->openTransaction();
-    }
-  }
-
-  protected function didWriteData() {
-    $size = self::CHUNK_SIZE;
-    $length = strlen($this->fullData);
-    if ($length > $size) {
-      $conn = $this->establishConnection('w');
-      for ($offset = $size; $offset < $length; $offset += $size) {
-        queryfx(
-          $conn,
-          'UPDATE %T SET data = CONCAT(data, %s) WHERE %C = %d',
-          $this->getTableName(),
-          substr($this->fullData, $offset, $size),
-          $this->getIDKeyForUse(),
-          $this->getID());
-      }
-      $this->saveTransaction();
-    }
-
-    parent::didWriteData();
+  public function getConfiguration() {
+    return array(
+      self::CONFIG_BINARY => array(
+        'data' => true,
+      ),
+    ) + parent::getConfiguration();
   }
 
 }

src/applications/passphrase/storage/PassphraseSecret.php

@@ -7,6 +7,9 @@ final class PassphraseSecret extends PassphraseDAO {
   public function getConfiguration() {
     return array(
       self::CONFIG_TIMESTAMPS => false,
+      self::CONFIG_BINARY => array(
+        'secretData' => true,
+      ),
     ) + parent::getConfiguration();
   }
 

src/applications/repository/storage/PhabricatorRepositoryPushLog.php

@@ -59,6 +59,9 @@ final class PhabricatorRepositoryPushLog
     return array(
       self::CONFIG_AUX_PHID => true,
       self::CONFIG_TIMESTAMPS => false,
+      self::CONFIG_BINARY => array(
+        'refNameRaw' => true,
+      ),
     ) + parent::getConfiguration();
   }
 

src/applications/repository/storage/PhabricatorRepositoryRefCursor.php

@@ -24,6 +24,9 @@ final class PhabricatorRepositoryRefCursor extends PhabricatorRepositoryDAO
   public function getConfiguration() {
     return array(
       self::CONFIG_TIMESTAMPS => false,
+      self::CONFIG_BINARY => array(
+        'refNameRaw' => true,
+      ),
     ) + parent::getConfiguration();
   }
 

src/infrastructure/__tests__/PhabricatorInfrastructureTestCase.php

@@ -77,5 +77,28 @@ final class PhabricatorInfrastructureTestCase
     $this->assertEqual($buf, $read->getBigData());
   }
 
+  public function testRejectMySQLBMPQueries() {
+    $table = new HarbormasterScratchTable();
+    $conn_r = $table->establishConnection('w');
+
+    $snowman = "\xE2\x98\x83";
+    $gclef = "\xF0\x9D\x84\x9E";
+
+    qsprintf($conn_r, 'SELECT %B', $snowman);
+    qsprintf($conn_r, 'SELECT %s', $snowman);
+    qsprintf($conn_r, 'SELECT %B', $gclef);
+
+    $caught = null;
+    try {
+      qsprintf($conn_r, 'SELECT %s', $gclef);
+    } catch (AphrontQueryCharacterSetException $ex) {
+      $caught = $ex;
+    }
+
+    $this->assertEqual(
+      true,
+      ($caught instanceof AphrontQueryCharacterSetException));
+  }
+
 }
 

src/infrastructure/storage/__tests__/QueryFormattingTestCase.php

@@ -35,6 +35,23 @@ final class QueryFormattingTestCase extends PhabricatorTestCase {
     $this->assertEqual(
       'NULL',
       qsprintf($conn_r, '%ns', null));
+
+    $this->assertEqual(
+      "'<S>', '<S>'",
+      qsprintf($conn_r, '%Ls', array('x', 'y')));
+
+    $this->assertEqual(
+      "'<B>'",
+      qsprintf($conn_r, '%B', null));
+
+    $this->assertEqual(
+      "NULL",
+      qsprintf($conn_r, '%nB', null));
+
+    $this->assertEqual(
+      "'<B>', '<B>'",
+      qsprintf($conn_r, '%LB', array('x', 'y')));
   }
 
+
 }

src/infrastructure/storage/lisk/LiskDAO.php

@@ -171,6 +171,7 @@ abstract class LiskDAO {
   const CONFIG_AUX_PHID             = 'auxiliary-phid';
   const CONFIG_SERIALIZATION        = 'col-serialization';
   const CONFIG_PARTIAL_OBJECTS      = 'partial-objects';
+  const CONFIG_BINARY               = 'binary';
 
   const SERIALIZATION_NONE          = 'id';
   const SERIALIZATION_JSON          = 'json';
@@ -356,6 +357,11 @@ abstract class LiskDAO {
    * directly access or assign protected members of your class (use the getters
    * and setters).
    *
+   * CONFIG_BINARY
+   * You can optionally provide a map of columns to a flag indicating that
+   * they store binary data. These columns will not raise an error when
+   * handling binary writes.
+   *
    * @return dictionary  Map of configuration options to values.
    *
    * @task   config
@@ -1148,10 +1154,15 @@ abstract class LiskDAO {
     }
 
     $conn = $this->establishConnection('w');
+    $binary = $this->getBinaryColumns();
 
     foreach ($map as $key => $value) {
+      if (!empty($binary[$key])) {
+        $map[$key] = qsprintf($conn, '%C = %nB', $key, $value);
+      } else {
         $map[$key] = qsprintf($conn, '%C = %ns', $key, $value);
       }
+    }
     $map = implode(', ', $map);
 
     $id = $this->getID();
@@ -1242,10 +1253,15 @@ abstract class LiskDAO {
     $this->willWriteData($data);
 
     $columns = array_keys($data);
+    $binary = $this->getBinaryColumns();
 
     foreach ($data as $key => $value) {
       try {
+        if (!empty($binary[$key])) {
+          $data[$key] = qsprintf($conn, '%nB', $value);
+        } else {
           $data[$key] = qsprintf($conn, '%ns', $value);
+        }
       } catch (AphrontQueryParameterException $parameter_exception) {
         throw new PhutilProxyException(
           pht(
@@ -1803,4 +1819,8 @@ abstract class LiskDAO {
     return $conn_w->getInsertID();
   }
 
+  private function getBinaryColumns() {
+    return $this->getConfigOption(self::CONFIG_BINARY);
+  }
+
 }