ci_find_file/unix: add fast-path for file lookup #1350

rofl0r · 2021-07-08T16:55:40Z

this achieves 2 major improvements:

if the directory and filename are correct, only a single stat
syscall needs to be done to check whether the file exists.
no need to open the directory (1 syscall) and check whether
each one of the entries matches the filename (1 syscall each).
for perspective, each syscall requires to enter the kernel,
a full contex switch, and therefore at least 1000 cpu cycles.
if the filename contains a directory separator to dive into
a subdir, the existing method fails.
this allows to use e.g. PlayMP3File("sounds/sound25.ogg")
successfully.

this achieves 2 major improvements: - if the directory and filename are correct, only a single stat syscall needs to be done to check whether the file exists. no need to open the directory (1 syscall) and check whether each one of the entries matches the filename (1 syscall each). for perspective, each syscall requires to enter the kernel, a full contex switch, and therefore at least 1000 cpu cycles. - if the filename contains a directory separator to dive into a subdir, the existing method fails. this allows to use e.g. PlayMP3File("sounds/sound25.ogg") successfully.

ericoporto · 2021-07-08T17:34:34Z

Why using goto in a C++ codebase?

If you can't refactor your code use the do while(false) pattern and break to keep the stack making sense.

rofl0r · 2021-07-08T17:47:46Z

Why using goto in a C++ codebase?

because it's valid C++ ?

If you can't refactor your code use the do while(false) pattern and break to keep the stack making sense.

the change you propose is much more intrusive, as it adds another indentation level for all code that follows, and makes the diff 50+ lines rather than 10.

ericoporto · 2021-07-08T17:56:55Z

You can modify more lines, it will be code that builds. It will do the same in your architecture and not blow up my wasm port. Now I need to PR a New version of this for wasm because there's no way to goto there.

rofl0r · 2021-07-08T17:59:42Z

You can modify more lines, it will be code that builds.

yeah, but it's not only intrusive, it also breaks git features like git blame.

not blow up my wasm port. Now I need to PR a New version of this for wasm because there's no way to goto there.

emscripten/wasm don't support goto ? first time i hear this. do you have a link with more info about that?

ericoporto · 2021-07-08T18:00:14Z

WebAssembly/design#796

Everything that uses goto has to be refactored. This is one of the reasons I can't build everything from source or build old things from source that had goto. This is why I bring some stuff from Emscripten ports.

It has a mechanism where it can kinda guess with binaryen and I use it and it can guess go-tos and it will refactor with this do while false pattern. It doesn't always work but sometimes does.

rofl0r · 2021-07-08T18:03:50Z

WebAssembly/design#796
Everything that uses goto has to be refactored. This is one of the reasons I can't build everything from source or build old things from source that had goto. This is why I bring some stuff from Emscripten ports.

this seems to be a restriction of the wasm bytecode format itself, certainly emscripten itself can deal with goto as it is used in a lot of codebases, for example extensively even in musl libc which emscripten uses as its libc implementation.

rofl0r · 2021-07-08T18:07:21Z

$ global -xg goto | wc -l
1278

current master branch has 1278 uses of goto in different sources, so if emscripten can't deal with it your effort is prone to fail anyway, this one goto doesn't change the overall situation.

rofl0r · 2021-07-08T18:23:59Z

just found this comment by kripken (emscripten lead dev) confirming that emscripten can deal just fine with goto: WebAssembly/design#796 (comment)

ericoporto · 2021-07-08T19:04:20Z

I strongly disenchourage using goto. There's no need at all for it, it's a simple refactor to not use it. If you can't refactor to use inline function, use the do while false break pattern.

There are additional problems:

I also recommend using dir_name == nullptr && file_name == nullptr to be clear of intentions instead (like in the lines above) of the cryptic directory && filename.
There's also a magic number 1024 there as size of a buffer... What does it limits? It's a mystery, there is no comment.
You also forgot the AGS_PLATFORM_DEBUG check that is expected to print debug messages.
the code is hardcoding / as file separator instead of using append_filename.

This entire file though will probably get axed soon since it has already other problems - like compile time case sensitivity assumption, this doesn't hold at all on MacOS.

ivan-mogilko · 2021-07-08T20:53:42Z

I don't have too much problem with this change if it keeps working everywhere for now. This function is a very old code that probably supposed to be refactored later anyway.

goto may be dangerous, so I'd double check that there is nothing left deinitialized.
At least there are no C++ constructable objects in this function, only C-style buffers.

NOTE: the only other gotos found directly in the Engine code are located in the "route_finder_impl_legacy.cpp". The rest is in the C libraries.

This entire file though will probably get axed soon since it has already other problems

It may be okay to put one fix even if it's temporary.

I also recommend using dir_name == nullptr && file_name == nullptr to be clear of intentions instead (like in the lines above) of the cryptic directory && filename.

The rest of this particular file uses == explicit comparison indeed.

To be fair, we do not have this requirement for the code style in general though. Personally I don't find this condition cryptic at all, and we use this pointer check everywhere in the code, and the newest code. I usually write in a same way for instance, and nobody ever mentioned that it's difficult to understand for some reason.

There's also a magic number 1024 there as size of a buffer... What does it limits? It's a mystery

Why is that a mystery? it's just a temp buffer and used in a sprintf one line below, and that's it.

ivan-mogilko · 2021-07-08T21:03:00Z

By the way, since we started talking about goto, looking around this function, am I seeing things or there are potential memory leak cases?

The return statements on lines 155, 160, 165, they may leave local buffers directory and filename allocated but not freed.

rofl0r · 2021-07-08T21:44:57Z

i found goto to be used in:

Common/libsrc/freetype-2.1.3/*
Compiler/script/cs_parser.cpp
Editor/scintilla/src/*
Engine/ac/dialog.cpp
Engine/ac/route_finder_impl_legacy.cpp
Engine/libsrc/apeg-1.2.1/audio/layer2.c
Engine/libsrc/apeg-1.2.1/audio/layer3.c
Engine/libsrc/apeg-1.2.1/audio/readers.c
Engine/libsrc/apeg-1.2.1/getbits.c
Engine/libsrc/apeg-1.2.1/getpic.c
Engine/libsrc/apeg-1.2.1/mpeg1dec.c
Engine/libsrc/apeg-1.2.1/ogg.c
Engine/platform/util/pe.c
Tools/data/dialogscriptconv.cpp
libsrc/allegro/src/blit.c
libsrc/allegro/src/file.c
libsrc/allegro/src/fli.c
libsrc/allegro/src/rotate.c
libsrc/allegro/src/unicode.c

There's also a magic number 1024 there as size of a buffer... What does it limits? It's a mystery

it's big enough under most circumstances to hold a filepath, but not enough to cause issues with huge stack buffers (eventually overflowing thread stacks) and if it isn't the code will fail gracefully and use the non-fast-path implementation below.

goto is dangerous, so I'd double check that there is nothing left deinitialized.

it's not really dangerous, but i already checked the code for these issues.

the code is hardcoding / as file separator instead of using append_filename.

the code is only used on non-case-insentive filesystems, so only on platforms that have '/' as directory separator.

You also forgot the AGS_PLATFORM_DEBUG check that is expected to print debug messages.

what?

I strongly disenchourage using goto. There's no need at all for it, it's a simple refactor to not use it. If you can't refactor to use inline function, use the do while false break pattern.

once out of rational arguments, you refrain to "disencourage" use without arguments... alright.
the need for it is to not have code duplication and not to clutter unrelated code with gratuitous whitespace changes and breaking git blame functionality for all affected lines.

I also recommend using dir_name == nullptr && file_name == nullptr to be clear of intentions instead (like in the lines above) of the cryptic directory && filename.

it's not cryptic, it's actually more clear than having the gratuitous comparisons which make the code longer for no reason to do the same and cause more strain and effort for the reader.

The return statements on lines 155, 160, 165, they may leave local buffers directory and filename allocated but not freed.

i'll take a look and fix those in a followup commit, if indeed an issue.

ivan-mogilko · 2021-07-08T21:50:22Z

You also forgot the AGS_PLATFORM_DEBUG check that is expected to print debug messages.

what?

I think @ericoporto may be referring to fprintf calls. Apprently only one of them was wrapped in AGS_PLATFORM_DEBUG
check as seen here, and the rest are not. This pr does not change anything there though.

ivan-mogilko · 2021-07-08T21:55:24Z

i found goto to be used in:
Engine/ac/dialog.cpp
Tools/data/dialogscriptconv.cpp

They do not have goto in real code, they just mention this dialog script command (and there's goto in an old commented code).
The only part of engine's own code that actually uses goto is route_finder_impl_legacy.cpp, the rest is library C sources.

ivan-mogilko · 2021-07-08T22:04:28Z

Common/util/misc.cpp

@@ -115,7 +115,7 @@ char *ci_find_file(const char *dir_name, const char *file_name)
    fix_filename_slashes(filename);
  }

-  if(directory && filename) {
+  if(directory && filename && !strstr(filename, "..")) {


I don't think this is a correct check. It's possible to have a file name containing two dots.

yeah, but in that case we fallback to the chdir/opendir approach below

thank you ) (that's even more elaborate than I thought about)

ericoporto · 2021-07-09T00:02:56Z

Case Sensitivity: No need to add the additional comment clause at right, it's false, MacOS is Unix and Case Insensitive by default: https://support.apple.com/guide/disk-utility/file-system-formats-available-in-disk-utility-dsku19ed921c/mac

Unfortunately this can't be fixed here in this PR, AGS should be able to workout Case Sensitivity at runtime.

Also this function used to print when it found a diamond in debug and now it doesn't.

I still strongly object against using goto in AGS codebase - using AGS own string type would make this code much cleaner and remove any need for goto at no performance impact at all.

rofl0r · 2021-07-09T00:23:41Z

the comment says "only used on UNIX platforms", not "on ALL UNIX platforms"

ivan-mogilko · 2021-07-09T00:46:50Z

using AGS own string type would make this code much cleaner and remove any need for goto at no performance impact at all.

This will require to rewrite whole function, and also adjust its uses, as the code that calls it expects a buffer that got to be freed.

Also this function used to print when it found a diamond in debug and now it doesn't.

Does it have to, if there was no search and the file was found by a matching name as-is? The Windows variant also does not print anything.

ivan-mogilko · 2021-07-09T01:08:28Z

@rofl0r I think your memory leaks fix with gotos may leave some things unitialized.

There is a case where prevdir is storing handle from opendir, and there's also a call to chdir that is not getting reverted if you break execution in the middle.

EDIT: the fact that it uses chdir is annoying, this makes this function harder to use in a multithreaded enviroment... if we ever rewrite it, chdir should not be used imho.

ivan-mogilko · 2021-07-09T01:13:22Z

About what you wrote in PR description:

if the filename contains a directory separator to dive into
a subdir, the existing method fails.
this allows to use e.g. PlayMP3File("sounds/sound25.ogg")
successfully.

From what I understand, your added code only deals with the trivial case-matching situation. Does this mean that if there's PlayMP3File("SOUNDS/SOUND25.ogg"); (while the path & file are lowercase) instead - it will still fail?

rofl0r · 2021-07-09T02:05:47Z

There is a case where prevdir is storing handle from opendir, and there's also a call to chdir that is not getting reverted if you break execution in the middle.

good catch, it's fixed now

the fact that it uses chdir is annoying, this makes this function harder to use in a multithreaded enviroment... if we ever rewrite it, chdir should not be used imho.

absolutely correct. using chdir is a real hack and codesmell.

Does this mean that if there's PlayMP3File("SOUNDS/SOUND25.ogg"); (while the path & file are lowercase) instead - it will still fail?

yes, fastpath code (and that usecase) only works when the case is 100% correct.

ivan-mogilko · 2021-07-09T03:05:21Z

@ericoporto I am okay with goto there mostly because it's a small library function and purely C-style with no C++ objects around. This pr is also a small fix, it does not change the function much (and it fixed few potential memory leaks now too).

But it definitely better be rewritten at some point in the future, it's not good for number of reasons. Some cases won't even work with it, not even after this fix (I mentioned one example above).

You mentioned that your emscripten port does not work with it? Could you mention how do you deal with the legacy route finder code, do you not include it, or had to refactor the whole thing? There are also multiple gotos in the allegro library, except I don't know if emscripten port includes that particular code.

Are there other things that do not work in your port, could I see the changes that were necessary for it to work?

ivan-mogilko added the pull: under review label Jul 8, 2021

rofl0r added 2 commits July 8, 2021 22:58

ci_find_file: fix memory leaks in error paths

1f7c216

ci_find_file: add security check to not allow fs traversal

0a9e89d

ivan-mogilko reviewed Jul 8, 2021

View reviewed changes

ci_find_file: add comment about .. fs traversal check

56f1322

ci_find_file: fix directory handle leak on error

3951d93

ivan-mogilko merged commit 66aecac into adventuregamestudio:master Jul 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci_find_file/unix: add fast-path for file lookup #1350

ci_find_file/unix: add fast-path for file lookup #1350

rofl0r commented Jul 8, 2021

ericoporto commented Jul 8, 2021 •

edited

Loading

rofl0r commented Jul 8, 2021

ericoporto commented Jul 8, 2021 •

edited

Loading

rofl0r commented Jul 8, 2021

ericoporto commented Jul 8, 2021 •

edited

Loading

rofl0r commented Jul 8, 2021

rofl0r commented Jul 8, 2021

rofl0r commented Jul 8, 2021

ericoporto commented Jul 8, 2021 •

edited

Loading

ivan-mogilko commented Jul 8, 2021 •

edited

Loading

ivan-mogilko commented Jul 8, 2021

rofl0r commented Jul 8, 2021

ivan-mogilko commented Jul 8, 2021 •

edited

Loading

ivan-mogilko commented Jul 8, 2021 •

edited

Loading

ivan-mogilko Jul 8, 2021

rofl0r Jul 8, 2021

rofl0r Jul 8, 2021

ivan-mogilko Jul 8, 2021 •

edited

Loading

ericoporto commented Jul 9, 2021 •

edited

Loading

rofl0r commented Jul 9, 2021

ivan-mogilko commented Jul 9, 2021 •

edited

Loading

ivan-mogilko commented Jul 9, 2021 •

edited

Loading

ivan-mogilko commented Jul 9, 2021 •

edited

Loading

rofl0r commented Jul 9, 2021

ivan-mogilko commented Jul 9, 2021 •

edited

Loading

ci_find_file/unix: add fast-path for file lookup #1350

ci_find_file/unix: add fast-path for file lookup #1350

Conversation

rofl0r commented Jul 8, 2021

ericoporto commented Jul 8, 2021 • edited Loading

rofl0r commented Jul 8, 2021

ericoporto commented Jul 8, 2021 • edited Loading

rofl0r commented Jul 8, 2021

ericoporto commented Jul 8, 2021 • edited Loading

rofl0r commented Jul 8, 2021

rofl0r commented Jul 8, 2021

rofl0r commented Jul 8, 2021

ericoporto commented Jul 8, 2021 • edited Loading

ivan-mogilko commented Jul 8, 2021 • edited Loading

ivan-mogilko commented Jul 8, 2021

rofl0r commented Jul 8, 2021

ivan-mogilko commented Jul 8, 2021 • edited Loading

ivan-mogilko commented Jul 8, 2021 • edited Loading

ivan-mogilko Jul 8, 2021

Choose a reason for hiding this comment

rofl0r Jul 8, 2021

Choose a reason for hiding this comment

rofl0r Jul 8, 2021

Choose a reason for hiding this comment

ivan-mogilko Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

ericoporto commented Jul 9, 2021 • edited Loading

rofl0r commented Jul 9, 2021

ivan-mogilko commented Jul 9, 2021 • edited Loading

ivan-mogilko commented Jul 9, 2021 • edited Loading

ivan-mogilko commented Jul 9, 2021 • edited Loading

rofl0r commented Jul 9, 2021

ivan-mogilko commented Jul 9, 2021 • edited Loading

ericoporto commented Jul 8, 2021 •

edited

Loading

ericoporto commented Jul 8, 2021 •

edited

Loading

ericoporto commented Jul 8, 2021 •

edited

Loading

ericoporto commented Jul 8, 2021 •

edited

Loading

ivan-mogilko commented Jul 8, 2021 •

edited

Loading

ivan-mogilko commented Jul 8, 2021 •

edited

Loading

ivan-mogilko commented Jul 8, 2021 •

edited

Loading

ivan-mogilko Jul 8, 2021 •

edited

Loading

ericoporto commented Jul 9, 2021 •

edited

Loading

ivan-mogilko commented Jul 9, 2021 •

edited

Loading

ivan-mogilko commented Jul 9, 2021 •

edited

Loading

ivan-mogilko commented Jul 9, 2021 •

edited

Loading

ivan-mogilko commented Jul 9, 2021 •

edited

Loading