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.

Sign up for GitHub

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

Misc compiler warnings on Debian sid #2789

Closed
wants to merge 3 commits into from
Closed

Misc compiler warnings on Debian sid #2789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
32cff0c
Check system() return values in testsuite
cgzones Jan 3, 2021
ec49428
Do not call exit in library code
cgzones Jan 3, 2021
f9dc0b6
Add missing fallthrough attributes
cgzones Nov 28, 2024
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
20 changes: 10 additions & 10 deletions src/common/c_jhash.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,17 +151,17 @@ static inline uint32_t c_jhash(const uint8_t *k, uint32_t length, uint32_t initv
c += length;
/* all the case statements fall through */
switch(len) {
case 11: c+=((uint32_t)k[10]<<24);
case 10: c+=((uint32_t)k[9]<<16);
case 9 : c+=((uint32_t)k[8]<<8);
case 11: c+=((uint32_t)k[10]<<24); Q_FALLTHROUGH();
case 10: c+=((uint32_t)k[9]<<16); Q_FALLTHROUGH();
case 9 : c+=((uint32_t)k[8]<<8); Q_FALLTHROUGH();
/* the first byte of c is reserved for the length */
case 8 : b+=((uint32_t)k[7]<<24);
case 7 : b+=((uint32_t)k[6]<<16);
case 6 : b+=((uint32_t)k[5]<<8);
case 5 : b+=k[4];
case 4 : a+=((uint32_t)k[3]<<24);
case 3 : a+=((uint32_t)k[2]<<16);
case 2 : a+=((uint32_t)k[1]<<8);
case 8 : b+=((uint32_t)k[7]<<24); Q_FALLTHROUGH();
case 7 : b+=((uint32_t)k[6]<<16); Q_FALLTHROUGH();
case 6 : b+=((uint32_t)k[5]<<8); Q_FALLTHROUGH();
case 5 : b+=k[4]; Q_FALLTHROUGH();
case 4 : a+=((uint32_t)k[3]<<24); Q_FALLTHROUGH();
case 3 : a+=((uint32_t)k[2]<<16); Q_FALLTHROUGH();
case 2 : a+=((uint32_t)k[1]<<8); Q_FALLTHROUGH();
case 1 : a+=k[0];
/* case 0: nothing left to add */
}
Expand Down
18 changes: 11 additions & 7 deletions src/libsync/clientsideencryption.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,41 +703,41 @@ QByteArray encryptStringAsymmetric(EVP_PKEY *publicKey, const QByteArray& data)
auto ctx = PKeyCtx::forKey(publicKey, ENGINE_get_default_RSA());
if (!ctx) {
qCInfo(lcCse()) << "Could not initialize the pkey context.";
exit(1);
Copy link
Member

Choose a reason for hiding this comment

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

Callers are not going to handle that change in behavior, so likely needs some error handling up the call stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly there seems to be no actual error handling in src/libsync/clientsideencryption.cpp.
So for now the key is just skipped in FolderMetadata::encryptedMetadata().

Copy link
Member

Choose a reason for hiding this comment

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

Yes, definitely. This needs being addressed otherwise it's too much of a behavior change, this will start trickling up and not being blocked later on while right now it has no risk of trickling up and corrupting things (even though it does so in a super aggressive and less than ideal way indeed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the error handling in the callers.

return {};
}

if (EVP_PKEY_encrypt_init(ctx) != 1) {
qCInfo(lcCse()) << "Error initilaizing the encryption.";
exit(1);
return {};
}

if (EVP_PKEY_CTX_set_rsa_padding(ctx, RSA_PKCS1_OAEP_PADDING) <= 0) {
qCInfo(lcCse()) << "Error setting the encryption padding.";
exit(1);
return {};
}

if (EVP_PKEY_CTX_set_rsa_oaep_md(ctx, EVP_sha256()) <= 0) {
qCInfo(lcCse()) << "Error setting OAEP SHA 256";
exit(1);
return {};
}

if (EVP_PKEY_CTX_set_rsa_mgf1_md(ctx, EVP_sha256()) <= 0) {
qCInfo(lcCse()) << "Error setting MGF1 padding";
exit(1);
return {};
}

size_t outLen = 0;
if (EVP_PKEY_encrypt(ctx, nullptr, &outLen, (unsigned char *)data.constData(), data.size()) != 1) {
qCInfo(lcCse()) << "Error retrieving the size of the encrypted data";
exit(1);
return {};
} else {
qCInfo(lcCse()) << "Encryption Length:" << outLen;
}

QByteArray out(static_cast<int>(outLen), '\0');
if (EVP_PKEY_encrypt(ctx, unsignedData(out), &outLen, (unsigned char *)data.constData(), data.size()) != 1) {
qCInfo(lcCse()) << "Could not encrypt key." << err;
exit(1);
return {};
}

qCInfo(lcCse()) << out.toBase64();
Expand Down Expand Up @@ -816,6 +816,10 @@ bool ClientSideEncryption::checkPublicKeyValidity(const AccountPtr &account) con
auto publicKey = PKey::readPublicKey(publicKeyBio);

auto encryptedData = EncryptionHelper::encryptStringAsymmetric(publicKey, data.toBase64());
if (encryptedData.isEmpty()) {
qCInfo(lcCse()) << "encryption failed";
return false;
}

Bio privateKeyBio;
QByteArray privateKeyPem = account->e2e()->_privateKey;
Expand Down
4 changes: 3 additions & 1 deletion src/libsync/clientsideencryptionjobs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,9 @@ bool LockEncryptFolderApiJob::finished()

if (!_publicKey.isNull()) {
const auto folderTokenEncrypted = EncryptionHelper::encryptStringAsymmetric(_publicKey, token);
_journalDb->setE2EeLockedFolder(_fileId, folderTokenEncrypted);
if (!folderTokenEncrypted.isEmpty()) {
_journalDb->setE2EeLockedFolder(_fileId, folderTokenEncrypted);
}
}

//TODO: Parse the token and submit.
Expand Down
13 changes: 12 additions & 1 deletion src/libsync/foldermetadata.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,6 +704,11 @@ QByteArray FolderMetadata::encryptedMetadataLegacy()
const auto version = _account->capabilities().clientSideEncryptionVersion();
// multiple toBase64() just to keep with the old (wrong way)
const auto encryptedMetadataKey = encryptDataWithPublicKey(metadataKeyForEncryption().toBase64().toBase64(), _account->e2e()->_publicKey).toBase64();
if (encryptedMetadataKey.isEmpty()) {
qCDebug(lcCseMetadata) << "Metadata generation failed! Encryption failed!";
_account->reportClientStatus(OCC::ClientStatusReportingStatus::E2EeError_GeneralError);
return {};
}
const QJsonObject metadata{
{versionKey, version},
{metadataKeyKey, QJsonValue::fromVariant(encryptedMetadataKey)},
Expand Down Expand Up @@ -1049,10 +1054,16 @@ bool FolderMetadata::addUser(const QString &userId, const QSslCertificate &certi
}

createNewMetadataKeyForEncryption();
const auto encryptedKey = encryptDataWithPublicKey(metadataKeyForEncryption(), certificatePublicKey);
if (encryptedKey.isEmpty()) {
qCWarning(lcCseMetadata()) << "Could not add a folder user. Encryption failure.";
return false;
}

UserWithFolderAccess newFolderUser;
newFolderUser.userId = userId;
newFolderUser.certificatePem = certificate.toPem();
newFolderUser.encryptedMetadataKey = encryptDataWithPublicKey(metadataKeyForEncryption(), certificatePublicKey);
newFolderUser.encryptedMetadataKey = encryptedKey;
_folderUsers[userId] = newFolderUser;
updateUsersEncryptedMetadataKey();

Expand Down
20 changes: 14 additions & 6 deletions test/testfolderwatcher.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,20 @@
*
*/

#include <QtGlobal>
#include <QtTest>

#include "folderwatcher.h"
#include "common/utility.h"
#include "logger.h"

#define CHECKED_SYSTEM(cmd_) \
do { \
int result_ = system(cmd_); \
Q_ASSERT(WIFEXITED(result_)); \
Q_ASSERT(WEXITSTATUS(result_) == 0); \
} while(0)

void touch(const QString &file)
{
#ifdef Q_OS_WIN
Expand All @@ -19,7 +27,7 @@ void touch(const QString &file)
QString cmd;
cmd = QString("touch %1").arg(file);
qDebug() << "Command: " << cmd;
system(cmd.toLocal8Bit());
CHECKED_SYSTEM(cmd.toLocal8Bit());
#endif
}

Expand All @@ -31,7 +39,7 @@ void mkdir(const QString &file)
#else
QString cmd = QString("mkdir %1").arg(file);
qDebug() << "Command: " << cmd;
system(cmd.toLocal8Bit());
CHECKED_SYSTEM(cmd.toLocal8Bit());
#endif
}

Expand All @@ -43,7 +51,7 @@ void rmdir(const QString &file)
#else
QString cmd = QString("rmdir %1").arg(file);
qDebug() << "Command: " << cmd;
system(cmd.toLocal8Bit());
CHECKED_SYSTEM(cmd.toLocal8Bit());
#endif
}

Expand All @@ -54,7 +62,7 @@ void rm(const QString &file)
#else
QString cmd = QString("rm %1").arg(file);
qDebug() << "Command: " << cmd;
system(cmd.toLocal8Bit());
CHECKED_SYSTEM(cmd.toLocal8Bit());
#endif
}

Expand All @@ -65,7 +73,7 @@ void mv(const QString &file1, const QString &file2)
#else
QString cmd = QString("mv %1 %2").arg(file1, file2);
qDebug() << "Command: " << cmd;
system(cmd.toLocal8Bit());
CHECKED_SYSTEM(cmd.toLocal8Bit());
#endif
}

Expand Down Expand Up @@ -160,7 +168,7 @@ private slots:
QString cmd;
cmd = QString("echo \"xyz\" > \"%1\"").arg(file);
qDebug() << "Command: " << cmd;
system(cmd.toLocal8Bit());
CHECKED_SYSTEM(cmd.toLocal8Bit());

QVERIFY(waitForPathChanged(file));
}
Expand Down
10 changes: 9 additions & 1 deletion test/testinotifywatcher.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,19 @@
* any purpose.
* */

#include <QtGlobal>
#include <QtTest>

#include "folderwatcher_linux.h"
#include "common/utility.h"

#define CHECKED_SYSTEM(cmd_) \
do { \
int result_ = system(cmd_); \
Q_ASSERT(WIFEXITED(result_)); \
Q_ASSERT(WEXITSTATUS(result_) == 0); \
} while(0)

using namespace OCC;

class TestInotifyWatcher: public FolderWatcherPrivate
Expand Down Expand Up @@ -62,7 +70,7 @@ private slots:

void cleanupTestCase() {
if( _root.startsWith(QDir::tempPath() )) {
system( QString("rm -rf %1").arg(_root).toLocal8Bit() );
CHECKED_SYSTEM( QString("rm -rf %1").arg(_root).toLocal8Bit() );
}
}
};
Expand Down