From: allexzander Date: Thu, 21 Jan 2021 11:19:41 +0000 (+0200) Subject: Fix folder upload issue due to wrong Lock/Unlock order X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1~1^2~12^2~21^2~406^2~1 X-Git-Url: https://dgit.raspbian.org/?a=commitdiff_plain;h=483a874cb6403176fb8b53a36dc1b4b795967355;p=nextcloud-desktop.git Fix folder upload issue due to wrong Lock/Unlock order Signed-off-by: allexzander --- diff --git a/src/libsync/propagateremotemkdir.cpp b/src/libsync/propagateremotemkdir.cpp index 4ce07ddac..beb33e244 100644 --- a/src/libsync/propagateremotemkdir.cpp +++ b/src/libsync/propagateremotemkdir.cpp @@ -32,7 +32,6 @@ PropagateRemoteMkdir::PropagateRemoteMkdir(OwncloudPropagator *propagator, const : PropagateItemJob(propagator, item) , _deleteExisting(false) , _uploadEncryptedHelper(nullptr) - , _isEncryptedRootFolder(_item->_isEncrypted && _item->_encryptedFileName.isEmpty()) { const auto path = _item->_file; const auto slashPosition = path.lastIndexOf('/'); @@ -96,8 +95,6 @@ void PropagateRemoteMkdir::slotStartEncryptedMkcolJob(const QString &path, const propagator()->fullRemotePath(filename), {{"e2e-token", _uploadEncryptedHelper->_folderToken }}, this); - connect(job, qOverload(&MkColJob::finished), - _uploadEncryptedHelper, &PropagateUploadEncrypted::unlockFolder); connect(job, qOverload(&MkColJob::finished), this, &PropagateRemoteMkdir::slotMkcolJobFinished); _job = job; @@ -119,6 +116,57 @@ void PropagateRemoteMkdir::setDeleteExisting(bool enabled) _deleteExisting = enabled; } +void PropagateRemoteMkdir::finalizeMkColJob(QNetworkReply::NetworkError err, const QString &jobHttpReasonPhraseString, const QString &jobPath) +{ + if (_item->_httpErrorCode == 405) { + // This happens when the directory already exists. Nothing to do. + } else if (err != QNetworkReply::NoError) { + SyncFileItem::Status status = classifyError(err, _item->_httpErrorCode, + &propagator()->_anotherSyncNeeded); + done(status, _item->_errorString); + return; + } else if (_item->_httpErrorCode != 201) { + // Normally we expect "201 Created" + // If it is not the case, it might be because of a proxy or gateway intercepting the request, so we must + // throw an error. + done(SyncFileItem::NormalError, + tr("Wrong HTTP code returned by server. Expected 201, but received \"%1 %2\".") + .arg(_item->_httpErrorCode) + .arg(jobHttpReasonPhraseString)); + return; + } + + if (_item->_fileId.isEmpty()) { + // Owncloud 7.0.0 and before did not have a header with the file id. + // (https://github.com/owncloud/core/issues/9000) + // So we must get the file id using a PROPFIND + // This is required so that we can detect moves even if the folder is renamed on the server + // while files are still uploading + propagator()->_activeJobList.append(this); + auto propfindJob = new PropfindJob(propagator()->account(), jobPath, this); + propfindJob->setProperties(QList() << "http://owncloud.org/ns:id"); + QObject::connect(propfindJob, &PropfindJob::result, this, &PropagateRemoteMkdir::propfindResult); + QObject::connect(propfindJob, &PropfindJob::finishedWithError, this, &PropagateRemoteMkdir::propfindError); + propfindJob->start(); + _job = propfindJob; + return; + } + + if (!_uploadEncryptedHelper && !_item->_isEncrypted) { + success(); + } else { + // We still need to mark that folder encrypted + propagator()->_activeJobList.append(this); + + // We're expecting directory path in /Foo/Bar convention... + Q_ASSERT(jobPath.startsWith('/') && !jobPath.endsWith('/')); + // But encryption job expect it in Foo/Bar/ convention + auto job = new OCC::EncryptFolderJob(propagator()->account(), propagator()->_journal, jobPath.mid(1), _item->_fileId, this); + connect(job, &OCC::EncryptFolderJob::finished, this, &PropagateRemoteMkdir::slotEncryptFolderFinished); + job->start(); + } +} + void PropagateRemoteMkdir::slotMkdir() { const auto path = _item->_file; @@ -158,56 +206,21 @@ void PropagateRemoteMkdir::slotMkcolJobFinished() _item->_responseTimeStamp = _job->responseTimestamp(); _item->_requestId = _job->requestId(); - if (_item->_httpErrorCode == 405) { - // This happens when the directory already exists. Nothing to do. - } else if (err != QNetworkReply::NoError) { - SyncFileItem::Status status = classifyError(err, _item->_httpErrorCode, - &propagator()->_anotherSyncNeeded); - done(status, _job->errorString()); - return; - } else if (_item->_httpErrorCode != 201) { - // Normally we expect "201 Created" - // If it is not the case, it might be because of a proxy or gateway intercepting the request, so we must - // throw an error. - done(SyncFileItem::NormalError, - tr("Wrong HTTP code returned by server. Expected 201, but received \"%1 %2\".") - .arg(_item->_httpErrorCode) - .arg(_job->reply()->attribute(QNetworkRequest::HttpReasonPhraseAttribute).toString())); - return; - } - _item->_fileId = _job->reply()->rawHeader("OC-FileId"); - if (_item->_fileId.isEmpty()) { - // Owncloud 7.0.0 and before did not have a header with the file id. - // (https://github.com/owncloud/core/issues/9000) - // So we must get the file id using a PROPFIND - // This is required so that we can detect moves even if the folder is renamed on the server - // while files are still uploading - propagator()->_activeJobList.append(this); - auto propfindJob = new PropfindJob(_job->account(), _job->path(), this); - propfindJob->setProperties(QList() << "http://owncloud.org/ns:id"); - QObject::connect(propfindJob, &PropfindJob::result, this, &PropagateRemoteMkdir::propfindResult); - QObject::connect(propfindJob, &PropfindJob::finishedWithError, this, &PropagateRemoteMkdir::propfindError); - propfindJob->start(); - _job = propfindJob; - return; - } + _item->_errorString = _job->errorString(); - if (!_uploadEncryptedHelper && !_isEncryptedRootFolder) { - success(); - } else { - // We still need to mark that folder encrypted - propagator()->_activeJobList.append(this); + const auto jobHttpReasonPhraseString = _job->reply()->attribute(QNetworkRequest::HttpReasonPhraseAttribute).toString(); - // We're expecting directory path in /Foo/Bar convention... - Q_ASSERT(_job->path().startsWith('/') && !_job->path().endsWith('/')); - // But encryption job expect it in Foo/Bar/ convention - const auto path = _isEncryptedRootFolder ? _job->path() : _job->path().mid(1); + const auto jobPath = _job->path(); - auto job = new OCC::EncryptFolderJob(propagator()->account(), propagator()->_journal, path, _item->_fileId, this); - connect(job, &OCC::EncryptFolderJob::finished, this, &PropagateRemoteMkdir::slotEncryptFolderFinished); - job->start(); + if (_uploadEncryptedHelper && !_uploadEncryptedHelper->_folderId.isEmpty()) { + connect(_uploadEncryptedHelper, &PropagateUploadEncrypted::folderUnlocked, this, [this, err, jobHttpReasonPhraseString, jobPath]() { + finalizeMkColJob(err, jobHttpReasonPhraseString, jobPath); + }); + _uploadEncryptedHelper->unlockFolder(); + } else { + finalizeMkColJob(err, jobHttpReasonPhraseString, jobPath); } } diff --git a/src/libsync/propagateremotemkdir.h b/src/libsync/propagateremotemkdir.h index 1cbe4defe..9b75e005d 100644 --- a/src/libsync/propagateremotemkdir.h +++ b/src/libsync/propagateremotemkdir.h @@ -59,6 +59,9 @@ private slots: void success(); private: - bool _isEncryptedRootFolder = false; + void finalizeMkColJob(QNetworkReply::NetworkError err, const QString &jobHttpReasonPhraseString, const QString &jobPath); + +private: + QPair _status = { SyncFileItem::NoStatus, QString() }; }; } diff --git a/src/libsync/propagateremotemove.cpp b/src/libsync/propagateremotemove.cpp index 7735c46bd..df9935a7d 100644 --- a/src/libsync/propagateremotemove.cpp +++ b/src/libsync/propagateremotemove.cpp @@ -85,31 +85,6 @@ void PropagateRemoteMove::start() if (origin == _item->_renameTarget) { // The parent has been renamed already so there is nothing more to do. - - if (!_item->_encryptedFileName.isEmpty()) { - const auto path = _item->_file; - const auto slashPosition = path.lastIndexOf('/'); - const auto parentPath = slashPosition >= 0 ? path.left(slashPosition) : QString(); - - SyncJournalFileRecord parentRec; - bool ok = propagator()->_journal->getFileRecord(parentPath, &parentRec); - if (!ok) { - done(SyncFileItem::NormalError); - return; - } - - // We should be encrypted as well since our parent is - const auto remoteParentPath = parentRec._e2eMangledName.isEmpty() ? parentPath : parentRec._e2eMangledName; - - const auto lastSlashPosition = _item->_encryptedFileName.lastIndexOf('/'); - const auto encryptedName = slashPosition >= 0 ? _item->_encryptedFileName.mid(lastSlashPosition + 1) : QString(); - - if (!encryptedName.isEmpty()) { - _item->_encryptedFileName = remoteParentPath + "/" + encryptedName; - } - - } - finalize(); return; } diff --git a/src/libsync/propagateupload.cpp b/src/libsync/propagateupload.cpp index 7f4cf6d86..ddab3774b 100644 --- a/src/libsync/propagateupload.cpp +++ b/src/libsync/propagateupload.cpp @@ -237,8 +237,10 @@ void PropagateUploadFileCommon::start() _uploadEncryptedHelper = new PropagateUploadEncrypted(propagator(), remoteParentPath, _item, this); connect(_uploadEncryptedHelper, &PropagateUploadEncrypted::finalized, this, &PropagateUploadFileCommon::setupEncryptedFile); - connect(_uploadEncryptedHelper, &PropagateUploadEncrypted::error, - []{ qCDebug(lcPropagateUpload) << "Error setting up encryption."; }); + connect(_uploadEncryptedHelper, &PropagateUploadEncrypted::error, [this]{ + qCDebug(lcPropagateUpload) << "Error setting up encryption."; + done(SyncFileItem::FatalError, tr("Failed to upload encrypted file.")); + }); _uploadEncryptedHelper->start(); } @@ -384,11 +386,7 @@ void PropagateUploadFileCommon::slotStartUpload(const QByteArray &transmissionCh const QString originalFilePath = propagator()->fullLocalPath(_item->_file); if (!FileSystem::fileExists(fullFilePath)) { - if (_uploadingEncrypted) { - _uploadEncryptedHelper->unlockFolder(); - } - done(SyncFileItem::SoftError, tr("File Removed (start upload) %1").arg(fullFilePath)); - return; + return slotOnErrorStartFolderUnlock(SyncFileItem::SoftError, tr("File Removed (start upload) %1").arg(fullFilePath)); } time_t prevModtime = _item->_modtime; // the _item value was set in PropagateUploadFile::start() // but a potential checksum calculation could have taken some time during which the file could @@ -397,12 +395,8 @@ void PropagateUploadFileCommon::slotStartUpload(const QByteArray &transmissionCh _item->_modtime = FileSystem::getModTime(originalFilePath); if (prevModtime != _item->_modtime) { propagator()->_anotherSyncNeeded = true; - if (_uploadingEncrypted) { - _uploadEncryptedHelper->unlockFolder(); - } qDebug() << "prevModtime" << prevModtime << "Curr" << _item->_modtime; - done(SyncFileItem::SoftError, tr("Local file changed during syncing. It will be resumed.")); - return; + return slotOnErrorStartFolderUnlock(SyncFileItem::SoftError, tr("Local file changed during syncing. It will be resumed.")); } _fileToUpload._size = FileSystem::getSize(fullFilePath); @@ -413,16 +407,33 @@ void PropagateUploadFileCommon::slotStartUpload(const QByteArray &transmissionCh // or not yet fully copied to the destination. if (fileIsStillChanging(*_item)) { propagator()->_anotherSyncNeeded = true; - if (_uploadingEncrypted) { - _uploadEncryptedHelper->unlockFolder(); - } - done(SyncFileItem::SoftError, tr("Local file changed during sync.")); - return; + return slotOnErrorStartFolderUnlock(SyncFileItem::SoftError, tr("Local file changed during sync.")); } doStartUpload(); } +void PropagateUploadFileCommon::slotFolderUnlocked(const QByteArray &folderId, int httpReturnCode) +{ + qDebug() << "Failed to unlock encrypted folder" << folderId; + if (_status.first == SyncFileItem::NoStatus && httpReturnCode != 200) { + done(SyncFileItem::FatalError, tr("Failed to unlock encrypted folder.")); + } else { + done(_status.first, _status.second); + } +} + +void PropagateUploadFileCommon::slotOnErrorStartFolderUnlock(SyncFileItem::Status status, const QString &errorString) +{ + if (_uploadingEncrypted) { + _status = { status, errorString }; + connect(_uploadEncryptedHelper, &PropagateUploadEncrypted::folderUnlocked, this, &PropagateUploadFileCommon::slotFolderUnlocked); + _uploadEncryptedHelper->unlockFolder(); + } else { + done(status, errorString); + } +} + UploadDevice::UploadDevice(const QString &fileName, qint64 start, qint64 size, BandwidthManager *bwm) : _file(fileName) , _start(start) @@ -775,9 +786,12 @@ void PropagateUploadFileCommon::finalize() propagator()->_journal->commit("upload file start"); if (_uploadingEncrypted) { - _uploadEncryptedHelper->unlockFolder(); + _status = { SyncFileItem::Success, QString() }; + connect(_uploadEncryptedHelper, &PropagateUploadEncrypted::folderUnlocked, this, &PropagateUploadFileCommon::slotFolderUnlocked); + _uploadEncryptedHelper->unlockFolder(); + } else { + done(SyncFileItem::Success); } - done(SyncFileItem::Success); } void PropagateUploadFileCommon::abortNetworkJobs( diff --git a/src/libsync/propagateupload.h b/src/libsync/propagateupload.h index 38a50b7d4..43b1f51c3 100644 --- a/src/libsync/propagateupload.h +++ b/src/libsync/propagateupload.h @@ -257,6 +257,10 @@ private slots: void slotComputeTransmissionChecksum(const QByteArray &contentChecksumType, const QByteArray &contentChecksum); // transmission checksum computed, prepare the upload void slotStartUpload(const QByteArray &transmissionChecksumType, const QByteArray &transmissionChecksum); + // invoked when encrypted folder lock has been released + void slotFolderUnlocked(const QByteArray &folderId, int httpReturnCode); + // invoked on internal error to unlock a folder and faile + void slotOnErrorStartFolderUnlock(SyncFileItem::Status status, const QString &errorString); public: virtual void doStartUpload() = 0; @@ -310,6 +314,7 @@ protected: private: PropagateUploadEncrypted *_uploadEncryptedHelper; bool _uploadingEncrypted; + QPair _status = { SyncFileItem::NoStatus, QString() }; }; /** diff --git a/src/libsync/propagateuploadencrypted.cpp b/src/libsync/propagateuploadencrypted.cpp index d02e7bc37..487083fc8 100644 --- a/src/libsync/propagateuploadencrypted.cpp +++ b/src/libsync/propagateuploadencrypted.cpp @@ -178,7 +178,7 @@ void PropagateUploadEncrypted::slotFolderEncryptedMetadataReceived(const QJsonDo if (!encryptionResult) { qCDebug(lcPropagateUploadEncrypted()) << "There was an error encrypting the file, aborting upload."; - unlockFolder(); + unlockFolder(true); return; } @@ -229,7 +229,7 @@ void PropagateUploadEncrypted::slotUpdateMetadataError(const QByteArray& fileId, { qCDebug(lcPropagateUploadEncrypted) << "Update metadata error for folder" << fileId << "with error" << httpErrorResponse; qCDebug(lcPropagateUploadEncrypted()) << "Unlocking the folder."; - unlockFolder(); + unlockFolder(true); } void PropagateUploadEncrypted::slotFolderLockedError(const QByteArray& fileId, int httpErrorCode) @@ -262,14 +262,51 @@ void PropagateUploadEncrypted::slotFolderEncryptedIdError(QNetworkReply *r) qCDebug(lcPropagateUploadEncrypted) << "Error retrieving the Id of the encrypted folder."; } -void PropagateUploadEncrypted::unlockFolder() +void PropagateUploadEncrypted::unlockFolder(bool uploadFailed) { + { + QMutexLocker locker(&_isUnlockRunningMutex); + + ASSERT(!_isUnlockRunning); + + if (_isUnlockRunning) { + qWarning() << "Double-call to unlockFolder."; + return; + } + + _isUnlockRunning = true; + } + qDebug() << "Calling Unlock"; auto *unlockJob = new UnlockEncryptFolderApiJob(_propagator->account(), _folderId, _folderToken, this); - connect(unlockJob, &UnlockEncryptFolderApiJob::success, []{ qDebug() << "Successfully Unlocked"; }); - connect(unlockJob, &UnlockEncryptFolderApiJob::error, []{ qDebug() << "Unlock Error"; }); + connect(unlockJob, &UnlockEncryptFolderApiJob::success, [this, uploadFailed](const QByteArray &folderId) { + qDebug() << "Successfully Unlocked"; + _folderToken = ""; + _folderId = ""; + + emit folderUnlocked(folderId, 200); + + if (uploadFailed) { + emit error(); + } + + QMutexLocker locker(&_isUnlockRunningMutex); + _isUnlockRunning = false; + }); + connect(unlockJob, &UnlockEncryptFolderApiJob::error, [this, uploadFailed](const QByteArray &folderId, int httpStatus) { + qDebug() << "Unlock Error"; + + emit folderUnlocked(folderId, httpStatus); + + if (uploadFailed) { + emit error(); + } + + QMutexLocker locker(&_isUnlockRunningMutex); + _isUnlockRunning = false; + }); unlockJob->start(); } diff --git a/src/libsync/propagateuploadencrypted.h b/src/libsync/propagateuploadencrypted.h index e281a17b1..f1225a726 100644 --- a/src/libsync/propagateuploadencrypted.h +++ b/src/libsync/propagateuploadencrypted.h @@ -36,7 +36,7 @@ public: void start(); /* unlocks the current folder that holds this file */ - void unlockFolder(); + void unlockFolder(bool uploadFailed = false); // Used by propagateupload QByteArray _folderToken; QByteArray _folderId; @@ -56,6 +56,7 @@ signals: // Emmited after the file is encrypted and everythign is setup. void finalized(const QString& path, const QString& filename, quint64 size); void error(); + void folderUnlocked(const QByteArray &folderId, int httpStatus); private: OwncloudPropagator *_propagator; @@ -70,6 +71,9 @@ private: FolderMetadata *_metadata; EncryptedFile _encryptedFile; QString _completeFileName; + + bool _isUnlockRunning = false; // protection against multiple calls to unlock the same folder + QMutex _isUnlockRunningMutex; // corresponding mutex for _isUnlockRunning };