From a298a79bdac6cd2904c949b18d8f4b65cfb2afc9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 23 Feb 2014 16:20:46 -0800 Subject: [PATCH] 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 | 11 ++++- .../PhabricatorKeyValueDatabaseCache.php | 2 +- .../storage/PhabricatorFileStorageBlob.php | 41 +++---------------- .../passphrase/storage/PassphraseSecret.php | 3 ++ .../storage/PhabricatorRepositoryPushLog.php | 3 ++ .../PhabricatorRepositoryRefCursor.php | 3 ++ .../PhabricatorInfrastructureTestCase.php | 23 +++++++++++ .../__tests__/QueryFormattingTestCase.php | 17 ++++++++ src/infrastructure/storage/lisk/LiskDAO.php | 24 ++++++++++- 9 files changed, 88 insertions(+), 39 deletions(-) diff --git a/src/aphront/console/DarkConsoleCore.php b/src/aphront/console/DarkConsoleCore.php index 36598026b5..3501f514ea 100644 --- a/src/aphront/console/DarkConsoleCore.php +++ b/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)); diff --git a/src/applications/cache/PhabricatorKeyValueDatabaseCache.php b/src/applications/cache/PhabricatorKeyValueDatabaseCache.php index 675edee1f1..822b90ffcf 100644 --- a/src/applications/cache/PhabricatorKeyValueDatabaseCache.php +++ b/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, diff --git a/src/applications/files/storage/PhabricatorFileStorageBlob.php b/src/applications/files/storage/PhabricatorFileStorageBlob.php index 38a7e46695..502b7dd8d7 100644 --- a/src/applications/files/storage/PhabricatorFileStorageBlob.php +++ b/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(); } } diff --git a/src/applications/passphrase/storage/PassphraseSecret.php b/src/applications/passphrase/storage/PassphraseSecret.php index 5661338692..c600ef571d 100644 --- a/src/applications/passphrase/storage/PassphraseSecret.php +++ b/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(); } diff --git a/src/applications/repository/storage/PhabricatorRepositoryPushLog.php b/src/applications/repository/storage/PhabricatorRepositoryPushLog.php index 3c0fdff381..9efa3a0a1a 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryPushLog.php +++ b/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(); } diff --git a/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php b/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php index 8f93490056..7cc6eeaa9e 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryRefCursor.php +++ b/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(); } diff --git a/src/infrastructure/__tests__/PhabricatorInfrastructureTestCase.php b/src/infrastructure/__tests__/PhabricatorInfrastructureTestCase.php index 7697f344b1..47cdbf14fe 100644 --- a/src/infrastructure/__tests__/PhabricatorInfrastructureTestCase.php +++ b/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)); + } + } diff --git a/src/infrastructure/storage/__tests__/QueryFormattingTestCase.php b/src/infrastructure/storage/__tests__/QueryFormattingTestCase.php index b7ef5f4ce8..c583497751 100644 --- a/src/infrastructure/storage/__tests__/QueryFormattingTestCase.php +++ b/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( + "'', ''", + qsprintf($conn_r, '%Ls', array('x', 'y'))); + + $this->assertEqual( + "''", + qsprintf($conn_r, '%B', null)); + + $this->assertEqual( + "NULL", + qsprintf($conn_r, '%nB', null)); + + $this->assertEqual( + "'', ''", + qsprintf($conn_r, '%LB', array('x', 'y'))); } + } diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php index d755fba930..b0fed9decf 100644 --- a/src/infrastructure/storage/lisk/LiskDAO.php +++ b/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,9 +1154,14 @@ abstract class LiskDAO { } $conn = $this->establishConnection('w'); + $binary = $this->getBinaryColumns(); foreach ($map as $key => $value) { - $map[$key] = qsprintf($conn, '%C = %ns', $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); @@ -1242,10 +1253,15 @@ abstract class LiskDAO { $this->willWriteData($data); $columns = array_keys($data); + $binary = $this->getBinaryColumns(); foreach ($data as $key => $value) { try { - $data[$key] = qsprintf($conn, '%ns', $value); + 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); + } + }