Daniel Kinzler has uploaded a new change for review. ( https://gerrit.wikimedia.org/r/399208 )
Change subject: Prevent write operations to database replicas. ...................................................................... Prevent write operations to database replicas. Bug: T183265 Change-Id: I8e17644d1b447416adee18e42cf0122b52a80b22 --- M includes/libs/rdbms/database/Database.php M includes/libs/rdbms/loadbalancer/ILoadBalancer.php M includes/libs/rdbms/loadbalancer/LoadBalancer.php 3 files changed, 60 insertions(+), 0 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core refs/changes/08/399208/1 diff --git a/includes/libs/rdbms/database/Database.php b/includes/libs/rdbms/database/Database.php index 7f0718c..1c9cad5 100644 --- a/includes/libs/rdbms/database/Database.php +++ b/includes/libs/rdbms/database/Database.php @@ -237,6 +237,12 @@ protected $trxProfiler; /** + * @var bool Whether writing is allowed on this connection. + * Should be false for connections to replicas. + */ + protected $allowWrite = true; + + /** * Constructor and database handle and attempt to connect to the DB server * * IDatabase classes should not be constructed directly in external @@ -277,6 +283,7 @@ $this->connLogger = $params['connLogger']; $this->queryLogger = $params['queryLogger']; $this->errorLogger = $params['errorLogger']; + $this->allowWrite = empty( $params['noWrite'] ); // Set initial dummy domain until open() sets the final DB/prefix $this->currentDomain = DatabaseDomain::newUnspecified(); @@ -908,6 +915,13 @@ } if ( $isWrite ) { + if ( !$this->allowWrite ) { + throw new DBError( + $this, + 'Write operations are not allowed on this database connection!' + ); + } + # In theory, non-persistent writes are allowed in read-only mode, but due to things # like https://bugs.mysql.com/bug.php?id=33669 that might not work anyway... $reason = $this->getReadOnlyReason(); @@ -2951,6 +2965,13 @@ } final public function begin( $fname = __METHOD__, $mode = self::TRANSACTION_EXPLICIT ) { + if ( !$this->allowWrite ) { + throw new DBError( + $this, + 'Write operations are not allowed on this database connection!' + ); + } + // Protect against mismatched atomic section, transaction nesting, and snapshot loss if ( $this->mTrxLevel ) { if ( $this->mTrxAtomicLevels ) { @@ -3012,6 +3033,14 @@ } final public function commit( $fname = __METHOD__, $flush = '' ) { + if ( !$this->allowWrite ) { + // we should never get here, since begin() would already throw + throw new DBError( + $this, + 'Write operations are not allowed on this database connection!' + ); + } + if ( $this->mTrxLevel && $this->mTrxAtomicLevels ) { // There are still atomic sections open. This cannot be ignored $levels = implode( ', ', $this->mTrxAtomicLevels ); diff --git a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php index 86c4335..b565b3b 100644 --- a/includes/libs/rdbms/loadbalancer/ILoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/ILoadBalancer.php @@ -87,6 +87,9 @@ /** @var int DB handle should have DBO_TRX disabled and the caller will leave it as such */ const CONN_TRX_AUTO = 1; + /** Disable writing for the given connection. Used internally. Do not use with DB_MASTER! */ + const CONN_NO_WRITE = 2; + /** * Construct a manager of IDatabase connection objects * diff --git a/includes/libs/rdbms/loadbalancer/LoadBalancer.php b/includes/libs/rdbms/loadbalancer/LoadBalancer.php index a9eaa99..eb288dd 100644 --- a/includes/libs/rdbms/loadbalancer/LoadBalancer.php +++ b/includes/libs/rdbms/loadbalancer/LoadBalancer.php @@ -645,6 +645,12 @@ $oldConnsOpened = $this->connsOpened; // connections open now if ( $i == self::DB_MASTER ) { + if ( $flags & self::CONN_NO_WRITE ) { + throw new InvalidArgumentException( + 'Cannot set CONN_NO_WRITE flag on master connection!' + ); + } + $i = $this->getWriterIndex(); } else { # Try to find an available server in any the query groups (in order) @@ -655,6 +661,9 @@ break; } } + + // Request no-write connection, even if $i == $this->getWriterIndex(). + $flags |= self::CONN_NO_WRITE; } # Operation-based index @@ -671,6 +680,9 @@ $this->reportConnectionError(); return null; // not reached } + + // Request no-write connection, even if $i == $this->getWriterIndex(). + $flags |= self::CONN_NO_WRITE; } # Now we have an explicit index into the servers array @@ -791,6 +803,13 @@ // a) those are usually set to implicitly use transaction rounds via DBO_TRX // b) those must support the use of explicit transaction rounds via beginMasterChanges() $autoCommit = ( ( $flags & self::CONN_TRX_AUTO ) == self::CONN_TRX_AUTO ); + $noWrite = ( ( $flags & self::CONN_NO_WRITE ) == self::CONN_NO_WRITE ); + + if ( $noWrite && $i === $this->getWriterIndex() ) { + // We can't disable writes on the master connection! + // TODO: Wrap the master connection, so write operations fail! + $noWrite = false; + } if ( $domain !== false ) { // Connection is to a foreign domain @@ -807,6 +826,7 @@ // Open a new connection $server = $this->mServers[$i]; $server['serverIndex'] = $i; + $server['noWrite'] = $noWrite; $server['autoCommitOnly'] = $autoCommit; $conn = $this->reallyOpenConnection( $server, false ); $host = $this->getServerName( $i ); @@ -863,6 +883,13 @@ $dbName = $domainInstance->getDatabase(); $prefix = $domainInstance->getTablePrefix(); $autoCommit = ( ( $flags & self::CONN_TRX_AUTO ) == self::CONN_TRX_AUTO ); + $noWrite = ( ( $flags & self::CONN_NO_WRITE ) == self::CONN_NO_WRITE ); + + if ( $noWrite && $i === $this->getWriterIndex() ) { + // We can't disable writes on the master connection! + // TODO: Wrap the master connection, so write operations fail! + $noWrite = false; + } if ( $autoCommit ) { $connFreeKey = self::KEY_FOREIGN_FREE_NOROUND; @@ -910,6 +937,7 @@ $server['foreignPoolRefCount'] = 0; $server['foreign'] = true; $server['autoCommitOnly'] = $autoCommit; + $server['noWrite'] = $noWrite; $conn = $this->reallyOpenConnection( $server, $dbName ); if ( !$conn->isOpen() ) { $this->connLogger->warning( __METHOD__ . ": connection error for $i/$domain" ); -- To view, visit https://gerrit.wikimedia.org/r/399208 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I8e17644d1b447416adee18e42cf0122b52a80b22 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/core Gerrit-Branch: master Gerrit-Owner: Daniel Kinzler <daniel.kinz...@wikimedia.de> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits