#2599 closed defect (fixed)
ALL: Valgrind warnings in our file-handling classes
| Reported by: | eriktorbjorn | Owned by: | eriktorbjorn |
|---|---|---|---|
| Priority: | normal | Component: | Common |
| Version: | 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 , 19 years ago
comment:2 by , 19 years ago
Also, it's a Linux box so presumably it's the POSIX filesystem classes that are used.
comment:3 by , 19 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 , 19 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 , 19 years ago
| Owner: | set to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
comment:6 by , 6 years ago
| Component: | --Unset-- → Common |
|---|---|
| Game: | → Kyrandia 1 |
Just to clarify, this is with a freshly compiled SVN snapshot.