[Fixed] Ogre::FileSystemArchive issues

[Eugene]

One of updates for our project (Live Interior 3D) does not passed Apple AppStore validation process due to the problems with Ogre::FileSystemArchive implementation. Specifically, application should not try to open its own resources in application bundle in read/write mode, or validation process would be failed. It`s similar to writing to C:\Program Files\ in Windows world.

Ogre::FileSystemArchive::load() tries to create temporary file to determine is it created in readonly or readwrite mode. This mechanism is bad not only because it brokes Apple`s guidelines but also it is unreliable (RO/RW mode and access rights for subfolders could be different from RO/RW mode and access rights for parent folder) and still have unnecessary file creation overhead.

Additionally Ogre::FileSystemArchive::open() has bug in implementation that always open stream in readonly mode if location was determined to be read-write

Code: Select all

	DataStreamPtr FileSystemArchive::open(const String& filename, bool readOnly) const
	{
		<...>
		if (!readOnly && isReadOnly())
		{
			mode |= std::ios::out;
			rwStream = OGRE_NEW_T(std::fstream, MEMCATEGORY_GENERAL)();
			rwStream->open(full_path.c_str(), mode);
			baseStream = rwStream;
		}
		else
		{
			roStream = OGRE_NEW_T(std::ifstream, MEMCATEGORY_GENERAL)();
			roStream->open(full_path.c_str(), mode);
			baseStream = roStream;
		}

To avoid all those problems, RO/RW mode should be passed explicitly via additional flag:

Code: Select all

ResourceGroupManager::addResourceLocation(..., bool readOnly = true)
    ArchiveManager::load(..., bool readOnly)
        FileSystemArchiveFactory::createInstance(..., bool readOnly)
            FileSystemArchive::FileSystemArchive(..., bool readOnly)

instead of unnecessary and error-prone runtime detection.

To fix bug in FileSystemArchive::open() code above should be replaced with

Code: Select all

	DataStreamPtr FileSystemArchive::open(const String& filename, bool readOnly) const
	{
		<...>
		if (!readOnly && isReadOnly())
		{
			OGRE_EXCEPT(Exception::ERR_INVALIDPARAMS, 
				"Cannot open a file in read-write mode in a read-only archive", 
				"FileSystemArchive::open");
		}

		if(!readOnly)
		{
			mode |= std::ios::out;
			rwStream = OGRE_NEW_T(std::fstream, MEMCATEGORY_GENERAL)();
			rwStream->open(full_path.c_str(), mode);
			baseStream = rwStream;
		}
		else
		{
			roStream = OGRE_NEW_T(std::ifstream, MEMCATEGORY_GENERAL)();
			roStream->open(full_path.c_str(), mode);
			baseStream = roStream;
		}

[Klaim]

To avoid all those problems, RO/RW mode should be passed explicitly via additional flag:

May I suggest to use an enum instead of a bool, to make it more clear when you read the calling code?

[Eugene]

Not yet fixed.

Ogre::FileSystemArchive::load() tries to create temporary file to determine is it created in readonly or readwrite mode. This mechanism is bad not only because it brokes Apple`s guidelines and fails AppStore validation process but also it is unreliable (RO/RW mode and access rights for subfolders could be different from RO/RW mode and access rights for parent folder) and still have unnecessary file creation overhead.

[masterfalcon]

Yes, you are correct. Sorry, been busy. I was also kind of waiting to see if you had an opinion on Klaim's suggestion.

[Eugene]

Yes, you are correct. Sorry, been busy. I was also kind of waiting to see if you had an opinion on Klaim's suggestion.

It`s excellent suggestion. But I`m only reporter here, as my problem is already solved by commenting out this RW check, and I don`t want to implement generic fix and go through the Ogre bureaucracy after half year attempts to push WinRT patches - at least not in this decade.

[masterfalcon]

How's this?

[Eugene]

How's this?

It seems not very good to force Ogre::FactoryObj::createInstance() to have extra parameter readOnly. As call stack is

Code: Select all

ResourceGroupManager::addResourceLocation(..., bool readOnly = true)
    ArchiveManager::load(..., bool readOnly)
        FileSystemArchiveFactory::createInstance(..., bool readOnly)
            FileSystemArchive::FileSystemArchive(..., bool readOnly)

than may be better add createInstance(name, readOnly) to the class ArchiveFactory rather than FactoryObj,
and ArchiveManager::load would call this method instead of createInstance(name) as it knows that it has at least ArchiveFactory or derived.
Default implementation for ArchiveFactory::createInstance(name, readOnly) could assert that readOnly is true and invoke createInstance(name)

[Eugene]

How's this?

Here https://bitbucket.org/sinbad/ogre/pull- ... -only/diff is revised patch, where readOnly flag is added to ArchiveFactory rather than to FactoryObj.