Re: D24489: KAutosaveFile not respecting maximum filename length

[Jean-Baptiste Mardelle]

Ouch sorry about that! Won't be able to work on it before tomorrow afternoon at 
best. Feel free to revert if you think it's best and I can work on an updated 
version...

On Dec 13, 2019, 21:49, at 21:49, David Faure  wrote:
>dfaure added a comment.
>
>
>  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/
>
>REPOSITORY
>  R244 KCoreAddons
>
>REVISION DETAIL
>  https://phabricator.kde.org/D24489
>
>To: mardelle, #frameworks, dfaure, mpyne
>Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh,
>ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[David Faure]

dfaure added a comment.


  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.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Ahmad Samir]

ahmadsamir added a comment.


  According to [1], for windows, it's _MAX_FNAME defined in stdlib.h
  [1] 
https://docs.microsoft.com/en-us/cpp/c-runtime-library/path-field-limits?view=vs-2019
  
  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...

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Jean-Baptiste Mardelle]

mardelle added a comment.


  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...

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[David Faure]

dfaure added a comment.


  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/

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Ahmad Samir]

ahmadsamir added a comment.


  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 ***Failed0.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://u...@example.com/home/remote/test.txt"));
  
  so the call to managedFile.toLocalFile() returns an empty string. Using 
.path() instead seems to work.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[David Faure]

dfaure accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Jean-Baptiste Mardelle]

mardelle marked 3 inline comments as done.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Jean-Baptiste Mardelle]

mardelle updated this revision to Diff 71081.
mardelle added a comment.


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

REPOSITORY
  R244 KCoreAddons

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D24489?vs=67489=71081

REVISION DETAIL
  https://phabricator.kde.org/D24489

AFFECTED FILES
  src/lib/io/kautosavefile.cpp

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[David Faure]

dfaure added a comment.


  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.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Jean-Baptiste Mardelle]

mardelle added a comment.


  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!

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Ahmad Samir]

ahmadsamir added a comment.


  In D24489#573152 , @mardelle wrote:
  
  > 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. :)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Jean-Baptiste Mardelle]

mardelle added a comment.


  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);
}
  
  
  
  2. 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..

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> kautosavefile.cpp:70
>  const QString protocol(managedFile.scheme());
> -const QString path(managedFile.adjusted(QUrl::RemoveFilename | 
> QUrl::StripTrailingSlash).path());
> -QString name(managedFile.fileName());
> +const QString 
> path(QString::fromLatin1(QUrl::toPercentEncoding(managedFile.adjusted(QUrl::RemoveFilename
>  | QUrl::StripTrailingSlash).path()).constData()));
> +QString 
> name(QString::fromLatin1(QUrl::toPercentEncoding(managedFile.fileName()).constData()));

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 )` was added in Qt 5.0, this code is probably older than that.

> kautosavefile.cpp:71
> +const QString 
> path(QString::fromLatin1(QUrl::toPercentEncoding(managedFile.adjusted(QUrl::RemoveFilename
>  | QUrl::StripTrailingSlash).path()).constData()));
> +QString 
> name(QString::fromLatin1(QUrl::toPercentEncoding(managedFile.fileName()).constData()));
>  

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 '/'.

> kautosavefile.cpp:76
>  // Subtract 1 for the _ char, 3 for the padding separator, 5 is for the 
> .lock
> -int pathLengthLimit = FILENAME_MAX - NamePadding - name.size() - 
> protocol.size() - 9;
> +int pathLengthLimit = NAME_MAX - NamePadding - name.size() - 
> protocol.size() - 9;
>  

(while changing this line: prepend `const`)

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Ahmad Samir]

ahmadsamir added a comment.


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

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Ahmad Samir]

ahmadsamir added a reviewer: mpyne.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure, mpyne
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Ahmad Samir]

ahmadsamir added a dependent revision: D25767: KAutoSaveFile: add a unit test 
to check max. filename length.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Ahmad Samir]

ahmadsamir added a comment.


  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...

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Ahmad Samir]

ahmadsamir added a comment.


  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.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[David Faure]

dfaure added a comment.


  A unittest would be very welcome.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Nathaniel Graham]

ngraham edited the summary of this revision.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Ahmad Samir]

ahmadsamir added a comment.


  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_MAX4096  /* # chars in a path name including nul */

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure
Cc: ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Jean-Baptiste Mardelle]

mardelle added a reviewer: dfaure.

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

To: mardelle, #frameworks, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D24489: KAutosaveFile not respecting maximum filename length

[Jean-Baptiste Mardelle]

mardelle created this revision.
mardelle added a reviewer: Frameworks.
mardelle added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
mardelle requested review of this revision.

REVISION 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.
  
  This fixes the follwing bug: https://bugs.kde.org/show_bug.cgi?id=412519

TEST PLAN
  Bug is fixed with the changes

REPOSITORY
  R244 KCoreAddons

REVISION DETAIL
  https://phabricator.kde.org/D24489

AFFECTED FILES
  src/lib/io/kautosavefile.cpp

To: mardelle, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns