[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-frameworks-devel
Subject: D24489: KAutosaveFile not respecting maximum filename length
From: Jean-Baptiste Mardelle <noreply () phabricator ! kde ! org>
Date: 2019-12-06 12:37:54
Message-ID: 9754fc65d3de83b700c45b92203017e5 () localhost ! localdomain
[Download RAW message or body]
[Attachment #2 (text/plain)]
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
[Attachment #3 (text/html)]
<table><tr><td style="">mardelle added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; \
float: right; color: #464C5C; font-weight: bold; border-radius: 3px; \
background-color: #F7F7F9; background-image: linear-gradient(to \
bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid \
rgba(71,87,120,.2);" href="https://phabricator.kde.org/D24489">View \
Revision</a></tr></table><br /><div><div><p>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:</p>
<ol class="remarkup-list">
<li class="remarkup-list-item">When using \
KAutoSaveFile::staleFiles(filename) to check if there is an existing stale \
file for a document, we use the function \
KAutoSaveFile::extractManagedFilePath</li> </ol>
<p>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.<br /> Instead I would propose to \
replace extractManagedFilePath with a new function, comparing filename, \
then paths like:</p>
<div class="remarkup-code-block" style="margin: 12px 0;" \
data-code-lang="text" data-sigil="remarkup-code-block"><pre \
class="remarkup-code" style="font: 11px/15px "Menlo", \
"Consolas", "Monaco", monospace; padding: 12px; margin: \
0; background: rgba(71, 87, 120, 0.08);">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);
}</pre></div>
<ol class="remarkup-list" start="2">
<li class="remarkup-list-item">The QLockFile class used in KAutoSaveFile \
creates a temporary file to manage locking:</li> </ol>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; \
font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; \
background-color: #f8f9fc;"><p>QLockFile rmlock(d->fileName + \
QLatin1String(".rmlock"));</p></blockquote>
<p><a href="https://github.com/qt/qtbase/blob/dev/src/corelib/io/qlockfile.cpp#L259" \
class="remarkup-link" target="_blank" \
rel="noreferrer">https://github.com/qt/qtbase/blob/dev/src/corelib/io/qlockfile.cpp#L259</a></p>
<p>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.<br /> Easier solution is probably to make maximum \
filename in KAutoSave File::tempFileName() a bit shorter.</p>
<p>Thoughts? Should I prepare separate patches ? But changing the fileName \
encoding as suggested:</p>
<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; \
font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; \
background-color: #f8f9fc;"><p>Simpler than toPercentEncoding: use \
QUrl::fileName(QUrl::FullyEncoded).</p></blockquote>
<p>Might break more the path recovery method \
extractManagedFilePath..</p></div></div><br \
/><div><strong>REPOSITORY</strong><div><div>R244 \
KCoreAddons</div></div></div><br /><div><strong>REVISION \
DETAIL</strong><div><a \
href="https://phabricator.kde.org/D24489">https://phabricator.kde.org/D24489</a></div></div><br \
/><div><strong>To: </strong>mardelle, Frameworks, dfaure, mpyne<br \
/><strong>Cc: </strong>ahmadsamir, kde-frameworks-devel, LeGast00n, GB_2, \
michaelh, ngraham, bruns<br /></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic