bitwiseworks / libc

LIBC Next (kLIBC fork)
9 stars 4 forks source link

abspath/realpath fails with File name too long #73

Closed dmik closed 4 years ago

dmik commented 4 years ago

Given something like this while staying in D:\Coding\qt5\qt5-dev-build\qtwebengine\src\core\debug:

../../../../../qt5/qtwebengine/src/3rdparty/chromium/third_party/mesa_headers/../../../../../qt5/qtwebengine/src/3rdparty/chromium/gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc

both APIs will file with errno 63 (Fila name too long).

Apparently, the real file name is much shorter: 132 characters in total (with 260 characters being the OS limit). We should make these APIs smarter and work around the limitation.

A real life example is here: https://github.com/bitwiseworks/qtwebengine-chromium-os2/issues/3#issuecomment-622141378.

StevenLevine commented 4 years ago

When implementing support for relative path normalization, we might want to consider accounting for the fact that file name limits on other platforms often exceed CCHMAXPATH and that the ported tools often implicitly depend on this when generating paths, as we have found.

The kernel does normalize the paths input to DosOpenL and friends, but the supplied value is still limited to CCHMAXPATH.

With the current directory set to

i:\sla_dev2\moz2_dev\mozilla-os2-davey.git\browser\components\dirprovider\tests

DosOpenL will accept the 259 character path:

../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/unit\head_dirprovider.js

However DosOpenL will fail with ERROR_FILENAME_EXCED_RANGE given the 268 character path:

../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/../tests/unit\head_dirprovider.js

It is unfortunate that the the path length is check before the normalization occurs.

What I am suggesting is that any path length limit checks be done only after the the path is normalized.

dmik commented 4 years ago

Thanks for the DosOpen feedabck! Although, LIBC does such things on its own, no Dos APIs involved. It turned out to be harder to fix than I thought though due to the way LIBC code is written. It's checked for buffer overflows many times before starting to process . and ... So quite a bit needs to be changed there.

BTW, seems that I faced this problem already several years ago: http://trac.netlabs.org/libc/ticket/379.

StevenLevine commented 4 years ago

I'm familiar with the kLIBC code. I only mentioned the DosOpenL limitations to suggest that kLIBC could be more forgiving.

To avoid issues with ported apps, it would be best if kLIB accepted any path as long as the normalized path is something that the OS/2 APIs would accept.

dmik commented 4 years ago

Some more details. It's only realpath which behaves badly —_abspath is perfectly capable of doing the right thing in this case. More exactly, it's not reaplath itself but __libc_back_fsResolveUnix called by __libc_back_fsResolve in Unix mode (i.e. when no -Zno-unix is given). __libc_back_fsResolve is in turn a work horse for __libc_Back_PathResolve (whcih is a work horse for realpath).

Yes, the above means that _abspath and realpath use two completely separate logics to resolve .. and . entries (as well as to turn relative paths into absolute). These logics basically duplicate each other (and the abspath code explicitly states that it mimics the __libc_back_fsResolveUnix behavior). This is really bad design I must say. As this code is really big, complex and should have been a single point of execution.

Anyway, the reason why __libc_back_fsResolveUnix fails and _abspath does not is because the former does intermediate string buffer management in a different way and appends the relative path as it was given by the caller (with all its .. and .) to the current directory BEFORE processing .. and . (the latter does that dir by dir WHILE processing it). So, if the overall length of the user input and the current directory exceeds PATH_MAX, it fails immediately (even if the effective path is as simple as D:/foo.txt). More over, even __libc_PathRewrite is called on the path before resolving .. and . (which simply makes no sense because path rewrite rules are supposed to be always absolute by design). And this is another weak point on the realpath side of things.

Even further, the OS/2 version of the resolver __libc_back_fsResolveOS2 does not do any .. and . resolution at all. This clearly breaks POSIX requirements for realpath. The reason why it didn't trigger long ago is perhaps because noone ever uses -Zno-unix with kLIBC...

Note that __libc_back_fsResolve is used all over LIBC code. This means that not only realpath is subject to the File name too long error. E.g. spawn/exec will fail with exactly the same error (which I believe http://trac.netlabs.org/libc/ticket/379 is about).

The best solution here is to perform preliminary path reduction to get rid of .. and . upfront, the way _abspath does that. The simplest way to achieve this is to call _abspath in __libc_back_fsResolve before doing anything else (this will solve ALL of the above problems and will also remove duplication of logic by making __libc_back_fsResolveUnix own relative path handling and path reduction code redundant as it won't be ever triggered).

This solution is very simple patch-wise but I've faced some locking issue there. _abspath calls __libc_Back_fsDirCurrentGet which locks the global FS mutex which is not recursive. So, if __libc_back_fsResolve is called from under the FS mutex lock, calling _abspath from there crashes the application with the respective LIBC panic. I've attempted to solve it by introducing __libc_back_fsDirCurrentGet (note theback casing, by convention) which doesn't do FS mutex locking and calling this from _abspath instead.

The above breaks thread safety in _abspath with relation to current drive/directory detection. Which means that I will also have to introduce a specal non-locking version of _abspath to do things right. I will do it and then test the new LIBC thoroughly by making it my main DLL to make sure it doesn't break anything.

dmik commented 4 years ago

I've rebooted and it seems to work. Both the Chromium compilation issue has gone and mkdir/exec in the shell script also works fine now. I will give it another spin and then commit.