From aa4f4d6b53ae3551678a5ba7e1a021492f090287 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 26 Apr 2019 13:11:09 -0700 Subject: [PATCH] jobqueue: make JobQueueDB stricter about broken job_params fields Also rename throwDBException() to getDBException() and make the callers throw the result to avoid phpstorm warnings. Remove $row assignment that is always overridden as well. Change-Id: I84bc4b11f10152eada6dd6f4788c4f79dcb4a2fb --- includes/jobqueue/JobQueueDB.php | 68 ++++++++++++----------------- includes/jobqueue/JobQueueRedis.php | 2 +- 2 files changed, 30 insertions(+), 40 deletions(-) diff --git a/includes/jobqueue/JobQueueDB.php b/includes/jobqueue/JobQueueDB.php index 47ee5886d4..7c78f40031 100644 --- a/includes/jobqueue/JobQueueDB.php +++ b/includes/jobqueue/JobQueueDB.php @@ -91,7 +91,7 @@ class JobQueueDB extends JobQueue { 'job', '1', [ 'job_cmd' => $this->type, 'job_token' => '' ], __METHOD__ ); } catch ( DBError $e ) { - $this->throwDBException( $e ); + throw $this->getDBException( $e ); } return !$found; @@ -118,7 +118,7 @@ class JobQueueDB extends JobQueue { __METHOD__ ); } catch ( DBError $e ) { - $this->throwDBException( $e ); + throw $this->getDBException( $e ); } $this->cache->set( $key, $size, self::CACHE_TTL_SHORT ); @@ -150,7 +150,7 @@ class JobQueueDB extends JobQueue { __METHOD__ ); } catch ( DBError $e ) { - $this->throwDBException( $e ); + throw $this->getDBException( $e ); } $this->cache->set( $key, $count, self::CACHE_TTL_SHORT ); @@ -187,7 +187,7 @@ class JobQueueDB extends JobQueue { __METHOD__ ); } catch ( DBError $e ) { - $this->throwDBException( $e ); + throw $this->getDBException( $e ); } $this->cache->set( $key, $count, self::CACHE_TTL_SHORT ); @@ -281,7 +281,7 @@ class JobQueueDB extends JobQueue { count( $rowSet ) + count( $rowList ) - count( $rows ) ); } catch ( DBError $e ) { - $this->throwDBException( $e ); + throw $this->getDBException( $e ); } if ( $flags & self::QOS_ATOMIC ) { $dbw->endAtomic( $method ); @@ -316,12 +316,7 @@ class JobQueueDB extends JobQueue { $this->incrStats( 'pops', $this->type ); // Get the job object from the row... - $params = self::extractBlob( $row->job_params ); - $params = is_array( $params ) ? $params : []; // sanity - $params += [ 'namespace' => $row->job_namespace, 'title' => $row->job_title ]; - $job = $this->factoryJob( $row->job_cmd, $params ); - $job->setMetadata( 'id', $row->job_id ); - $job->setMetadata( 'timestamp', $row->job_timestamp ); + $job = $this->jobFromRow( $row ); break; // done } while ( true ); @@ -331,7 +326,7 @@ class JobQueueDB extends JobQueue { $this->recycleAndDeleteStaleJobs(); } } catch ( DBError $e ) { - $this->throwDBException( $e ); + throw $this->getDBException( $e ); } return $job; @@ -352,7 +347,6 @@ class JobQueueDB extends JobQueue { // Check cache to see if the queue has <= OFFSET items $tinyQueue = $this->cache->get( $this->getCacheKey( 'small' ) ); - $row = false; // the row acquired $invertedDirection = false; // whether one job_random direction was already scanned // This uses a replication safe method for acquiring jobs. One could use UPDATE+LIMIT // instead, but that either uses ORDER BY (in which case it deadlocks in MySQL) or is @@ -505,7 +499,7 @@ class JobQueueDB extends JobQueue { $this->incrStats( 'acks', $this->type ); } catch ( DBError $e ) { - $this->throwDBException( $e ); + throw $this->getDBException( $e ); } } @@ -560,7 +554,7 @@ class JobQueueDB extends JobQueue { try { $dbw->delete( 'job', [ 'job_cmd' => $this->type ] ); } catch ( DBError $e ) { - $this->throwDBException( $e ); + throw $this->getDBException( $e ); } return true; @@ -619,22 +613,11 @@ class JobQueueDB extends JobQueue { return new MappedIterator( $dbr->select( 'job', self::selectFields(), $conds ), function ( $row ) { - $params = strlen( $row->job_params ) ? unserialize( $row->job_params ) : []; - $params = is_array( $params ) ? $params : []; // sanity - $params += [ - 'namespace' => $row->job_namespace, - 'title' => $row->job_title - ]; - - $job = $this->factoryJob( $row->job_cmd, $params ); - $job->setMetadata( 'id', $row->job_id ); - $job->setMetadata( 'timestamp', $row->job_timestamp ); - - return $job; + return $this->jobFromRow( $row ); } ); } catch ( DBError $e ) { - $this->throwDBException( $e ); + throw $this->getDBException( $e ); } } @@ -764,7 +747,7 @@ class JobQueueDB extends JobQueue { $dbw->unlock( "jobqueue-recycle-{$this->type}", __METHOD__ ); } catch ( DBError $e ) { - $this->throwDBException( $e ); + throw $this->getDBException( $e ); } return $count; @@ -895,23 +878,30 @@ class JobQueueDB extends JobQueue { } /** - * @param string $blob - * @return bool|mixed + * @param stdClass $row + * @return RunnableJob|null */ - protected static function extractBlob( $blob ) { - if ( (string)$blob !== '' ) { - return unserialize( $blob ); - } else { - return false; + protected function jobFromRow( $row ) { + $params = ( (string)$row->job_params !== '' ) ? unserialize( $row->job_params ) : []; + if ( !is_array( $params ) ) { // this shouldn't happen + throw new UnexpectedValueException( + "Could not unserialize job with ID '{$row->job_id}'." ); } + + $params += [ 'namespace' => $row->job_namespace, 'title' => $row->job_title ]; + $job = $this->factoryJob( $row->job_cmd, $params ); + $job->setMetadata( 'id', $row->job_id ); + $job->setMetadata( 'timestamp', $row->job_timestamp ); + + return $job; } /** * @param DBError $e - * @throws JobQueueError + * @return JobQueueError */ - protected function throwDBException( DBError $e ) { - throw new JobQueueError( get_class( $e ) . ": " . $e->getMessage() ); + protected function getDBException( DBError $e ) { + return new JobQueueError( get_class( $e ) . ": " . $e->getMessage() ); } /** diff --git a/includes/jobqueue/JobQueueRedis.php b/includes/jobqueue/JobQueueRedis.php index 886468826b..2140043b8f 100644 --- a/includes/jobqueue/JobQueueRedis.php +++ b/includes/jobqueue/JobQueueRedis.php @@ -639,7 +639,7 @@ LUA; } $item = $this->unserialize( $data ); if ( !is_array( $item ) ) { // this shouldn't happen - throw new UnexpectedValueException( "Could not find job with ID '$uid'." ); + throw new UnexpectedValueException( "Could not unserialize job with ID '$uid'." ); } $params = $item['params']; -- 2.20.1