[Scummvm-tracker] [ScummVM :: Bugs] #16163: MACVENTURE: Container class seems to access file out of bounds

[ScummVM :: Bugs]

#16163: MACVENTURE: Container class seems to access file out of bounds
-------------------------+-----------------------
Reporter:  eriktorbjorn  |      Owner:  (none)
    Type:  defect        |     Status:  new
Priority:  normal        |  Component:  --Other--
 Version:                |   Keywords:
    Game:                |
-------------------------+-----------------------
 I realize the MacVenture engine is still work in progress, but I didn't
 know where else to report this finding. Running Deja Vu, I was getting
 Valgrind warnings like this:

 {{{
 ==133413== Conditional jump or move depends on uninitialised value(s)
 ==133413==    at 0x541F030: MacVenture::Container::Container(Common::Path
 const&) (container.cpp:112)
 ==133413==    by 0x542437E: MacVenture::Gui::loadGraphics() (gui.cpp:478)
 ==133413==    by 0x5422DEB: MacVenture::Gui::initGUI() (gui.cpp:173)
 ==133413==    by 0x5422B19:
 MacVenture::Gui::Gui(MacVenture::MacVentureEngine*,
 Common::MacResManager*) (gui.cpp:133)
 ==133413==    by 0x543187F: MacVenture::MacVentureEngine::run()
 (macventure.cpp:146)
 ==133413==    by 0x2C101D5: runGame(Plugin const*, OSystem&, DetectedGame
 const&, void const*) (main.cpp:317)
 ==133413==    by 0x2C1263D: scummvm_main (main.cpp:803)
 ==133413==    by 0x2C0D0D4: main (posix-main.cpp:44)
 ==133413==  Uninitialised value was created by a stack allocation
 ==133413==    at 0x2C6E79C: Common::ReadStream::readUint32BE()
 (stream.h:515)
 }}}

 (Thank heavens for --track-origins=yes because I was completely baffled
 before I added that!)

 It appears that this code in container.cpp

 {{{
                                 mask = _res->readUint32BE();
                                 mask >>= (16 - bits);
                                 mask &= 0xFFFF;
                                 debugC(4, kMVDebugContainer, "Load mask of
 object &%d:%d is %x", i, j, mask);
                                 _res->seek(-4, SEEK_CUR);
 }}}

 is reading 4 bytes from the stream, extracting the bits it wants, and then
 re-positions the stream again. But there may not be four bytes left to
 read in the stream, and then the value may be at least partly
 uninitialized.

 It should probably use a bit reader instead, but I don't understand the
 code well enough to suggest exactly how it should look.
-- 
Ticket URL: <https://bugs.scummvm.org/ticket/16163>
ScummVM :: Bugs <https://bugs.scummvm.org>
ScummVM