SCI: Dispose uncached volume file streams

The stream returned by a call to ResourceManager::getVolumeFile
either MUST (when returning an I/O stream from a Common::FSNode)
or must NOT (when returning a Common::File *) be deleted by the
caller, depending upon some internal implementation details of
ResourceSource that should never have been exposed to callers.

FSNode streams that should have been deleted were not being
deleted all the time, which leaked and eventually caused ScummVM
to run out of FDs.

This commit improves this situation by shielding callers from
these internal details by centralizing the destruction logic in
one place, so FSNode read streams stop being leaked and callers
no longer need to know stuff about the internals of the
ResourceSource they are trying to read in order to avoid leaking
or breaking the volume file cache.

Fixes Trac#9782.

3 files changed, 36 insertions, 13 deletions

diff --git a/engines/sci/resource.cpp b/engines/sci/resource.cpp
index 6ee77fd4c7..f1b2a58577 100644
--- a/engines/sci/resource.cpp
+++ b/engines/sci/resource.cpp
@@ -387,6 +387,24 @@ Common::SeekableReadStream *ResourceManager::getVolumeFile(ResourceSource *sourc
 	return NULL;
 }
 
+void ResourceManager::disposeVolumeFileStream(Common::SeekableReadStream *fileStream, Sci::ResourceSource *source) {
+#ifdef ENABLE_SCI32
+	ChunkResourceSource *chunkSource = dynamic_cast<ChunkResourceSource *>(source);
+	if (chunkSource != nullptr) {
+		delete fileStream;
+		return;
+	}
+#endif
+
+	if (source->_resourceFile) {
+		delete fileStream;
+		return;
+	}
+
+	// Other volume file streams are cached in _volumeFiles and should only be
+	// deleted from _volumeFiles
+}
+
 void ResourceManager::loadResource(Resource *res) {
 	res->_source->loadResource(this, res);
 }
@@ -575,8 +593,7 @@ void ResourceSource::loadResource(ResourceManager *resMan, Resource *res) {
 		res->unalloc();
 	}
 
-	if (_resourceFile)
-		delete fileStream;
+	resMan->disposeVolumeFileStream(fileStream, this);
 }
 
 Resource *ResourceManager::testResource(ResourceId id) {
@@ -2041,6 +2058,7 @@ Resource *ResourceManager::updateResource(ResourceId resId, ResourceSource *src,
 	if (avSrc != nullptr && !avSrc->relocateMapOffset(offset, size)) {
 		warning("Compressed volume %s does not contain a valid entry for %s (map offset %u)", src->getLocationName().c_str(), resId.toString().c_str(), offset);
 		_hasBadResources = true;
+		disposeVolumeFileStream(volumeFile, src);
 		return res;
 	}
 
@@ -2059,6 +2077,7 @@ Resource *ResourceManager::updateResource(ResourceId resId, ResourceSource *src,
 		_hasBadResources = true;
 	}
 
+	disposeVolumeFileStream(volumeFile, src);
 	return res;
 }
 
@@ -2252,13 +2271,11 @@ ResourceCompression ResourceManager::getViewCompression() {
 		ResourceCompression compression;
 
 		if (res->readResourceInfo(_volVersion, fileStream, szPacked, compression)) {
-			if (res->_source->_resourceFile)
-				delete fileStream;
+			disposeVolumeFileStream(fileStream, res->_source);
 			continue;
 		}
 
-		if (res->_source->_resourceFile)
-			delete fileStream;
+		disposeVolumeFileStream(fileStream, res->_source);
 
 		if (compression != kCompNone)
 			return compression;
diff --git a/engines/sci/resource.h b/engines/sci/resource.h
index 2bbbd42a4a..1a10fb2ccd 100644
--- a/engines/sci/resource.h
+++ b/engines/sci/resource.h
@@ -516,7 +516,13 @@ protected:
 	 */
 	const char *versionDescription(ResVersion version) const;
 
+	/**
+	 * All calls to getVolumeFile must be followed with a corresponding
+	 * call to disposeVolumeFileStream once the stream is finished being used.
+	 * Do NOT call delete directly on returned streams, as they may be cached.
+	 */
 	Common::SeekableReadStream *getVolumeFile(ResourceSource *source);
+	void disposeVolumeFileStream(Common::SeekableReadStream *fileStream, ResourceSource *source);
 	void loadResource(Resource *res);
 	void freeOldResources();
 	bool validateResource(const ResourceId &resourceId, const Common::String &sourceMapLocation, const Common::String &sourceName, const uint32 offset, const uint32 size, const uint32 sourceSize) const;
diff --git a/engines/sci/resource_audio.cpp b/engines/sci/resource_audio.cpp
index 0b00725d8b..7553efa452 100644
--- a/engines/sci/resource_audio.cpp
+++ b/engines/sci/resource_audio.cpp
@@ -43,7 +43,7 @@ AudioVolumeResourceSource::AudioVolumeResourceSource(ResourceManager *resMan, co
 	 * table for later usage.
 	 */
 
-	Common::SeekableReadStream *fileStream = getVolumeFile(resMan, 0);
+	Common::SeekableReadStream *fileStream = getVolumeFile(resMan, nullptr);
 	if (!fileStream)
 		return;
 
@@ -75,8 +75,7 @@ AudioVolumeResourceSource::AudioVolumeResourceSource(ResourceManager *resMan, co
 		lastEntry->size = fileStream->size() - lastEntry->offset;
 	}
 
-	if (_resourceFile)
-		delete fileStream;
+	resMan->disposeVolumeFileStream(fileStream, this);
 }
 
 bool Resource::loadFromWaveFile(Common::SeekableReadStream *file) {
@@ -320,6 +319,7 @@ int ResourceManager::readAudioMapSCI11(IntMapResourceSource *map) {
 	}
 
 	const uint32 srcSize = fileStream->size();
+	disposeVolumeFileStream(fileStream, src);
 
 	SciSpan<const byte>::const_iterator ptr = mapRes->cbegin();
 
@@ -400,6 +400,8 @@ int ResourceManager::readAudioMapSCI11(IntMapResourceSource *map) {
 			}
 			addResource(audioResId, src, offset, size, map->getLocationName());
 		}
+
+		disposeVolumeFileStream(stream, src);
 	} else {
 		bool isEarly = (entrySize != 11);
 
@@ -928,8 +930,7 @@ void WaveResourceSource::loadResource(ResourceManager *resMan, Resource *res) {
 
 	fileStream->seek(res->_fileOffset, SEEK_SET);
 	res->loadFromWaveFile(fileStream);
-	if (_resourceFile)
-		delete fileStream;
+	resMan->disposeVolumeFileStream(fileStream, this);
 }
 
 void AudioVolumeResourceSource::loadResource(ResourceManager *resMan, Resource *res) {
@@ -951,8 +952,7 @@ void AudioVolumeResourceSource::loadResource(ResourceManager *resMan, Resource *
 	else
 		res->loadFromAudioVolumeSCI11(fileStream);
 
-	if (_resourceFile)
-		delete fileStream;
+	resMan->disposeVolumeFileStream(fileStream, this);
 }
 
 bool ResourceManager::addAudioSources() {