Basedir possibly added twice

[fabiangreffrath]

Hi there,

when run as quake2 +set basedir /usr/share/games/quake2, i.e. with basedir set to the same directory that is already defined as SYSTEMDIR, this directory is added twice:

$ quake2 +set basedir /usr/share/games/quake2

Yamagi Quake II v5.31
=====================

Client build options:
 + SDL2
 - CD audio
 + OGG/Vorbis
 + OpenAL audio
 + Zip file support
Platform: Linux
Architecture: amd64
Byte ordering: little endian

Using /usr/share/games/quake2//baseq2/ to fetch paks
Added packfile '/usr/share/games/quake2//baseq2/pak0.pak' (3386 files).
Added packfile '/usr/share/games/quake2//baseq2/pak1.pak' (279 files).
Added packfile '/usr/share/games/quake2//baseq2/pak2.pak' (2 files).
Added packfile '/usr/share/games/quake2//baseq2/pak3.pak' (8 files).
Added packfile '/usr/share/games/quake2/baseq2/pak0.pak' (3386 files).
Added packfile '/usr/share/games/quake2/baseq2/pak1.pak' (279 files).
Added packfile '/usr/share/games/quake2/baseq2/pak2.pak' (2 files).
Added packfile '/usr/share/games/quake2/baseq2/pak3.pak' (8 files).
Using binary dir /usr/lib/yamagi-quake2/baseq2/ to fetch paks
Using '/home/greffrath/.yq2/baseq2' for writing.
execing default.cfg
execing yq2.cfg
execing config.cfg
basedir is write protected.
Console initialized.
[...]

Please note that it is first added by FS_AddSystemwideGameDirectory() with an additional slash added between the SYSTEMDIR and the BASEDIRNAME part. And then it is added a second time by FS_AddGameDirectory() without this additional slash.

I believe two things need to be done here:

• A check should be added to either FS_AddGameDirectory() or all of the higher-level directory-adding functions to prevent the same directory from being loaded twice.
• For this to work, directory names need to be sanitized and must not contain redundant slashes or similar artifacts.

Thanks,

Fabian

[fabiangreffrath]

On the other hand, maybe it is best to just completely omit that call to FS_AddSystemwideGameDirectory() if a basedir is explicitly given on the command line. See e.g. what happens if I insist to run the demo version that is installed into /usr/share/games/quake2-demo:

$ quake2 +set basedir /usr/share/games/quake2-demo

Yamagi Quake II v5.31
=====================
[...]
Using /usr/share/games/quake2//baseq2/ to fetch paks
Added packfile '/usr/share/games/quake2//baseq2/pak0.pak' (3386 files).
Added packfile '/usr/share/games/quake2//baseq2/pak1.pak' (279 files).
Added packfile '/usr/share/games/quake2//baseq2/pak2.pak' (2 files).
Added packfile '/usr/share/games/quake2//baseq2/pak3.pak' (8 files).
Added packfile '/usr/share/games/quake2-demo/baseq2/pak0.pak' (3386 files).
Added packfile '/usr/share/games/quake2-demo/baseq2/pak1.pak' (279 files).
Added packfile '/usr/share/games/quake2-demo/baseq2/pak2.pak' (2 files).
Added packfile '/usr/share/games/quake2-demo/baseq2/pak3.pak' (8 files).
Using binary dir /usr/lib/yamagi-quake2/baseq2/ to fetch paks
Using '/home/greffrath/.yq2/baseq2' for writing.
[...]

Please note that the binary has been built with WITH_SYSTEMWIDE=yes but without WITH_SYSTEMDIR set.

[smcv]

$ quake2 +set basedir /usr/share/games/quake2-demo

That specific command-line is not correct; the point of Debian's quake2 wrapper script is that it passes appropriate command-line arguments to the engine to select the right basedir.

The right thing would be one of

quake2
quake2 --demo

which end up running

+ exec /usr/lib/quake2/quake2-engine +set basedir /usr/share/games/quake2
+ exec /usr/lib/quake2/quake2-engine +set basedir /usr/share/games/quake2-demo

quake2-engine is a symlink to Yamagi's Quake II in this case, although in principle it could be any Q2 engine (we don't have a second Quake II engine packaged, but we do have more than one Quake 1 engine, and the setup is basically the same).

yquake2 in Debian is deliberately not on the default $PATH, because it is intended to be used via the quake2 script, which also does things like checking that the necessary data files are present.

Previous uploads of yquake2 in Debian have circumvented this issue by either setting SYSTEMDIR to /usr/lib/yamagi-quake2, or not setting SYSTEMWIDE at all.

[smcv]

Totally untested patch which might be one way to simplify this: master...smcv:wip/systemwide-basedir

[fabiangreffrath]

That specific command-line is not correct

Yes, I know. But this is upstream's bug tracker, upstream's executable is called quake2 and I wanted my example to be directly reproducible for them. Also, I wanted to avoid the necessity to introduce them into the oddities of our "quake2" package, but now that you did, it's fine. ;)

[fabiangreffrath]

Totally untested patch [...]

I think Simon's patch is right, FS_AddSystemwideGameDirectory() has to go.

This patch cures both symptoms: (1) base dirs are not loaded twice anymore and (b) if an explicit basedir is given on the command line, this one is used, else it falls back to either SYSTEMDIR if this is defined or the current directory else.

I was working on a similar approach that would only call FS_AddSystemwideGameDirectory() if SYSTEMDIR and fs_basedir->string were different, because else the exact same directory would be added in the next step again. But, on the other hand, if fs_basedir->string was different from SYSTEMDIR it was probably some path given on the command line and again this is a reason not to call FS_AddSystemwideGameDirectory(). Since both conditions contradict each other, there would be no way this function was called at all.

[Yamagi]

Hmm.. yeah. You're right. The whole WITH_SYSTEMWIDE logic is kind of messy and needs to be refactored. I'm very short on time right now. I'm afraid that it'll take some time until I'm able to have closer look. :(

[fabiangreffrath]

Hey Simon,

maybe we should add your patch to the Debian package? This would serve it a lot more testing and if it proves satisfactory, that could be a good reason for Yamagi to apply it upstream. Also, it will fix the "regression" I introduced in my previous upload.

[Yamagi]

I've just pushed the patch by smcv. Thank you. :)

[fabiangreffrath]

Good idea! We have applied it to the Debian package since Oct 06 without any reported problems.

[mckaygerhard]

i'll will test in the two ways: building as a linux box and using a debian in a debian bvox, and see i the expected behaviour are the dessired!

and i'll post the feedback in a week .. if i found some problems i open a issue request at here.. due i noted there's no much users using the debian engine (i remited to the debian conquets)and cited that why providing guindows binarys.. that's why there's no bugs reports.. guindows users conforms to used the engine and no more..

[mckaygerhard]

quickly tested! i opened a new issue if necesary:

i tested the package in debian with debian sid installation (to make more confortable to bugs re1uest)

when i donload the JUG mod (quake 2 juggernaut) and try to load as u'r suggested way simon, fail to load

now either using quake2 or directly the binary lauch, base patch not loading (/usr/share/games/quake2/baseq2) and only loads the manually set basedir for the downloaded game by example JUG (/media/pendrive/JUG), many custom mods need loaded the complete game at base dir

to the engine does not work!

NOTE: juggernaut does not need a game lib loader game.so