search

stachenov / quazip

Qt/C++ wrapper over minizip
Other
582 stars 232 forks source link

Crash: Calling QuaZip::close() twice while QuaZip has a file I/O error can access dangling internal file pointers #125

Closed hinrichsenhans closed 3 years ago

hinrichsenhans commented 3 years ago

Quick overview QuaZip::close() may not be safely called twice (once explicitly, a second time by the d'tor) in some cases where there is a file I/O error, because some internal pointers will be invalidated after the first close() call and close will not set itself back to the mdNotOpen mode.

More in-depth I recently received a crash report that's been easy to reproduce in my project, but difficult to reproduce outside of it.

I am using QuaZip 0.8.3, but the master branch appears to be the same. Windows platform (7 and 10), MSVC 2019, Qt 5.15.3. x64 architecture

The original field report is that if we are writing a zip file to a flash drive, and the flash drive is removed during the process, QuaZip will cause a crash with a dangling pointer.

I've stepped into the code, and while I don't have a full grasp on the internals, I believe the following is happening:

  1. QuaZip is instantiated on the stack with a path to a flash drive
  2. The file is opened with mdCreate
  3. We write some contents to the file before it is pulled, instantiating QuaZipFile objects and doing the file I/O there
  4. User pulls the flash drive
  5. We continue to write contents, which fail in some way (very expected!)
  6. We explicitly call QuaZip::close() 6b. QuaZip close() will have p->zipError set to -1 / 0xFFFFFFFF 6c. Because we opened with a file name, p->ioDevice will be deleted 6d. Because we have a zipError, we do NOT set p->mode back to mdNotOpen
  7. QuaZip falls out of scope 7b. QuaZip still believes it is open, and attempts to close the file again 7c. Some internal file handles are now dangling, and will be accessed 7d. Specifically, we crash in qiodevice64_tell_file_func when iodevice->isSequential() is called. 7e. Callstack is:

-> qiodevice64_tell_file_func(void opaque, void stream) zipClose(void file, const char global_comment) QuaZip::close() QuaZip::~QuaZip() ...

I have been unable to reproduce in a separate app using much of the same code, so there may be something about my environment that is off.

Generically, I do believe that close() should be always setting p->mode to mdNotOpen. Even if it fails, we should not attempt to reference freed resources again later.