Opened 14 years ago

Closed 14 years ago

Last modified 21 months ago

#2599 closed defect (fixed)

ALL: Valgrind warnings in our file-handling classes

Reported by: eriktorbjorn Owned by: eriktorbjorn
Priority: normal Component: Common
Keywords: Cc:
Game: Kyrandia 1

Description

I just had a strange problem when trying to start Kyra 1. Two or three times in a row, ScummVM complained that it couldn't find any game in the specified directory. I haven't been able to reproduce that since then, but it did prompt me to run Valgrind.

It seems that when starting *some* games, Valgrind will warn about uninitialized variable(s) in our file-handling classes. The warnings seem to indicate that _isDirectory isn't initialized, but so far I've been unable to figure out why. Maybe Valgrind is imagining things, but that doesn't explain the problem I had...

Since Valgrind is very slow, I only tried a couple of games. I got warning messages for Kyra 1, Broken Sword 2 and Simon the Sorcerer 1, but not for Monkey Island 2, Beneath a Steel Sky or Flight of the Amazon Queen.

There seemed to be two different warnings: One about File::exists(), and one about FilesystemNode::listDir().

Example warnings:

==31046== Conditional jump or move depends on uninitialised value(s) ==31046== at 0x81059F7: Common::File::exists(Common::String const&) (file.cpp:312) ==31046== by 0x4D9F6CD: Kyra::KyraEngine_v1::setupGameFlags() (plugin.cpp:276) ==31046== by 0x4D9DE07: Kyra::KyraEngine::init() (kyra.cpp:109) ==31046== by 0x807627B: runGame(Plugin const*, OSystem&, Common::String const&) (main.cpp:244) ==31046== by 0x8076EFE: scummvm_main (main.cpp:362) ==31046== by 0x8074271: main (sdl.cpp:99)

==31834== at 0x80FD382: FilesystemNode::listDir(FSList&, FilesystemNode::ListMode) const (fs.cpp:103) ==31834== by 0x8104C8F: Common::File::addDefaultDirectoryRecursive(FilesystemNode const&, int) (file.cpp:128) ==31834== by 0x8104F52: Common::File::addDefaultDirectory(Common::String const&) (file.cpp:111) ==31834== by 0x4CB761A: Sword2::Sword2Engine::Sword2Engine(OSystem*) (sword2.cpp:126) ==31834== by 0x4CB798F: Engine_SWORD2_create(OSystem*, Engine**) (sword2.cpp:116) ==31834== by 0x4CB79C7: PLUGIN_createEngine (sword2.cpp:120) ==31834== by 0x807D7FA: DynamicPlugin::createInstance(OSystem*, Engine**) const (plugins.cpp:162) ==31834== by 0x8075CB7: runGame(Plugin const*, OSystem&, Common::String const&) (main.cpp:201) ==31834== by 0x8076EFE: scummvm_main (main.cpp:362) ==31834== by 0x8074271: main (sdl.cpp:99)

Ticket imported from: #1483450. Ticket imported from: bugs/2599.

Change History (6)

comment:1 by eriktorbjorn, 14 years ago

Just to clarify, this is with a freshly compiled SVN snapshot.

comment:2 by eriktorbjorn, 14 years ago

Also, it's a Linux box so presumably it's the POSIX filesystem classes that are used.

comment:3 by fingolfin, 14 years ago

Odd. POSIXFilesystemNode has two constructors, and both _isDirectory. Of course, the errors are triggered outside of class POSIXFilesystemNode. Maybe the _realNode pointer in class FilesystemNode contains a bogus value? Maybe a refcounting bug ??? Hurm. I commited two paranoid checks to SVN, but I doubt that'll catch anything (but it's worth a short anyway).

comment:4 by eriktorbjorn, 14 years ago

That didn't make any difference, but I believe I see the problem. In the second POSIXFilesystemNode, we have the following code:

if (verify) { struct stat st; _isValid = (0 == stat(_path.c_str(), &st)); _isDirectory = _isValid ? S_ISDIR(st.st_mode); }

(Slightly simplified. There's an #ifdef ... #else ... #endif in there, but that's not relevant to the discussion.)

The C library and Linux kernel are big, scary beasts, but from what I understand, the C library itself never initialises 'st', and the kernel only does when vfs_stat_fd() returns 0, i.e. when _isValid is true.

I've changed it so that if _isValid is false, then _isDirectory is also false. That made the warning go away.

(If I change it so that _isDirectory is true when _isValid is false, Kyra 1 fails to start. The error message is the same as the one I saw yesterday.)

There were a few other places where we used S_ISDIR(). I've changed them, too, to check if stat() succeeded, even if I don't know if it's necessary in those cases.

comment:5 by eriktorbjorn, 14 years ago

Owner: set to eriktorbjorn
Resolution: fixed
Status: newclosed

comment:6 by digitall, 21 months ago

Component: --Unset--Common
Game: Kyrandia 1
Note: See TracTickets for help on using tickets.