Differential D4847 Diff 12251 src/buffer/katesecuretextbuffer.cpp

Changeset View

Standalone View

View Options

src/buffer/katesecuretextbuffer.cpp

  • This file was added.
1​/* This file is part of the KTextEditor project.
2​ *
dfaureUnsubmitted
Done

Missing copyright header

dfaure: Missing copyright header
3​ * Copyright (C) 2017 KDE Developers
4​ *
5​ * This library is free software; you can redistribute it and/or
6​ * modify it under the terms of the GNU Library General Public
7​ * License as published by the Free Software Foundation; either
8​ * version 2 of the License, or (at your option) any later version.
9​ *
10​ * This library is distributed in the hope that it will be useful,
11​ * but WITHOUT ANY WARRANTY; without even the implied warranty of
12​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
13​ * Library General Public License for more details.
14​ *
15​ * You should have received a copy of the GNU Library General Public License
16​ * along with this library; see the file COPYING.LIB. If not, write to
17​ * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
18​ * Boston, MA 02110-1301, USA.
19​ */
20​
21​#include "katesecuretextbuffer_p.h"
22​
23​#ifndef Q_OS_WIN
24​#include <unistd.h>
25​#include <errno.h>
26​
27​// needed for umask application
dfaureUnsubmitted
Done

where is stdio.h used?

dfaure: where is stdio.h used?
28​#include <sys/types.h>
29​#include <sys/stat.h>
30​#endif
31​
32​#include <QString>
33​#include <QFile>
34​#include <QSaveFile>
35​#include <QDataStream>
36​
37​KAUTH_HELPER_MAIN("org.kde.ktexteditor.katetextbuffer", SecureTextBuffer)
38​
39​ActionReply SecureTextBuffer::movefile(const QVariantMap &args)
40​{
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?
41​ return movefileAction(args);
42​}
43​
44​ActionReply SecureTextBuffer::movefileAction(const QVariantMap &args)
45​{
46​ const QString sourceFile = args[QLatin1String("sourceFile")].toString();
47​ const QString targetFile = args[QLatin1String("targetFile")].toString();
48​ const uint ownerId = (uint) args[QLatin1String("ownerId")].toInt();
49​ const uint groupId = (uint) args[QLatin1String("groupId")].toInt();
50​
51​ const bool ok = moveFileInternal(sourceFile, targetFile, ownerId, groupId);
52​
53​ return ok ? ActionReply::SuccessReply() : ActionReply::HelperErrorReply();
54​}
55​
56​void SecureTextBuffer::setUnixOwner(const QString &filename, const uint ownerId, const uint groupId)
57​{
58​ if (ownerId != (uint)-2 && groupId != (uint)-2) {
59​ const int result = chown(QFile::encodeName(filename).constData(), ownerId, groupId);
60​ // set at least correct group if owner cannot be changed
61​ if (result != 0 && errno == EPERM) {
62​ chown(QFile::encodeName(filename).constData(), getuid(), groupId);
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…
63​ }
64​ }
65​}
66​
67​bool SecureTextBuffer::moveFileInternal(const QString &sourceFile, const QString &targetFile, const uint ownerId, const uint groupId)
68​{
69​#ifndef Q_OS_WIN
70​ const bool newFile = !QFile::exists(targetFile);
71​#endif
72​
73​ /**
74​ * There is no atomic rename operation publicly exposed by QT.
dfaureUnsubmitted
Done

It's Qt, not QT.

QT is QuickTime ;)

dfaure: It's Qt, not QT. QT is QuickTime ;)
75​ *
76​ * We also cannot use std::rename since sourceFile is usually saved on different device -> RAM (tmpfs).
77​ *
78​ * Therefore we are copying source file to destination with the help of QSaveFile
79​ * to ensure targetFile is overwritten safely.
80​ */
81​ QFile readFile(sourceFile);
82​ QSaveFile saveFile(targetFile);
83​ if (!readFile.open(QIODevice::ReadOnly) || !saveFile.open(QIODevice::WriteOnly)) {
84​ return false;
85​ }
86​ const int len = 4096;
87​ char buffer[len];
88​ QDataStream dataStreamFrom(&readFile);
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.
89​ QDataStream dataStreamTo(&saveFile);
90​ int read = -1;
91​ while ((read = dataStreamFrom.readRawData(buffer, len)) > 0) {
92​ if (dataStreamTo.writeRawData(buffer, read) == -1) {
93​ return false;
94​ }
95​ }
96​ if (!saveFile.flush()) {
97​ return false;
98​ }
99​
100​#ifndef Q_OS_WIN
101​ // ensure that the file is written to disk
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…
102​#ifdef HAVE_FDATASYNC
103​ fdatasync(saveFile.handle());
104​#else
105​ fsync(saveFile.handle());
106​#endif
107​#endif
108​
109​ if (dataStreamTo.status() != QDataStream::Ok) {
110​ return false;
111​ }
112​
113​ saveFile.commit();
114​
115​#ifndef Q_OS_WIN
116​ if (!newFile) {
117​ // ensure file has the same owner and group as before
118​ setUnixOwner(targetFile, ownerId, groupId);
119​ }
120​#endif
121​
122​ return true;
123​}