KAutosaveFile not respecting maximum filename length
AcceptedPublic

Authored by mardelle on Oct 8 2019, 11:16 AM.

Details

Reviewers
dfaure
mpyne
Group Reviewers
Frameworks
Summary

There are 2 different issues in current code regarding maximum filename length:

1- We use FILENAME_MAX which is defined as 4096, while most filesystems have a max length of 256. Replacing FILENAME_MAX with NAME_MAX fixes this first problem (could not test on Windows if it works)

2- We are calculating the maximum length on the UTF-8 string, then encoding to percent encoding. This can result in longer strings since single characters will be replaced by a percent string. So in some situations, we end up with a string longer than allowed. Doing the percent encoding before length calculation fixes the problem.

BUG: 412519

Test Plan

Bug is fixed with the changes

Diff Detail

Repository
R244 KCoreAddons
Lint
Lint Skipped
Unit
Unit Tests Skipped
mardelle created this revision.Oct 8 2019, 11:16 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptOct 8 2019, 11:16 AM
mardelle requested review of this revision.Oct 8 2019, 11:16 AM
ahmadsamir added a subscriber: ahmadsamir.EditedDec 1 2019, 4:54 PM

IIUC, FILENAME_MAX corresponds to PATH_MAX, 4096 (bytes; or chars in an array).

NAME_MAX is the max. filename (the filename part only, without the canonical path) length, this is 255 on Linux.

So using FILENAME_MAX is correct in the code.

See:
https://www.gnu.org/software/libc/manual/html_node/Limits-for-Files.html , which talks about FILENAME_MAX, NAME_MAX and PATH_MAX .... \o/
/usr/include/stdio.h (from glibc-devel)
/usr/include/bits/stdio_lim.h

And:
/usr/include/linux/limits.h:
#define NAME_MAX 255 /* # chars in a file name */
#define PATH_MAX 4096 /* # chars in a path name including nul */

EDIT: is using PATH_MAX instead of FILENAME_MAX feasible (I don't know about the portability issues between linux/windows/bsd/whatever)? it has a less ambiguous name, IMHO.

ngraham edited the summary of this revision. (Show Details)Dec 1 2019, 6:46 PM
dfaure added a comment.Dec 1 2019, 7:32 PM

A unittest would be very welcome.

ahmadsamir added a comment.EditedDec 3 2019, 7:05 PM

I was wrong, indeed NAME_MAX is what should be used; looking at the code in KAutoSaveFile::open(), and how tempFileName() is actually used:

tempFile = staleFilesDir + QChar::fromLatin1('/') + d->tempFileName();

NAME_MAX is what should be used, as tempFileName() returns the "filename" part only, not the whole path.

EDIT: ignore my noise (I need to investigate this further).

tempFilename() actually changes the filename to include the whole path plus some other bits and pieces; but as @mardelle proposes, the maximum filename length is NAME_MAX and it should take into account the percent encoding.

Again, sorry about the noise...

Unit test added in D25767. AFAICS, both points addressed in this diff are needed to fix the referenced bug.

dfaure requested changes to this revision.Dec 5 2019, 10:45 PM

Why is this not in the same commit as the related unittest, as is common practice?

src/lib/io/kautosavefile.cpp
70

That's one long line, hard to read. Extract a const QByteArray encodedPath = QUrl::toPercentEncoding(....) ?

[pre-existing: I would personally call this QByteArray encodedDirectory and QString directory, to my eyes path is a full path including filename. At least it's ambiguous, unlike directory]

Also check if you can remove the .constData(). QString::fromLatin1(const QByteArray &str) was added in Qt 5.0, this code is probably older than that.

71

and a const QByteArray encodedFileName = ..., for symmetry.

Simpler than toPercentEncoding: use QUrl::fileName(QUrl::FullyEncoded).
However this wouldn't work for encodedDirectory above, because it wouldn't encode '/'.

76

(while changing this line: prepend const)

This revision now requires changes to proceed.Dec 5 2019, 10:45 PM

The unittest is in another diff because it's a different author I guess. I can process the comments but looking closer I found 2 other important issues in this class:

  1. When using KAutoSaveFile::staleFiles(filename) to check if there is an existing stale file for a document, we use the function KAutoSaveFile::extractManagedFilePath

This function compares the orginal document path one reconstructed from the lock file name. However this logic seems flawed since the lock file name uses a trimmed path in some cases it is impossible to rebuild the original document's path from it.
Instead I would propose to replace extractManagedFilePath with a new function, comparing filename, then paths like:

bool staleMatchesManaged(const QString& staleFileName, const QString managedFile)
{
    const QStringRef sep = staleFileName.rightRef(3);
    int sepPos = staleFileName.indexOf(sep);
    if (QFileInfo(managedFile).fileName() != staleFileName.leftRef(sepPos).toString()) {
        // File names don't match
        return false;
    }
    int pathPos = staleFileName.indexOf(QChar::fromLatin1('_'), sepPos);
    const QByteArray encodedPath = staleFileName.midRef(pathPos+1, staleFileName.length()-pathPos-1-KAutoSaveFilePrivate::NamePadding).toLatin1();
    const QString sourcePathStart = QUrl::fromPercentEncoding(encodedPath);
    return QFileInfo(managedFile).absolutePath().startsWith(sourcePathStart);
}
  1. The QLockFile class used in KAutoSaveFile creates a temporary file to manage locking:

QLockFile rmlock(d->fileName + QLatin1String(".rmlock"));

https://github.com/qt/qtbase/blob/dev/src/corelib/io/qlockfile.cpp#L259

This means that in our situation where we have a filename that already has the maximum length, trying to lock will always fail, preventing usage of the autosave file.
Easier solution is probably to make maximum filename in KAutoSave File::tempFileName() a bit shorter.

Thoughts? Should I prepare separate patches ? But changing the fileName encoding as suggested:

Simpler than toPercentEncoding: use QUrl::fileName(QUrl::FullyEncoded).

Might break more the path recovery method extractManagedFilePath..

The unittest is in another diff because it's a different author I guess.

[...]
I didn't mean to step on your toes; feel free to disregard the unit test I wrote, or reuse whatever part(s) of it as you see fit. It's just that I was interested in the matter and sort of wanted to see if my understanding was correct. :)

Oh no don't misunderstand me. I am very glad that someone else stepped up! I have more than enough work on my plate with Kdenlive so please help :)
Hopefully David can give us some hints about the best way to move on!

dfaure added a comment.Dec 6 2019, 9:29 PM

I see. This answers my question about why two merge requests -- no problem, keep them separate, but commit the fix before the unittest
[this is so that bisecting never ends up in the situation where unittests are broken]

You can also ignore my suggestion about fileName(QUrl::FullyEncoded) if you're afraid it won't be symmetrical, no problem.

The rest of mardelle's comment looks sensible to me as well.

mardelle updated this revision to Diff 71081.Dec 8 2019, 12:19 PM

Update patch according to discussion.
extractManagedFilePath will be broken in case of long truncated path but that has always been the case...

mardelle marked 3 inline comments as done.Dec 8 2019, 12:20 PM
dfaure accepted this revision.Dec 8 2019, 9:43 PM
This revision is now accepted and ready to land.Dec 8 2019, 9:43 PM

FWIW, this broke kautosavefiletest:

3: ********* Start testing of KAutoSaveFileTest *********
3: Config: Using QtTest library 5.13.1, Qt 5.13.1 (x86_64-little_endian-lp64 shared (dynamic) release build; by GCC 9.2.1 20190903 [gcc-9-branch revision 275330])
3: PASS   : KAutoSaveFileTest::initTestCase()
3: PASS   : KAutoSaveFileTest::test_readWrite()
3: PASS   : KAutoSaveFileTest::test_fileNameMaxLength()
3: PASS   : KAutoSaveFileTest::test_fileStaleFiles()
3: PASS   : KAutoSaveFileTest::test_applicationStaleFiles()
3: FAIL!  : KAutoSaveFileTest::test_locking() '!staleFiles.isEmpty()' returned FALSE. ()
3:    Loc: [/home/ahmad/rpmbuild/dev/kcoreaddons/autotests/kautosavefiletest.cpp(148)]
3: PASS   : KAutoSaveFileTest::cleanupTestCase()
3: Totals: 6 passed, 1 failed, 0 skipped, 0 blacklisted, 87ms
3: ********* Finished testing of KAutoSaveFileTest *********
1/1 Test #3: kautosavefiletest ................***Failed    0.09 sec

I've tracked it down to line 201 in kautosavefile.cpp:

return QUrl::toPercentEncoding(managedFile.toLocalFile()).startsWith(encodedPath);

The test_locking(), in the unit test, is using a remote file:

QUrl normalFile(QString::fromLatin1("fish://user@example.com/home/remote/test.txt"));

so the call to managedFile.toLocalFile() returns an empty string. Using .path() instead seems to work.

It *also* broke Windows compilation:

C:\CI\workspace\Frameworks\kcoreaddons\kf5-qt5 WindowsMSVCQt5.13\src\lib\io\kautosavefile.cpp(79): error C2065: 'NAME_MAX': undeclared identifier

https://build.kde.org/job/Frameworks/view/Everything%20-%20kf5-qt5/job/kcoreaddons/job/kf5-qt5%20WindowsMSVCQt5.13/

Oh sorry about that. Won't be able to work on it before sunday.. feel free to revert if you think it's best - no computer access now... and I can work on an updated patch...

ahmadsamir added a comment.EditedDec 13 2019, 9:44 PM

According to:
https://docs.microsoft.com/en-us/cpp/c-runtime-library/path-field-limits?view=vs-2019
for windows, it's _MAX_FNAME defined in stdlib.h

see also https://docs.microsoft.com/en-us/cpp/c-runtime-library/filename-max?view=vs-2019

However, I've never built anything on windows...

ahmadsamir: feel free to push a windows fix made from documentation, and check if CI builds. And fix it if not :) Won't be the first time we "hack blind" for Windows. When it's about compilation errors it's easy enough to use the CI to find out what's right.