Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid detecting permissions to be invalid on linux NTFS file system #7745

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/csync/vio/csync_vio_local.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
#ifndef _CSYNC_VIO_LOCAL_H
#define _CSYNC_VIO_LOCAL_H

#include <QString>

Check failure on line 24 in src/csync/vio/csync_vio_local.h

View workflow job for this annotation

GitHub Actions / build

src/csync/vio/csync_vio_local.h:24:10 [clang-diagnostic-error]

'QString' file not found

struct csync_vio_handle_t;
namespace OCC {
Expand All @@ -30,8 +30,8 @@

csync_vio_handle_t OCSYNC_EXPORT *csync_vio_local_opendir(const QString &name);
int OCSYNC_EXPORT csync_vio_local_closedir(csync_vio_handle_t *dhandle);
std::unique_ptr<csync_file_stat_t> OCSYNC_EXPORT csync_vio_local_readdir(csync_vio_handle_t *dhandle, OCC::Vfs *vfs);
std::unique_ptr<csync_file_stat_t> OCSYNC_EXPORT csync_vio_local_readdir(csync_vio_handle_t *dhandle, OCC::Vfs *vfs, bool checkPermissionsValidity);

Check warning on line 33 in src/csync/vio/csync_vio_local.h

View workflow job for this annotation

GitHub Actions / build

src/csync/vio/csync_vio_local.h:33:1 [cppcoreguidelines-avoid-non-const-global-variables]

variable 'std' is non-const and globally accessible, consider making it const

int OCSYNC_EXPORT csync_vio_local_stat(const QString &uri, csync_file_stat_t *buf);
int OCSYNC_EXPORT csync_vio_local_stat(const QString &uri, csync_file_stat_t *buf, bool checkPermissionsValidity);

Check warning on line 35 in src/csync/vio/csync_vio_local.h

View workflow job for this annotation

GitHub Actions / build

src/csync/vio/csync_vio_local.h:35:5 [cppcoreguidelines-avoid-non-const-global-variables]

variable 'OCSYNC_EXPORT' is non-const and globally accessible, consider making it const

#endif /* _CSYNC_VIO_LOCAL_H */
20 changes: 11 additions & 9 deletions src/csync/vio/csync_vio_local_unix.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*

Check notice on line 1 in src/csync/vio/csync_vio_local_unix.cpp

View workflow job for this annotation

GitHub Actions / build

Run clang-format on src/csync/vio/csync_vio_local_unix.cpp

File src/csync/vio/csync_vio_local_unix.cpp does not conform to Custom style guidelines. (lines 38, 74)
* libcsync -- a library to sync a directory with another
*
* Copyright (c) 2008-2013 by Andreas Schneider <[email protected]>
Expand Down Expand Up @@ -28,15 +28,15 @@

#include <memory>

#include "c_private.h"

Check failure on line 31 in src/csync/vio/csync_vio_local_unix.cpp

View workflow job for this annotation

GitHub Actions / build

src/csync/vio/csync_vio_local_unix.cpp:31:10 [clang-diagnostic-error]

'c_private.h' file not found
#include "c_lib.h"
#include "csync.h"

#include "vio/csync_vio_local.h"
#include "common/vfs.h"

#include <QtCore/QLoggingCategory>
#include <QtCore/QFile>
#include <QLoggingCategory>
#include <QFile>

Q_LOGGING_CATEGORY(lcCSyncVIOLocal, "nextcloud.sync.csync.vio_local", QtInfoMsg)

Expand All @@ -49,7 +49,7 @@
QByteArray path;
};

static int _csync_vio_local_stat_mb(const mbchar_t *wuri, csync_file_stat_t *buf);
static int _csync_vio_local_stat_mb(const mbchar_t *wuri, csync_file_stat_t *buf, bool checkPermissionsValidity);

Check warning on line 52 in src/csync/vio/csync_vio_local_unix.cpp

View workflow job for this annotation

GitHub Actions / build

src/csync/vio/csync_vio_local_unix.cpp:52:12 [bugprone-reserved-identifier]

declaration uses identifier '_csync_vio_local_stat_mb', which is reserved in the global namespace

Check warning on line 52 in src/csync/vio/csync_vio_local_unix.cpp

View workflow job for this annotation

GitHub Actions / build

src/csync/vio/csync_vio_local_unix.cpp:52:12 [modernize-use-trailing-return-type]

use a trailing return type for this function

csync_vio_handle_t *csync_vio_local_opendir(const QString &name) {
auto handle = std::make_unique<csync_vio_handle_t>();
Expand All @@ -71,7 +71,7 @@
return rc;
}

std::unique_ptr<csync_file_stat_t> csync_vio_local_readdir(csync_vio_handle_t *handle, OCC::Vfs *vfs) {
std::unique_ptr<csync_file_stat_t> csync_vio_local_readdir(csync_vio_handle_t *handle, OCC::Vfs *vfs, bool checkPermissionsValidity) {

Check warning on line 74 in src/csync/vio/csync_vio_local_unix.cpp

View workflow job for this annotation

GitHub Actions / build

src/csync/vio/csync_vio_local_unix.cpp:74:36 [modernize-use-trailing-return-type]

use a trailing return type for this function

struct _tdirent *dirent = nullptr;
std::unique_ptr<csync_file_stat_t> file_stat;
Expand Down Expand Up @@ -114,7 +114,7 @@
if (file_stat->path.isNull())
return file_stat;

if (_csync_vio_local_stat_mb(fullPath.constData(), file_stat.get()) < 0) {
if (_csync_vio_local_stat_mb(fullPath.constData(), file_stat.get(), checkPermissionsValidity) < 0) {
// Will get excluded by _csync_detect_update.
file_stat->type = ItemTypeSkip;
}
Expand All @@ -131,12 +131,12 @@
}


int csync_vio_local_stat(const QString &uri, csync_file_stat_t *buf)
int csync_vio_local_stat(const QString &uri, csync_file_stat_t *buf, bool checkPermissionsValidity)

Check warning on line 134 in src/csync/vio/csync_vio_local_unix.cpp

View workflow job for this annotation

GitHub Actions / build

src/csync/vio/csync_vio_local_unix.cpp:134:5 [modernize-use-trailing-return-type]

use a trailing return type for this function
{
return _csync_vio_local_stat_mb(QFile::encodeName(uri).constData(), buf);
return _csync_vio_local_stat_mb(QFile::encodeName(uri).constData(), buf, checkPermissionsValidity);
}

static int _csync_vio_local_stat_mb(const mbchar_t *wuri, csync_file_stat_t *buf)
static int _csync_vio_local_stat_mb(const mbchar_t *wuri, csync_file_stat_t *buf, bool checkPermissionsValidity)

Check warning on line 139 in src/csync/vio/csync_vio_local_unix.cpp

View workflow job for this annotation

GitHub Actions / build

src/csync/vio/csync_vio_local_unix.cpp:139:12 [bugprone-reserved-identifier]

declaration uses identifier '_csync_vio_local_stat_mb', which is reserved in the global namespace

Check warning on line 139 in src/csync/vio/csync_vio_local_unix.cpp

View workflow job for this annotation

GitHub Actions / build

src/csync/vio/csync_vio_local_unix.cpp:139:12 [modernize-use-trailing-return-type]

use a trailing return type for this function
{
csync_stat_t sb;

Expand Down Expand Up @@ -169,7 +169,9 @@
buf->inode = sb.st_ino;
buf->modtime = sb.st_mtime;
buf->size = sb.st_size;
buf->isPermissionsInvalid = (sb.st_mode & S_IWOTH) == S_IWOTH;
if (checkPermissionsValidity) {
buf->isPermissionsInvalid = (sb.st_mode & S_IWOTH) == S_IWOTH;
}

return 0;
}
4 changes: 2 additions & 2 deletions src/libsync/discovery.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*

Check notice on line 1 in src/libsync/discovery.cpp

View workflow job for this annotation

GitHub Actions / build

Run clang-format on src/libsync/discovery.cpp

File src/libsync/discovery.cpp does not conform to Custom style guidelines. (lines 2140)
* Copyright (C) by Olivier Goffart <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
Expand Down Expand Up @@ -943,7 +943,7 @@

if (!base.isDirectory()) {
csync_file_stat_t buf;
if (csync_vio_local_stat(_discoveryData->_localDir + originalPathAdjusted, &buf)) {
if (csync_vio_local_stat(_discoveryData->_localDir + originalPathAdjusted, &buf, true)) {
qCInfo(lcDisco) << "Local file does not exist anymore." << originalPathAdjusted;
return;
}
Expand Down Expand Up @@ -2137,7 +2137,7 @@
void ProcessDirectoryJob::startAsyncLocalQuery()
{
QString localPath = _discoveryData->_localDir + _currentFolder._local;
auto localJob = new DiscoverySingleLocalDirectoryJob(_discoveryData->_account, localPath, _discoveryData->_syncOptions._vfs.data());
auto localJob = new DiscoverySingleLocalDirectoryJob(_discoveryData->_account, localPath, _discoveryData->_syncOptions._vfs.data(), _discoveryData->_fileSystemReliablePermissions);

_discoveryData->_currentlyActiveJobs++;
_pendingAsyncJobs++;
Expand Down
15 changes: 12 additions & 3 deletions src/libsync/discoveryphase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,17 @@
}
}

DiscoverySingleLocalDirectoryJob::DiscoverySingleLocalDirectoryJob(const AccountPtr &account, const QString &localPath, OCC::Vfs *vfs, QObject *parent)
: QObject(parent), QRunnable(), _localPath(localPath), _account(account), _vfs(vfs)
DiscoverySingleLocalDirectoryJob::DiscoverySingleLocalDirectoryJob(const AccountPtr &account,

Check warning on line 287 in src/libsync/discoveryphase.cpp

View workflow job for this annotation

GitHub Actions / build

src/libsync/discoveryphase.cpp:287:1 [cppcoreguidelines-pro-type-member-init]

constructor does not initialize these fields: , , _account, _vfs

Check warning on line 287 in src/libsync/discoveryphase.cpp

View workflow job for this annotation

GitHub Actions / build

src/libsync/discoveryphase.cpp:287:68 [bugprone-easily-swappable-parameters]

2 adjacent parameters of 'DiscoverySingleLocalDirectoryJob' of similar type ('const int &') are easily swapped by mistake
const QString &localPath,
OCC::Vfs *vfs,
bool fileSystemReliablePermissions,
QObject *parent)
: QObject{parent}
, QRunnable{}
, _localPath{localPath}
, _account{account}
, _vfs{vfs}
, _fileSystemReliablePermissions{fileSystemReliablePermissions}
{
qRegisterMetaType<QVector<OCC::LocalInfo> >("QVector<OCC::LocalInfo>");
}
Expand Down Expand Up @@ -318,7 +327,7 @@
QVector<LocalInfo> results;
while (true) {
errno = 0;
auto dirent = csync_vio_local_readdir(dh, _vfs);
auto dirent = csync_vio_local_readdir(dh, _vfs, _fileSystemReliablePermissions);
if (!dirent)
break;
if (dirent->type == ItemTypeSkip)
Expand Down
9 changes: 8 additions & 1 deletion src/libsync/discoveryphase.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*

Check notice on line 1 in src/libsync/discoveryphase.h

View workflow job for this annotation

GitHub Actions / build

Run clang-format on src/libsync/discoveryphase.h

File src/libsync/discoveryphase.h does not conform to Custom style guidelines. (lines 141)
* Copyright (C) by Olivier Goffart <[email protected]>
*
* This program is free software; you can redistribute it and/or modify
Expand All @@ -14,7 +14,7 @@

#pragma once

#include <QObject>

Check failure on line 17 in src/libsync/discoveryphase.h

View workflow job for this annotation

GitHub Actions / build

src/libsync/discoveryphase.h:17:10 [clang-diagnostic-error]

'QObject' file not found
#include <QElapsedTimer>
#include <QStringList>
#include <csync.h>
Expand Down Expand Up @@ -119,7 +119,11 @@
{
Q_OBJECT
public:
explicit DiscoverySingleLocalDirectoryJob(const AccountPtr &account, const QString &localPath, OCC::Vfs *vfs, QObject *parent = nullptr);
explicit DiscoverySingleLocalDirectoryJob(const AccountPtr &account,
const QString &localPath,
OCC::Vfs *vfs,
bool fileSystemReliablePermissions,
QObject *parent = nullptr);

void run() override;
signals:
Expand All @@ -134,6 +138,7 @@
QString _localPath;
AccountPtr _account;
OCC::Vfs* _vfs;
bool _fileSystemReliablePermissions = false;
public:
};

Expand Down Expand Up @@ -343,6 +348,8 @@

bool _noCaseConflictRecordsInDb = false;

bool _fileSystemReliablePermissions = false;

QSet<QString> _topLevelE2eeFolderPaths;

signals:
Expand Down
4 changes: 2 additions & 2 deletions src/libsync/filesystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ time_t FileSystem::getModTime(const QString &filename)
{
csync_file_stat_t stat;
time_t result = -1;
if (csync_vio_local_stat(filename, &stat) != -1 && (stat.modtime != 0)) {
if (csync_vio_local_stat(filename, &stat, true) != -1 && (stat.modtime != 0)) {
result = stat.modtime;
} else {
result = Utility::qDateTimeToTime_t(QFileInfo(filename).lastModified());
Expand Down Expand Up @@ -319,7 +319,7 @@ bool FileSystem::removeRecursively(const QString &path, const std::function<void
bool FileSystem::getInode(const QString &filename, quint64 *inode)
{
csync_file_stat_t fs;
if (csync_vio_local_stat(filename, &fs) == 0) {
if (csync_vio_local_stat(filename, &fs, true) == 0) {
*inode = fs.inode;
return true;
}
Expand Down
14 changes: 14 additions & 0 deletions src/libsync/syncengine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
#include <QProcess>
#include <QElapsedTimer>
#include <QFileInfo>
#include <QStorageInfo>
#include <qtextcodec.h>

namespace OCC {
Expand Down Expand Up @@ -638,6 +639,19 @@ void SyncEngine::startSync()
_remnantReadOnlyFolders.clear();

_discoveryPhase = std::make_unique<DiscoveryPhase>();

#if defined Q_OS_LINUX
const auto fileSystemInfo = QStorageInfo{_localPath};
qCInfo(lcEngine()) << "File system type for current sync folder:" << fileSystemInfo.fileSystemType();
if (fileSystemInfo.fileSystemType() == "NTFS") {
Copy link

@mbiebl mbiebl Jan 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting

2025-01-10 15:44:55:888 [ info nextcloud.sync.engine /drone/src/src/libsync/syncengine.cpp:645 ]:↦      File system type for current sync folder: "fuseblk"
michael@mars:~$ findmnt /mnt/data 
TARGET    SOURCE          FSTYPE  OPTIONS
/mnt/data /dev/nvme0n1p11 fuseblk rw,relatime,user_id=0,group_id=0,allow_other,blksize=4096
michael@mars:~$ grep data /etc/fstab 
LABEL=Data      /mnt/data       ntfs-3g auto            0       0

_discoveryPhase->_fileSystemReliablePermissions = false;
} else {
_discoveryPhase->_fileSystemReliablePermissions = true;
}
#else
_discoveryPhase->_fileSystemReliablePermissions = true;
#endif

_discoveryPhase->_leadingAndTrailingSpacesFilesAllowed = _leadingAndTrailingSpacesFilesAllowed;
_discoveryPhase->_account = _account;
_discoveryPhase->_excludes = _excludedFiles.data();
Expand Down
19 changes: 12 additions & 7 deletions test/csync/vio_tests/check_vio_ext.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/*

Check notice on line 1 in test/csync/vio_tests/check_vio_ext.cpp

View workflow job for this annotation

GitHub Actions / build

Run clang-format on test/csync/vio_tests/check_vio_ext.cpp

File test/csync/vio_tests/check_vio_ext.cpp does not conform to Custom style guidelines. (lines 164)
* libcsync -- a library to sync a directory with another
*
* Copyright (c) 2015-2013 by Klaas Freitag <[email protected]>
Expand All @@ -24,7 +24,7 @@
#include <cerrno>
#include <cstdio>

#include "csync.h"

Check failure on line 27 in test/csync/vio_tests/check_vio_ext.cpp

View workflow job for this annotation

GitHub Actions / build

test/csync/vio_tests/check_vio_ext.cpp:27:10 [clang-diagnostic-error]

'csync.h' file not found
#include "vio/csync_vio_local.h"

#include <QDir>
Expand Down Expand Up @@ -147,7 +147,7 @@
* whole tree.
*
*/
static void traverse_dir(void **state, const QString &dir, int *cnt)
static void traverse_dir(void **state, const QString &dir, int *cnt, bool checkPermissionsValidity)
{
csync_vio_handle_t *dh = nullptr;
std::unique_ptr<csync_file_stat_t> dirent;
Expand All @@ -161,7 +161,7 @@
assert_non_null(dh);

OCC::Vfs *vfs = nullptr;
while( (dirent = csync_vio_local_readdir(dh, vfs)) ) {
while( (dirent = csync_vio_local_readdir(dh, vfs, checkPermissionsValidity)) ) {
assert_non_null(dirent.get());
if (!dirent->original_path.isEmpty()) {
sv->ignored_dir = dirent->original_path;
Expand Down Expand Up @@ -190,7 +190,7 @@
}
output(subdir_out.constData());
if( is_dir ) {
traverse_dir(state, QString::fromUtf8(subdir), cnt);
traverse_dir(state, QString::fromUtf8(subdir), cnt, checkPermissionsValidity);
}
}

Expand All @@ -210,11 +210,12 @@
{
auto sv = (statevar*) *state;

bool checkPermissionsValidity = true;
const char *t1 = "alibaba/und/die/vierzig/räuber/";
create_dirs( t1 );
int files_cnt = 0;

traverse_dir(state, CSYNC_TEST_DIR, &files_cnt);
traverse_dir(state, CSYNC_TEST_DIR, &files_cnt, checkPermissionsValidity);

assert_string_equal(sv->result.constData(),
QString::fromUtf8("<DIR> %1/alibaba"
Expand All @@ -233,14 +234,15 @@
auto sv = (statevar*) *state;
int files_cnt = 0;

bool checkPermissionsValidity = true;
const char *t1 = "warum/nur/40/Räuber/";
create_dirs( t1 );

create_file( t1, "Räuber Max.txt", "Der Max ist ein schlimmer finger");
create_file( t1, "пя́тница.txt", "Am Freitag tanzt der Ürk");


traverse_dir(state, CSYNC_TEST_DIR, &files_cnt);
traverse_dir(state, CSYNC_TEST_DIR, &files_cnt, checkPermissionsValidity);

assert_string_equal(sv->result.constData(),
QString::fromUtf8("<DIR> %1/warum"
Expand All @@ -259,6 +261,8 @@
{
auto sv = (statevar*) *state;

bool checkPermissionsValidity = true;

/* Strange things here: Compilers only support strings with length of 4k max.
* The expected result string is longer, so it needs to be split up in r1, r2 and r3
*/
Expand Down Expand Up @@ -318,7 +322,7 @@
/* assemble the result string ... */
const auto result = (r1 + r2 + r3).toUtf8();
int files_cnt = 0;
traverse_dir(state, CSYNC_TEST_DIR, &files_cnt);
traverse_dir(state, CSYNC_TEST_DIR, &files_cnt, checkPermissionsValidity);
assert_int_equal(files_cnt, 0);
/* and compare. */
assert_string_equal(sv->result.constData(), result.constData());
Expand All @@ -333,6 +337,7 @@
// 3: ? ASCII: 191 - BF
// 4: ASCII: 32 - 20

bool checkPermissionsValidity = true;
QString p = QStringLiteral("%1/%2").arg(CSYNC_TEST_DIR, QStringLiteral("goodone/"));
int rc = oc_mkdir(p);
assert_int_equal(rc, 0);
Expand All @@ -344,7 +349,7 @@
assert_int_equal(rc, 0);

int files_cnt = 0;
traverse_dir(state, CSYNC_TEST_DIR, &files_cnt);
traverse_dir(state, CSYNC_TEST_DIR, &files_cnt, checkPermissionsValidity);
const auto expected_result = QStringLiteral("<DIR> %1/goodone"
"<DIR> %1/goodone/ugly\xEF\xBB\xBF\x32.txt")
.arg(CSYNC_TEST_DIR);
Expand Down
2 changes: 1 addition & 1 deletion test/testlongpath.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* License along with this library; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
*/
#include "common/filesystembase.h"

Check failure on line 20 in test/testlongpath.cpp

View workflow job for this annotation

GitHub Actions / build

test/testlongpath.cpp:20:10 [clang-diagnostic-error]

'common/filesystembase.h' file not found
#include "csync/csync.h"
#include "csync/vio/csync_vio_local.h"
#include "logger.h"
Expand Down Expand Up @@ -139,7 +139,7 @@
file.close();

csync_file_stat_t buf;
QVERIFY(csync_vio_local_stat(longPath.filePath(), &buf) != -1);
QVERIFY(csync_vio_local_stat(longPath.filePath(), &buf, true) != -1);
QVERIFY(buf.size == data.size());
QVERIFY(buf.size == longPath.size());

Expand Down
Loading