From 2f9e892c74584db0eca1d5ef9c4d734891495a30 Mon Sep 17 00:00:00 2001
From: daniel <daniel.kinzler@wikimedia.de>
Date: Tue, 19 Jun 2018 13:37:35 +0200
Subject: [PATCH] When encountering bad blobs, log the text row id.

Change-Id: Ie7c932f229854fd3582d507fa7cc154ffdd21f55
---
 includes/Revision.php                   | 13 +++++++++++-
 includes/Storage/SqlBlobStore.php       |  4 ++--
 tests/phpunit/includes/RevisionTest.php | 27 +++++++++++++++++++++++++
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/includes/Revision.php b/includes/Revision.php
index 548ef8d720..c6889ab1ab 100644
--- a/includes/Revision.php
+++ b/includes/Revision.php
@@ -1038,7 +1038,18 @@ class Revision implements IDBAccessObject {
 			? SqlBlobStore::makeAddressFromTextId( $row->old_id )
 			: null;
 
-		return self::getBlobStore( $wiki )->expandBlob( $text, $flags, $cacheKey );
+		$revisionText = self::getBlobStore( $wiki )->expandBlob( $text, $flags, $cacheKey );
+
+		if ( $revisionText === false ) {
+			if ( isset( $row->old_id ) ) {
+				wfLogWarning( __METHOD__ . ": Bad data in text row {$row->old_id}! " );
+			} else {
+				wfLogWarning( __METHOD__ . ": Bad data in text row! " );
+			}
+			return false;
+		}
+
+		return $revisionText;
 	}
 
 	/**
diff --git a/includes/Storage/SqlBlobStore.php b/includes/Storage/SqlBlobStore.php
index 72de2c961a..f097f67657 100644
--- a/includes/Storage/SqlBlobStore.php
+++ b/includes/Storage/SqlBlobStore.php
@@ -349,7 +349,7 @@ class SqlBlobStore implements IDBAccessObject, BlobStore {
 		$blob = $this->expandBlob( $row->old_text, $row->old_flags, $blobAddress );
 
 		if ( $blob === false ) {
-			wfWarn( __METHOD__ . ": Bad data in text row $textId." );
+			wfLogWarning( __METHOD__ . ": Bad data in text row $textId." );
 			return false;
 		}
 
@@ -486,7 +486,7 @@ class SqlBlobStore implements IDBAccessObject, BlobStore {
 			$blob = gzinflate( $blob );
 
 			if ( $blob === false ) {
-				wfLogWarning( __METHOD__ . ': gzinflate() failed' );
+				wfWarn( __METHOD__ . ': gzinflate() failed' );
 				return false;
 			}
 		}
diff --git a/tests/phpunit/includes/RevisionTest.php b/tests/phpunit/includes/RevisionTest.php
index ab067a4746..f50dc80817 100644
--- a/tests/phpunit/includes/RevisionTest.php
+++ b/tests/phpunit/includes/RevisionTest.php
@@ -439,6 +439,31 @@ class RevisionTest extends MediaWikiTestCase {
 		$this->testGetRevisionText( $expected, $rowData );
 	}
 
+	public function provideGetRevisionTextWithZlibExtension_badData() {
+		yield 'Generic gzip test' => [
+			'This is a small goat of revision text.',
+			[
+				'old_flags' => 'gzip',
+				'old_text' => 'DEAD BEEF',
+			],
+		];
+	}
+
+	/**
+	 * @covers Revision::getRevisionText
+	 * @dataProvider provideGetRevisionTextWithZlibExtension_badData
+	 */
+	public function testGetRevisionWithZlibExtension_badData( $expected, $rowData ) {
+		$this->checkPHPExtension( 'zlib' );
+		Wikimedia\suppressWarnings();
+		$this->assertFalse(
+			Revision::getRevisionText(
+				(object)$rowData
+			)
+		);
+		Wikimedia\suppressWarnings( true );
+	}
+
 	private function getWANObjectCache() {
 		return new WANObjectCache( [ 'cache' => new HashBagOStuff() ] );
 	}
@@ -802,6 +827,7 @@ class RevisionTest extends MediaWikiTestCase {
 	public function testGetRevisionText_external_returnsFalseWhenNotEnoughUrlParts(
 		$text
 	) {
+		Wikimedia\suppressWarnings();
 		$this->assertFalse(
 			Revision::getRevisionText(
 				(object)[
@@ -810,6 +836,7 @@ class RevisionTest extends MediaWikiTestCase {
 				]
 			)
 		);
+		Wikimedia\suppressWarnings( true );
 	}
 
 	/**
-- 
2.20.1