thirdparty/scummvm
2
0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

SCI: Dispose uncached volume file streams

Browse source 
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.
This commit is contained in:
Colin Snover 2017-05-13 22:07:53 -05:00
parent 1911b19e15
commit f44d8b6da6
3 changed files with 36 additions and 13 deletions
Download patch file Download diff file Expand all files Collapse all files

29
engines/sci/resource.cpp
View file

@ -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;

6
engines/sci/resource.h
View file

@ -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;

14
engines/sci/resource_audio.cpp
View file

@ -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() {