Opened 14 years ago

Closed 14 years ago

Last modified 13 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, 13 months ago

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