Differential D4847 Diff 12309 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​#include <stdio.h>
dfaureUnsubmitted
Done

where is stdio.h used?

dfaure: where is stdio.h used?
27​#endif
28​
29​#include <QString>
30​#include <QFile>
31​#include <QTemporaryFile>
32​#include <QSaveFile>
33​
34​KAUTH_HELPER_MAIN("org.kde.ktexteditor.katetextbuffer", SecureTextBuffer)
35​
36​ActionReply SecureTextBuffer::savefile(const QVariantMap &args)
37​{
38​ const ActionMode actionMode = static_cast<ActionMode>(args[QLatin1String("actionMode")].toInt());
39​ const QString targetFile = args[QLatin1String("targetFile")].toString();
40​ const uint ownerId = (uint) args[QLatin1String("ownerId")].toInt();
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​
42​ if (actionMode == ActionMode::Prepare) {
43​
44​ const QString temporaryFile = prepareTempFileInternal(targetFile, ownerId);
45​
46​ if (temporaryFile.isEmpty()) {
47​ return ActionReply::HelperErrorReply();
48​ }
49​
50​ ActionReply reply;
51​ reply.addData(QLatin1String("temporaryFile"), temporaryFile);
52​
53​ return reply;
54​
55​ }
56​
57​ if (actionMode == ActionMode::Move) {
58​
59​ const QString sourceFile = args[QLatin1String("sourceFile")].toString();
60​ const uint groupId = (uint) args[QLatin1String("groupId")].toInt();
61​
62​ if (moveFileInternal(sourceFile, targetFile, ownerId, 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​ return ActionReply::SuccessReply();
64​ }
65​ }
66​
67​ return ActionReply::HelperErrorReply();
68​}
69​
70​bool SecureTextBuffer::moveFileInternal(const QString &sourceFile, const QString &targetFile, const uint ownerId, const uint groupId)
71​{
72​ const bool newFile = !QFile::exists(targetFile);
73​ bool atomicRenameSucceeded = false;
74​
75​ /**
dfaureUnsubmitted
Done

It's Qt, not QT.

QT is QuickTime ;)

dfaure: It's Qt, not QT. QT is QuickTime ;)
76​ * There is no atomic rename operation publicly exposed by Qt.
77​ *
78​ * We use std::rename for UNIX and for now no-op for windows (triggers fallback).
79​ *
80​ * As fallback we are copying source file to destination with the help of QSaveFile
81​ * to ensure targetFile is overwritten safely.
82​ */
83​#ifndef Q_OS_WIN
84​ const int result = std::rename(QFile::encodeName(sourceFile).constData(), QFile::encodeName(targetFile).constData());
85​ if (result == 0) {
86​ syncToDisk(QFile(targetFile).handle());
87​ atomicRenameSucceeded = true;
88​ }
89​#else
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.
90​ atomicRenameSucceeded = false;
91​#endif
92​
93​ if (!atomicRenameSucceeded) {
94​ // as fallback copy the temporary file to the target with help of QSaveFile
95​ QFile readFile(sourceFile);
96​ QSaveFile saveFile(targetFile);
97​ if (!readFile.open(QIODevice::ReadOnly) || !saveFile.open(QIODevice::WriteOnly)) {
98​ return false;
99​ }
100​ const qint64 len = 4096;
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…
101​ char buffer[len];
102​ qint64 read = -1;
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…
103​ while ((read = readFile.read(buffer, len)) > 0) {
104​ if (saveFile.write(buffer, read) == -1) {
105​ return false;
106​ }
107​ }
108​ if (read == -1 || !saveFile.commit()) {
109​ return false;
110​ }
111​ }
112​
113​ if (!newFile) {
114​ // ensure file has the same owner and group as before
115​ setOwner(targetFile, ownerId, groupId);
116​ }
117​
118​ return true;
119​}
120​
121​QString SecureTextBuffer::prepareTempFileInternal(const QString &targetFile, const uint ownerId)
122​{
123​ QTemporaryFile tempFile(targetFile);
124​ if (!tempFile.open()) {
125​ return QString();
126​ }
127​ tempFile.setAutoRemove(false);
128​ setOwner(tempFile.fileName(), ownerId, -1);
129​ return tempFile.fileName();
130​}
131​
132​void SecureTextBuffer::setOwner(const QString &filename, const uint ownerId, const uint groupId)
133​{
134​#ifndef Q_OS_WIN
135​ if (ownerId != (uint)-2 && groupId != (uint)-2) {
136​ const int result = chown(QFile::encodeName(filename).constData(), ownerId, groupId);
137​ // set at least correct group if owner cannot be changed
138​ if (result != 0 && errno == EPERM) {
139​ chown(QFile::encodeName(filename).constData(), getuid(), groupId);
140​ }
141​ }
142​#else
143​ // no-op for windows
144​#endif
145​}
146​
147​void SecureTextBuffer::syncToDisk(const int fd)
148​{
149​#ifndef Q_OS_WIN
150​#ifdef HAVE_FDATASYNC
151​ fdatasync(fd);
152​#else
153​ fsync(fd);
154​#endif
155​#else
156​ // no-op for windows
157​#endif
158​}
159​No newline at end of file