Clarify who owns fp

[christian-intra2net]

In the current implementation, OleFileIO always closes self.fp in close(), even if fp was provided by the caller during OleFileIO.open().

That means in essence that OleFileIO "takes ownership" of the file handle it was given, and takes responsibility to close it if something went wrong. The caller just has the responsibility to call OleFileIO.close().

However, many callers do not call close, some forget it in case of error, some others forget it even in regular use (e.g. in the example code in the doc string of OleFileIO).

The result is a file handle that has to be closed by the garbage collector upon destruction of the OleFileIO instance, which is (1) bad style and therefore (2) raises a ResourceWarning in newer python versions.

I have played around and came up with 2 possible solutions:

(1) leave responsibility for resources to the one creating them. That means that if the caller opened the file and provided the handle as argument to OleFileIO.open() or its constructor, we do not close it in OleFileIO.close(). In this case, the caller does not necessarily have to call the OleFileIO.close() function. She just has to ensure that the file handle itself is closed. However, if the OleFileIO was opened with a file name as argument, we have to ensure this is closed, so I would create a constructor which closes the file handle in this case.

(2) We take ownership. In that case I would clearly state in the doc that callers must ensure that the OleFileIO is closed. I would create a destructor which closes the file_handle if it is still open and issue a warning in this case to get people a hint where in their code they forgot a call to OleFileIO.close()

In both scenarios, there is one case which is problematic: If a filename is given to the OleFileIO constructor, then it calls open() at the end of the constructor. This can easily fail if the file is not an OLE file. In that case, the owner of the resource does not really exist (since construction failed after open()ing the file handle), so no-one is responsible for closing the handle. I would add a try-finally around the call to open() in the constructor to close the handle if we opened it.

What do you think of this? Which solution do you prefer? I am happy to implement either one.

[christian-intra2net]

note: I pushed my unfinished playing-code to a branch in my fork of olefile. You can see the commits here. Installing this code gets rid of the ResourceWarnings issued e.g. when running the oletools unit tests

[decalage2]

Good catch, I think solution 1 looks better.

[christian-intra2net]

Implemented solution 1 in PR #121

[christian-intra2net]

Solved as far as I can tell