[Scummvm-tracker] [ScummVM :: Bugs] #17287: COMMON: GZipReadStream::seek() behavior on invalid resources created by ScummVM

[ScummVM :: Bugs]

#17287: COMMON: GZipReadStream::seek() behavior on invalid resources created by
ScummVM
---------------------+-----------------------
Reporter:  dwatteau  |      Owner:  (none)
    Type:  defect    |     Status:  new
Priority:  normal    |  Component:  Common
 Version:            |   Keywords:  gzip,zlib
    Game:            |
---------------------+-----------------------
 This was triggered on current Git HEAD, after the rewrite of the MADS
 engine, and with an old autosave of Rex Nebular (attached).

 This `nebular.000` save is from 2022-07-27, and was a Gzip stream made
 corrupt because of a ScummVM bug; before MADS bug #13642 was fixed, it'd
 try to write an autosave during a cutscene (and this shouldn't happen for
 this game) but then ScummVM would abort after having written this invalid
 save (and always left this file in place -- this was before any attempt to
 have any atomic stream, I think).

 If the stream is corrupted, calling `assert()` may not seem like a bad
 idea, but:

 1. what about non-debug builds?
 2. it's not really a "it shouldn't happen" case; ScummVM would produce
 such files itself, in some circumstances. And then it'd try to read them
 back.
 3. maybe a better error interface than a plain `assert()` is possible, in
 this case?

 Anyway, if you start Rex Nebular with this autosave nearby, debug builds
 will hit this:

 {{{
 User picked target 'nebular' (engine ID 'mads', game ID 'nebular')...
 Running Rex Nebular and the Cosmic Gender Bender (DOS/English)
 section1.hag: 6f725eb38660de8af31ec7cdd628d615, 927222 bytes.

 Assertion failed: (newPos >= 0), function seek, file zlib.cpp, line 237.
 Process 85645 stopped
   - thread #1, queue = 'com.apple.main-thread', stop reason = hit program
 assert
      frame #4: 0x00000001006ce7ac
 scummvm`Common::GZipReadStream::seek(this=0x00000007e873c000, offset=-4,
 whence=2) at zlib.cpp:237:3
      234                 break;
      235             }
      236
   -> 237             assert(newPos >= 0);
      238
      239             if ((uint32)newPos < _pos) {
      240                 // To search backward, we have to restart the
 whole decompression
     Target 0: (scummvm) stopped.
 }}}

 In this case, `newPos` is a negative value because a value bigger than
 INT32_MAX was read, but `newPos` still is an `int32`.

 By making it an `int64`, we avoid aborting ScummVM every time such a save
 is encountered (maybe a debug()/warning() line there would be helpful?).
 Also, returning an error instead of calling `assert()` may be a cleaner
 interface (but I can't say if all callers properly handle its errors).

 {{{
         bool seek(int64 offset, int whence = SEEK_SET) override {
 -               int32 newPos = 0;
 +               int64 newPos = 0;
                 switch (whence) {
                 default:
                         // fallthrough intended
 @@ -234,9 +234,14 @@ public:
                         break;
                 }

 -               assert(newPos >= 0);
 +               // Offsets and sizes may come from bogus data (e.g. the
 gzip trailer of
 +               // a truncated file for SEEK_END), so fail instead of
 asserting
 +               if (newPos < 0)
 +                       return false;

 -               if ((uint32)newPos < _pos) {
 +               if (newPos < _pos) {
                         // To search backward, we have to restart the
 whole decompression
                         // from the start of the file. A rather wasteful
 operation, best
                         // to avoid it. :/
 }}}

 I'm not submitting a PR, and I just report my findings here, because I'm
 not sure about the impacts of such a change.
-- 
Ticket URL: <https://bugs.scummvm.org/ticket/17287>
ScummVM :: Bugs <https://bugs.scummvm.org>
ScummVM