From f44d8b6da69a1444c76fcbce28a834f2368ddf35 Mon Sep 17 00:00:00 2001 From: Colin Snover Date: Sat, 13 May 2017 22:07:53 -0500 Subject: 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. --- engines/sci/resource.cpp | 29 +++++++++++++++++++++++------ engines/sci/resource.h | 6 ++++++ engines/sci/resource_audio.cpp | 14 +++++++------- 3 files changed, 36 insertions(+), 13 deletions(-) (limited to 'engines/sci') 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(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_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() { -- cgit v1.2.3