Differential D4847 Diff 12185 src/buffer/katesecuretextbuffer.cpp

Changeset View

Standalone View

View Options

src/buffer/katesecuretextbuffer.cpp

  • This file was added.
1​#include "katesecuretextbuffer.h"
dfaureUnsubmitted
Done

Missing copyright header

dfaure: Missing copyright header
2​
3​#ifndef Q_OS_WIN
4​#include <unistd.h>
5​#include <errno.h>
6​
7​// needed for umask application
8​#include <sys/types.h>
9​#include <sys/stat.h>
10​#endif
11​
12​#include <QString>
13​#include <QFile>
14​#include <QDir>
15​#include <QFileInfo>
16​#include <QDebug>
17​
18​KAUTH_HELPER_MAIN("org.kde.ktexteditor.katetextbuffer", SecureTextBuffer)
19​
20​ActionReply SecureTextBuffer::movefile(const QVariantMap& args)
21​{
22​ return movefileAction(args);
23​}
24​
25​ActionReply SecureTextBuffer::movefileAction(const QVariantMap& args)
26​{
27​ const QString sourceFile = args[QLatin1String("sourceFile")].toString();
dfaureUnsubmitted
Done

where is stdio.h used?

dfaure: where is stdio.h used?
28​ const QString targetFile = args[QLatin1String("targetFile")].toString();
29​ const uint ownerId = (uint) args[QLatin1String("ownerId")].toInt();
30​ const uint groupId = (uint) args[QLatin1String("groupId")].toInt();
31​
32​ const bool ok = moveFileInternal(sourceFile, targetFile, ownerId, groupId);
33​
34​ return ok ? ActionReply::SuccessReply() : ActionReply::HelperErrorReply();
35​}
36​
37​void SecureTextBuffer::setUnixPermissionsAndOwner(const QString &filename, const bool newFile, const uint ownerId, const uint groupId)
38​{
39​ // QTemporaryFile sets permissions to 0600, so fixing this
dfaureUnsubmitted
Done

Isn't it possible to call setPermissions on the QTemporaryFile instance instead?

dfaure: Isn't it possible to call setPermissions on the QTemporaryFile instance instead?
40​ if (newFile) {
41​ const mode_t mask = umask(0);
42​ umask(mask);
43​
44​ const mode_t fileMode = 0666 & ~mask;
45​ chmod(QFile::encodeName(filename).constData(), fileMode);
46​ }
47​ // ensure file has the same owner and group as before
48​ if (!newFile && ownerId != (uint)-2 && groupId != (uint)-2) {
49​ const int result = chown(QFile::encodeName(filename).constData(), ownerId, groupId);
50​ // set at least correct group if owner cannot be changed
51​ if (result != 0 && errno == EPERM) {
52​ chown(QFile::encodeName(filename).constData(), getuid(), groupId);
53​ }
54​ }
55​}
56​
57​bool SecureTextBuffer::moveFileInternal(const QString &sourceFile, const QString &targetFile, const uint ownerId, const uint groupId)
58​{
59​ const bool newFile = !QFile::exists(targetFile);
60​
61​ // remove target file if there is any
dfaureUnsubmitted
Done

Not sure it matters, but exists+remove+rename is racy. The file could be deleted by another process after exists() and before remove() (which would then fail) or the file could be created by another process after the call to remove() (and then rename() would fail).

QSaveFile doesn't have these issues because it uses the POSIX rename() function which is atomic (and replaces any existing files). Unfortunately QFile doesn't expose such a method (one is supposed to use QSaveFile instead...).

dfaure: Not sure it matters, but exists+remove+rename is racy. The file could be deleted by another…
62​ if (!newFile) {
63​ if (!QFile::remove(targetFile)) {
64​ return false;
65​ }
66​ }
67​
68​ // move file
69​ if (!QFile::rename(sourceFile, targetFile)) {
70​ return false;
71​ }
72​
73​#ifndef Q_OS_WIN
74​ setUnixPermissionsAndOwner(targetFile, newFile, ownerId, groupId);
75​#endif
dfaureUnsubmitted
Done

It's Qt, not QT.

QT is QuickTime ;)

dfaure: It's Qt, not QT. QT is QuickTime ;)
76​
77​ return true;
78​}
dfaureUnsubmitted
Done

You definitely don't need QDataStream just to call read and write on QIODevices.

dfaure: You definitely don't need QDataStream just to call read and write on QIODevices.
dfaureUnsubmitted
Done

Not necessary if you use QSaveFile, which does this already.

But maybe the best solution here (to avoid the file copy) is a temp file that you can atomically rename, so let's discuss that before you get rid of all the stuff-that-QSaveFile-does ;)

dfaure: Not necessary if you use QSaveFile, which does this already. But maybe the best solution here…
dfaureUnsubmitted
Done

make this static so that the next line doesn't look to the compiler like a variable-size array (which isn't standard, although I'm not sure about C++11).

Maybe it has to be a #define even, I'm not sure.

dfaure: make this static so that the next line doesn't look to the compiler like a variable-size array…