fix: Harden files scanner for invalid null access

Co-authored-by: Ferdinand Thiessen <opensource@fthiessen.de>
Co-authored-by: Kate <26026535+provokateurin@users.noreply.github.com>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

6 files changed, 170 insertions, 172 deletions

diff --git a/lib/private/Files/Cache/Cache.php b/lib/private/Files/Cache/Cache.php
index cb841755efd..7fbb625050b 100644
--- a/lib/private/Files/Cache/Cache.php
+++ b/lib/private/Files/Cache/Cache.php
@@ -109,7 +109,7 @@ class Cache implements ICache {
 	/**
 	 * get the stored metadata of a file or folder
 	 *
-	 * @param string | int $file either the path of a file or folder or the file id for a file or folder
+	 * @param string|int $file either the path of a file or folder or the file id for a file or folder
 	 * @return ICacheEntry|false the cache entry as array or false if the file is not found in the cache
 	 */
 	public function get($file) {
@@ -131,15 +131,17 @@ class Cache implements ICache {
 		$data = $result->fetch();
 		$result->closeCursor();
 
-		//merge partial data
-		if (!$data && is_string($file) && isset($this->partial[$file])) {
-			return $this->partial[$file];
-		} elseif (!$data) {
-			return $data;
-		} else {
+		if ($data !== false) {
 			$data['metadata'] = $metadataQuery->extractMetadata($data)->asArray();
 			return self::cacheEntryFromData($data, $this->mimetypeLoader);
+		} else {
+			//merge partial data
+			if (is_string($file) && isset($this->partial[$file])) {
+				return $this->partial[$file];
+			}
 		}
+
+		return false;
 	}
 
 	/**
@@ -886,19 +888,23 @@ class Cache implements ICache {
 	/**
 	 * Re-calculate the folder size and the size of all parent folders
 	 *
-	 * @param string|boolean $path
-	 * @param array $data (optional) meta data of the folder
+	 * @param array|ICacheEntry|null $data (optional) meta data of the folder
 	 */
-	public function correctFolderSize($path, $data = null, $isBackgroundScan = false) {
+	public function correctFolderSize(string $path, $data = null, bool $isBackgroundScan = false): void {
 		$this->calculateFolderSize($path, $data);
+
 		if ($path !== '') {
 			$parent = dirname($path);
 			if ($parent === '.' || $parent === '/') {
 				$parent = '';
 			}
+
 			if ($isBackgroundScan) {
 				$parentData = $this->get($parent);
-				if ($parentData['size'] !== -1 && $this->getIncompleteChildrenCount($parentData['fileid']) === 0) {
+				if ($parentData !== false
+					&& $parentData['size'] !== -1
+					&& $this->getIncompleteChildrenCount($parentData['fileid']) === 0
+				) {
 					$this->correctFolderSize($parent, $parentData, $isBackgroundScan);
 				}
 			} else {
@@ -1009,8 +1015,8 @@ class Cache implements ICache {
 			}
 
 			// only set unencrypted size for a folder if any child entries have it set, or the folder is empty
-			$shouldWriteUnEncryptedSize = $unencryptedMax > 0 || $totalSize === 0 || $entry['unencrypted_size'] > 0;
-			if ($entry['size'] !== $totalSize || ($entry['unencrypted_size'] !== $unencryptedTotal && $shouldWriteUnEncryptedSize)) {
+			$shouldWriteUnEncryptedSize = $unencryptedMax > 0 || $totalSize === 0 || ($entry['unencrypted_size'] ?? 0) > 0;
+			if ($entry['size'] !== $totalSize || (($entry['unencrypted_size'] ?? 0) !== $unencryptedTotal && $shouldWriteUnEncryptedSize)) {
 				if ($shouldWriteUnEncryptedSize) {
 					// if all children have an unencrypted size of 0, just set the folder unencrypted size to 0 instead of summing the sizes
 					if ($unencryptedMax === 0) {
diff --git a/lib/private/Files/Cache/Scanner.php b/lib/private/Files/Cache/Scanner.php
index 3bd674f79e2..1fb408a0655 100644
--- a/lib/private/Files/Cache/Scanner.php
+++ b/lib/private/Files/Cache/Scanner.php
@@ -83,7 +83,7 @@ class Scanner extends BasicEmitter implements IScanner {
 	 *
 	 * @param bool $useTransactions
 	 */
-	public function setUseTransactions($useTransactions) {
+	public function setUseTransactions($useTransactions): void {
 		$this->useTransactions = $useTransactions;
 	}
 
@@ -108,9 +108,9 @@ class Scanner extends BasicEmitter implements IScanner {
 	 * @param string $file
 	 * @param int $reuseExisting
 	 * @param int $parentId
-	 * @param array|null|false $cacheData existing data in the cache for the file to be scanned
+	 * @param array|CacheEntry|null|false $cacheData existing data in the cache for the file to be scanned
 	 * @param bool $lock set to false to disable getting an additional read lock during scanning
-	 * @param null $data the metadata for the file, as returned by the storage
+	 * @param array|null $data the metadata for the file, as returned by the storage
 	 * @return array|null an array of metadata of the scanned file
 	 * @throws \OCP\Lock\LockedException
 	 */
@@ -122,139 +122,130 @@ class Scanner extends BasicEmitter implements IScanner {
 				return null;
 			}
 		}
+
 		// only proceed if $file is not a partial file, blacklist is handled by the storage
-		if (!self::isPartialFile($file)) {
-			// acquire a lock
+		if (self::isPartialFile($file)) {
+			return null;
+		}
+
+		// acquire a lock
+		if ($lock) {
+			if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
+				$this->storage->acquireLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
+			}
+		}
+
+		try {
+			$data = $data ?? $this->getData($file);
+		} catch (ForbiddenException $e) {
 			if ($lock) {
 				if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
-					$this->storage->acquireLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
+					$this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
 				}
 			}
 
-			try {
-				$data = $data ?? $this->getData($file);
-			} catch (ForbiddenException $e) {
-				if ($lock) {
-					if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
-						$this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
-					}
+			return null;
+		}
+
+		try {
+			if ($data === null) {
+				$this->removeFromCache($file);
+			} else {
+				// pre-emit only if it was a file. By that we avoid counting/treating folders as files
+				if ($data['mimetype'] !== 'httpd/unix-directory') {
+					$this->emit('\OC\Files\Cache\Scanner', 'scanFile', [$file, $this->storageId]);
+					\OC_Hook::emit('\OC\Files\Cache\Scanner', 'scan_file', ['path' => $file, 'storage' => $this->storageId]);
 				}
 
-				return null;
-			}
+				$parent = dirname($file);
+				if ($parent === '.' || $parent === '/') {
+					$parent = '';
+				}
+				if ($parentId === -1) {
+					$parentId = $this->cache->getParentId($file);
+				}
 
-			try {
-				if ($data) {
-					// pre-emit only if it was a file. By that we avoid counting/treating folders as files
-					if ($data['mimetype'] !== 'httpd/unix-directory') {
-						$this->emit('\OC\Files\Cache\Scanner', 'scanFile', [$file, $this->storageId]);
-						\OC_Hook::emit('\OC\Files\Cache\Scanner', 'scan_file', ['path' => $file, 'storage' => $this->storageId]);
+				// scan the parent if it's not in the cache (id -1) and the current file is not the root folder
+				if ($file && $parentId === -1) {
+					$parentData = $this->scanFile($parent);
+					if ($parentData === null) {
+						return null;
 					}
 
-					$parent = dirname($file);
-					if ($parent === '.' || $parent === '/') {
-						$parent = '';
-					}
-					if ($parentId === -1) {
-						$parentId = $this->cache->getParentId($file);
-					}
+					$parentId = $parentData['fileid'];
+				}
+				if ($parent) {
+					$data['parent'] = $parentId;
+				}
 
-					// scan the parent if it's not in the cache (id -1) and the current file is not the root folder
-					if ($file && $parentId === -1) {
-						$parentData = $this->scanFile($parent);
-						if (!$parentData) {
-							return null;
-						}
-						$parentId = $parentData['fileid'];
-					}
-					if ($parent) {
-						$data['parent'] = $parentId;
-					}
-					if (is_null($cacheData)) {
-						/** @var CacheEntry $cacheData */
-						$cacheData = $this->cache->get($file);
-					}
-					if ($cacheData && $reuseExisting && isset($cacheData['fileid'])) {
-						// prevent empty etag
-						$etag = empty($cacheData['etag']) ? $data['etag'] : $cacheData['etag'];
-						$fileId = $cacheData['fileid'];
-						$data['fileid'] = $fileId;
-						// only reuse data if the file hasn't explicitly changed
-						$mtimeUnchanged = isset($data['storage_mtime']) && isset($cacheData['storage_mtime']) && $data['storage_mtime'] === $cacheData['storage_mtime'];
-						// if the folder is marked as unscanned, never reuse etags
-						if ($mtimeUnchanged && $cacheData['size'] !== -1) {
-							$data['mtime'] = $cacheData['mtime'];
-							if (($reuseExisting & self::REUSE_SIZE) && ($data['size'] === -1)) {
-								$data['size'] = $cacheData['size'];
-							}
-							if ($reuseExisting & self::REUSE_ETAG && !$this->storage->instanceOfStorage(IReliableEtagStorage::class)) {
-								$data['etag'] = $etag;
-							}
+				$cacheData = $cacheData ?? $this->cache->get($file);
+				if ($reuseExisting && $cacheData !== false && isset($cacheData['fileid'])) {
+					// prevent empty etag
+					$etag = empty($cacheData['etag']) ? $data['etag'] : $cacheData['etag'];
+					$fileId = $cacheData['fileid'];
+					$data['fileid'] = $fileId;
+					// only reuse data if the file hasn't explicitly changed
+					$mtimeUnchanged = isset($data['storage_mtime']) && isset($cacheData['storage_mtime']) && $data['storage_mtime'] === $cacheData['storage_mtime'];
+					// if the folder is marked as unscanned, never reuse etags
+					if ($mtimeUnchanged && $cacheData['size'] !== -1) {
+						$data['mtime'] = $cacheData['mtime'];
+						if (($reuseExisting & self::REUSE_SIZE) && ($data['size'] === -1)) {
+							$data['size'] = $cacheData['size'];
 						}
-
-						// we only updated unencrypted_size if it's already set
-						if ($cacheData['unencrypted_size'] === 0) {
-							unset($data['unencrypted_size']);
+						if ($reuseExisting & self::REUSE_ETAG && !$this->storage->instanceOfStorage(IReliableEtagStorage::class)) {
+							$data['etag'] = $etag;
 						}
-
-						// Only update metadata that has changed
-						// i.e. get all the values in $data that are not present in the cache already
-						$newData = $this->array_diff_assoc_multi($data, $cacheData->getData());
-						
-						// make it known to the caller that etag has been changed and needs propagation
-						if (isset($newData['etag'])) {
-							$data['etag_changed'] = true;
-						}
-					} else {
-						// we only updated unencrypted_size if it's already set
-						unset($data['unencrypted_size']);
-						$newData = $data;
-						$fileId = -1;
 					}
-					if (!empty($newData)) {
-						// Reset the checksum if the data has changed
-						$newData['checksum'] = '';
-						$newData['parent'] = $parentId;
-						$data['fileid'] = $this->addToCache($file, $newData, $fileId);
-					}
-
-					$data['oldSize'] = ($cacheData && isset($cacheData['size'])) ? $cacheData['size'] : 0;
 
-					if ($cacheData && isset($cacheData['encrypted'])) {
-						$data['encrypted'] = $cacheData['encrypted'];
+					// we only updated unencrypted_size if it's already set
+					if (isset($cacheData['unencrypted_size']) && $cacheData['unencrypted_size'] === 0) {
+						unset($data['unencrypted_size']);
 					}
 
-					// post-emit only if it was a file. By that we avoid counting/treating folders as files
-					if ($data['mimetype'] !== 'httpd/unix-directory') {
-						$this->emit('\OC\Files\Cache\Scanner', 'postScanFile', [$file, $this->storageId]);
-						\OC_Hook::emit('\OC\Files\Cache\Scanner', 'post_scan_file', ['path' => $file, 'storage' => $this->storageId]);
+					/**
+					 * Only update metadata that has changed.
+					 * i.e. get all the values in $data that are not present in the cache already
+					 *
+					 * We need the OC implementation for usage of "getData" method below.
+					 * @var \OC\Files\Cache\CacheEntry $cacheData
+					 */
+					$newData = $this->array_diff_assoc_multi($data, $cacheData->getData());
+					
+					// make it known to the caller that etag has been changed and needs propagation
+					if (isset($newData['etag'])) {
+						$data['etag_changed'] = true;
 					}
 				} else {
-					$this->removeFromCache($file);
+					unset($data['unencrypted_size']);
+					$newData = $data;
+					$fileId = -1;
 				}
-			} catch (\Exception $e) {
-				if ($lock) {
-					if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
-						$this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
-					}
+				if (!empty($newData)) {
+					// Reset the checksum if the data has changed
+					$newData['checksum'] = '';
+					$newData['parent'] = $parentId;
+					$data['fileid'] = $this->addToCache($file, $newData, $fileId);
 				}
-				throw $e;
-			}
 
-			// release the acquired lock
-			if ($lock) {
-				if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
-					$this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
+				if ($cacheData !== false) {
+					$data['oldSize'] = $cacheData['size'] ?? 0;
+					$data['encrypted'] = $cacheData['encrypted'] ?? false;
 				}
-			}
 
-			if ($data && !isset($data['encrypted'])) {
-				$data['encrypted'] = false;
+				// post-emit only if it was a file. By that we avoid counting/treating folders as files
+				if ($data['mimetype'] !== 'httpd/unix-directory') {
+					$this->emit('\OC\Files\Cache\Scanner', 'postScanFile', [$file, $this->storageId]);
+					\OC_Hook::emit('\OC\Files\Cache\Scanner', 'post_scan_file', ['path' => $file, 'storage' => $this->storageId]);
+				}
+			}
+		} finally {
+			// release the acquired lock
+			if ($lock && $this->storage->instanceOfStorage(ILockingStorage::class)) {
+				$this->storage->releaseLock($file, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
 			}
-			return $data;
 		}
 
-		return null;
+		return $data;
 	}
 
 	protected function removeFromCache($path) {
@@ -319,29 +310,26 @@ class Scanner extends BasicEmitter implements IScanner {
 		if ($reuse === -1) {
 			$reuse = ($recursive === self::SCAN_SHALLOW) ? self::REUSE_ETAG | self::REUSE_SIZE : self::REUSE_ETAG;
 		}
-		if ($lock) {
-			if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
-				$this->storage->acquireLock('scanner::' . $path, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider);
-				$this->storage->acquireLock($path, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
-			}
+
+		if ($lock && $this->storage->instanceOfStorage(ILockingStorage::class)) {
+			$this->storage->acquireLock('scanner::' . $path, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider);
+			$this->storage->acquireLock($path, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
 		}
+
 		try {
-			try {
-				$data = $this->scanFile($path, $reuse, -1, null, $lock);
-				if ($data && $data['mimetype'] === 'httpd/unix-directory') {
-					$size = $this->scanChildren($path, $recursive, $reuse, $data['fileid'], $lock, $data['size']);
-					$data['size'] = $size;
-				}
-			} catch (NotFoundException $e) {
-				$this->removeFromCache($path);
-				return null;
+			$data = $this->scanFile($path, $reuse, -1, lock: $lock);
+
+			if ($data !== null && $data['mimetype'] === 'httpd/unix-directory') {
+				$size = $this->scanChildren($path, $recursive, $reuse, $data['fileid'], $lock, $data['size']);
+				$data['size'] = $size;
 			}
+		} catch (NotFoundException $e) {
+			$this->removeFromCache($path);
+			return null;
 		} finally {
-			if ($lock) {
-				if ($this->storage->instanceOfStorage(ILockingStorage::class)) {
-					$this->storage->releaseLock($path, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
-					$this->storage->releaseLock('scanner::' . $path, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider);
-				}
+			if ($lock && $this->storage->instanceOfStorage(ILockingStorage::class)) {
+				$this->storage->releaseLock($path, ILockingProvider::LOCK_SHARED, $this->lockingProvider);
+				$this->storage->releaseLock('scanner::' . $path, ILockingProvider::LOCK_EXCLUSIVE, $this->lockingProvider);
 			}
 		}
 		return $data;
@@ -395,9 +383,9 @@ class Scanner extends BasicEmitter implements IScanner {
 	 * Get the children currently in the cache
 	 *
 	 * @param int $folderId
-	 * @return array[]
+	 * @return array<string, \OCP\Files\Cache\ICacheEntry>
 	 */
-	protected function getExistingChildren($folderId) {
+	protected function getExistingChildren($folderId): array {
 		$existingChildren = [];
 		$children = $this->cache->getFolderContentsById($folderId);
 		foreach ($children as $child) {
diff --git a/lib/private/Files/Cache/Wrapper/CacheJail.php b/lib/private/Files/Cache/Wrapper/CacheJail.php
index ea0f992114a..5c7bd4334f3 100644
--- a/lib/private/Files/Cache/Wrapper/CacheJail.php
+++ b/lib/private/Files/Cache/Wrapper/CacheJail.php
@@ -21,19 +21,15 @@ use OCP\Files\Search\ISearchOperator;
  * Jail to a subdirectory of the wrapped cache
  */
 class CacheJail extends CacheWrapper {
-	/**
-	 * @var string
-	 */
-	protected $root;
-	protected $unjailedRoot;
+
+	protected string $unjailedRoot;
 
 	public function __construct(
 		?ICache $cache,
-		string $root,
+		protected string $root,
 		?CacheDependencies $dependencies = null,
 	) {
 		parent::__construct($cache, $dependencies);
-		$this->root = $root;
 
 		if ($cache instanceof CacheJail) {
 			$this->unjailedRoot = $cache->getSourcePath($root);
@@ -42,6 +38,9 @@ class CacheJail extends CacheWrapper {
 		}
 	}
 
+	/**
+	 * @return string
+	 */
 	protected function getRoot() {
 		return $this->root;
 	}
@@ -55,7 +54,10 @@ class CacheJail extends CacheWrapper {
 		return $this->unjailedRoot;
 	}
 
-	protected function getSourcePath($path) {
+	/**
+	 * @return string
+	 */
+	protected function getSourcePath(string $path) {
 		if ($path === '') {
 			return $this->getRoot();
 		} else {
@@ -95,7 +97,7 @@ class CacheJail extends CacheWrapper {
 	/**
 	 * get the stored metadata of a file or folder
 	 *
-	 * @param string /int $file
+	 * @param string|int $file
 	 * @return ICacheEntry|false
 	 */
 	public function get($file) {
@@ -206,12 +208,12 @@ class CacheJail extends CacheWrapper {
 	/**
 	 * update the folder size and the size of all parent folders
 	 *
-	 * @param string|boolean $path
-	 * @param array $data (optional) meta data of the folder
+	 * @param array|ICacheEntry|null $data (optional) meta data of the folder
 	 */
-	public function correctFolderSize($path, $data = null, $isBackgroundScan = false) {
-		if ($this->getCache() instanceof Cache) {
-			$this->getCache()->correctFolderSize($this->getSourcePath($path), $data, $isBackgroundScan);
+	public function correctFolderSize(string $path, $data = null, bool $isBackgroundScan = false): void {
+		$cache = $this->getCache();
+		if ($cache instanceof Cache) {
+			$cache->correctFolderSize($this->getSourcePath($path), $data, $isBackgroundScan);
 		}
 	}
 
@@ -223,8 +225,9 @@ class CacheJail extends CacheWrapper {
 	 * @return int|float
 	 */
 	public function calculateFolderSize($path, $entry = null) {
-		if ($this->getCache() instanceof Cache) {
-			return $this->getCache()->calculateFolderSize($this->getSourcePath($path), $entry);
+		$cache = $this->getCache();
+		if ($cache instanceof Cache) {
+			return $cache->calculateFolderSize($this->getSourcePath($path), $entry);
 		} else {
 			return 0;
 		}
diff --git a/lib/private/Files/Cache/Wrapper/CacheWrapper.php b/lib/private/Files/Cache/Wrapper/CacheWrapper.php
index fdaa2cf4b7a..f2f1036d6a3 100644
--- a/lib/private/Files/Cache/Wrapper/CacheWrapper.php
+++ b/lib/private/Files/Cache/Wrapper/CacheWrapper.php
@@ -221,12 +221,12 @@ class CacheWrapper extends Cache {
 	/**
 	 * update the folder size and the size of all parent folders
 	 *
-	 * @param string|boolean $path
-	 * @param array $data (optional) meta data of the folder
+	 * @param array|ICacheEntry|null $data (optional) meta data of the folder
 	 */
-	public function correctFolderSize($path, $data = null, $isBackgroundScan = false) {
-		if ($this->getCache() instanceof Cache) {
-			$this->getCache()->correctFolderSize($path, $data, $isBackgroundScan);
+	public function correctFolderSize(string $path, $data = null, bool $isBackgroundScan = false): void {
+		$cache = $this->getCache();
+		if ($cache instanceof Cache) {
+			$cache->correctFolderSize($path, $data, $isBackgroundScan);
 		}
 	}
 
@@ -238,8 +238,9 @@ class CacheWrapper extends Cache {
 	 * @return int|float
 	 */
 	public function calculateFolderSize($path, $entry = null) {
-		if ($this->getCache() instanceof Cache) {
-			return $this->getCache()->calculateFolderSize($path, $entry);
+		$cache = $this->getCache();
+		if ($cache instanceof Cache) {
+			return $cache->calculateFolderSize($path, $entry);
 		} else {
 			return 0;
 		}
diff --git a/lib/private/Files/ObjectStore/ObjectStoreScanner.php b/lib/private/Files/ObjectStore/ObjectStoreScanner.php
index d8a77d36dee..05929a49aab 100644
--- a/lib/private/Files/ObjectStore/ObjectStoreScanner.php
+++ b/lib/private/Files/ObjectStore/ObjectStoreScanner.php
@@ -13,11 +13,11 @@ use OCP\Files\FileInfo;
 
 class ObjectStoreScanner extends Scanner {
 	public function scanFile($file, $reuseExisting = 0, $parentId = -1, $cacheData = null, $lock = true, $data = null) {
-		return [];
+		return null;
 	}
 
 	public function scan($path, $recursive = self::SCAN_RECURSIVE, $reuse = -1, $lock = true) {
-		return [];
+		return null;
 	}
 
 	protected function scanChildren(string $path, $recursive, int $reuse, int $folderId, bool $lock, int|float $oldSize, &$etagChanged = false) {
diff --git a/lib/private/Files/View.php b/lib/private/Files/View.php
index 072d3520ae9..5e09ce69fb2 100644
--- a/lib/private/Files/View.php
+++ b/lib/private/Files/View.php
@@ -610,13 +610,13 @@ class View {
 				$this->lockFile($path, ILockingProvider::LOCK_SHARED);
 
 				$exists = $this->file_exists($path);
-				$run = true;
 				if ($this->shouldEmitHooks($path)) {
+					$run = true;
 					$this->emit_file_hooks_pre($exists, $path, $run);
-				}
-				if (!$run) {
-					$this->unlockFile($path, ILockingProvider::LOCK_SHARED);
-					return false;
+					if (!$run) {
+						$this->unlockFile($path, ILockingProvider::LOCK_SHARED);
+						return false;
+					}
 				}
 
 				try {