[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