From 54f149fc1858cac7f560b9a6140e5412e7f68acb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Blin?=
<sebastien.blin@savoirfairelinux.com>
Date: Mon, 10 Jul 2023 16:01:45 -0400
Subject: [PATCH] conversationrepository: protect operation
+ Avoid concurrent operations on same repository
+ Reset HEAD to always get a clean commit
GitLab: #931
Change-Id: If417c06934e2acab3382481718dc0bc46f40e3f9
---
src/jamidht/conversationrepository.cpp | 76 ++++++++++++++++---
src/jamidht/conversationrepository.h | 3 +
.../conversation/conversationcommon.cpp | 6 ++
3 files changed, 76 insertions(+), 9 deletions(-)
diff --git a/src/jamidht/conversationrepository.cpp b/src/jamidht/conversationrepository.cpp
index cbf51c237d..35b613f080 100644
--- a/src/jamidht/conversationrepository.cpp
+++ b/src/jamidht/conversationrepository.cpp
@@ -40,6 +40,10 @@ constexpr size_t MAX_FETCH_SIZE {256 * 1024 * 1024}; // 256Mb
namespace jami {
+#ifdef LIBJAMI_TESTABLE
+bool ConversationRepository::DISABLE_RESET = false;
+#endif
+
static const std::regex regex_display_name("<|>");
inline std::string_view
@@ -158,9 +162,11 @@ public:
bool add(const std::string& path);
void addUserDevice();
+ void resetHard();
// Verify that the device in the repository is still valid
bool validateDevice();
std::string commit(const std::string& msg, bool verifyDevice = true);
+ std::string commitMessage(const std::string& msg, bool verifyDevice = true);
ConversationMode mode() const;
// NOTE! GitDiff needs to be deteleted before repo
@@ -419,6 +425,8 @@ public:
return true;
}
+
+ std::mutex opMtx_; // Mutex for operations
};
/////////////////////////////////////////////////////////////////////////////////
@@ -2877,6 +2885,8 @@ ConversationRepository::id() const
std::string
ConversationRepository::addMember(const std::string& uri)
{
+ std::lock_guard lkOp(pimpl_->opMtx_);
+ pimpl_->resetHard();
auto account = pimpl_->account_.lock();
auto repo = pimpl_->repository();
if (!account or not repo)
@@ -2980,6 +2990,8 @@ ConversationRepository::amend(const std::string& id, const std::string& msg)
bool
ConversationRepository::fetch(const std::string& remoteDeviceId)
{
+ std::lock_guard lkOp(pimpl_->opMtx_);
+ pimpl_->resetHard();
// Fetch distant repository
git_remote* remote_ptr = nullptr;
git_fetch_options fetch_opts;
@@ -3098,11 +3110,39 @@ ConversationRepository::Impl::addUserDevice()
}
}
+void
+ConversationRepository::Impl::resetHard()
+{
+#ifdef LIBJAMI_TESTABLE
+ if (DISABLE_RESET)
+ return;
+#endif
+ auto repo = repository();
+ if (!repo)
+ return;
+ git_object *head_commit_obj = nullptr;
+ auto error = git_revparse_single(&head_commit_obj, repo.get(), "HEAD");
+ if (error < 0) {
+ JAMI_ERROR("Could not get HEAD commit");
+ return;
+ }
+ GitObject target {head_commit_obj, git_object_free};
+ git_reset(repo.get(), head_commit_obj, GIT_RESET_HARD, nullptr);
+}
+
std::string
ConversationRepository::commitMessage(const std::string& msg, bool verifyDevice)
{
- pimpl_->addUserDevice();
- return pimpl_->commit(msg, verifyDevice);
+ std::lock_guard lkOp(pimpl_->opMtx_);
+ pimpl_->resetHard();
+ return pimpl_->commitMessage(msg, verifyDevice);
+}
+
+std::string
+ConversationRepository::Impl::commitMessage(const std::string& msg, bool verifyDevice)
+{
+ addUserDevice();
+ return commit(msg, verifyDevice);
}
std::vector<std::string>
@@ -3145,6 +3185,8 @@ ConversationRepository::getCommit(const std::string& commitId, bool logIfNotFoun
std::pair<bool, std::string>
ConversationRepository::merge(const std::string& merge_id, bool force)
{
+ std::lock_guard lkOp(pimpl_->opMtx_);
+ pimpl_->resetHard();
// First, the repository must be in a clean state
auto repo = pimpl_->repository();
if (!repo) {
@@ -3153,8 +3195,12 @@ ConversationRepository::merge(const std::string& merge_id, bool force)
}
int state = git_repository_state(repo.get());
if (state != GIT_REPOSITORY_STATE_NONE) {
- JAMI_ERROR("Merge operation aborted: repository is in unexpected state %d", state);
- return {false, ""};
+ pimpl_->resetHard();
+ int state = git_repository_state(repo.get());
+ if (state != GIT_REPOSITORY_STATE_NONE) {
+ JAMI_ERROR("Merge operation aborted: repository is in unexpected state {}", state);
+ return {false, ""};
+ }
}
// Checkout main (to do a `git_merge branch`)
if (git_repository_set_head(repo.get(), "refs/heads/main") < 0) {
@@ -3295,6 +3341,8 @@ ConversationRepository::changedFiles(std::string_view diffStats)
std::string
ConversationRepository::join()
{
+ std::lock_guard lkOp(pimpl_->opMtx_);
+ pimpl_->resetHard();
// Check that not already member
auto repo = pimpl_->repository();
if (!repo)
@@ -3361,12 +3409,14 @@ ConversationRepository::join()
pimpl_->saveMembers();
}
- return commitMessage(Json::writeString(wbuilder, json));
+ return pimpl_->commitMessage(Json::writeString(wbuilder, json));
}
std::string
ConversationRepository::leave()
{
+ std::lock_guard lkOp(pimpl_->opMtx_);
+ pimpl_->resetHard();
// TODO simplify
auto account = pimpl_->account_.lock();
auto repo = pimpl_->repository();
@@ -3458,6 +3508,8 @@ ConversationRepository::mode() const
std::string
ConversationRepository::voteKick(const std::string& uri, const std::string& type)
{
+ std::lock_guard lkOp(pimpl_->opMtx_);
+ pimpl_->resetHard();
auto repo = pimpl_->repository();
auto account = pimpl_->account_.lock();
if (!account || !repo)
@@ -3502,12 +3554,14 @@ ConversationRepository::voteKick(const std::string& uri, const std::string& type
Json::StreamWriterBuilder wbuilder;
wbuilder["commentStyle"] = "None";
wbuilder["indentation"] = "";
- return commitMessage(Json::writeString(wbuilder, json));
+ return pimpl_->commitMessage(Json::writeString(wbuilder, json));
}
std::string
ConversationRepository::voteUnban(const std::string& uri, const std::string_view type)
{
+ std::lock_guard lkOp(pimpl_->opMtx_);
+ pimpl_->resetHard();
auto repo = pimpl_->repository();
auto account = pimpl_->account_.lock();
if (!account || !repo)
@@ -3542,7 +3596,7 @@ ConversationRepository::voteUnban(const std::string& uri, const std::string_view
Json::StreamWriterBuilder wbuilder;
wbuilder["commentStyle"] = "None";
wbuilder["indentation"] = "";
- return commitMessage(Json::writeString(wbuilder, json));
+ return pimpl_->commitMessage(Json::writeString(wbuilder, json));
}
bool
@@ -3648,6 +3702,8 @@ ConversationRepository::resolveVote(const std::string& uri,
const std::string_view type,
const std::string& voteType)
{
+ std::lock_guard lkOp(pimpl_->opMtx_);
+ pimpl_->resetHard();
// Count ratio admin/votes
auto nbAdmins = 0, nbVotes = 0;
// For each admin, check if voted
@@ -3693,7 +3749,7 @@ ConversationRepository::resolveVote(const std::string& uri,
Json::StreamWriterBuilder wbuilder;
wbuilder["commentStyle"] = "None";
wbuilder["indentation"] = "";
- return commitMessage(Json::writeString(wbuilder, json));
+ return pimpl_->commitMessage(Json::writeString(wbuilder, json));
}
// If vote nok
@@ -3809,6 +3865,8 @@ ConversationRepository::uriFromDevice(const std::string& deviceId) const
std::string
ConversationRepository::updateInfos(const std::map<std::string, std::string>& profile)
{
+ std::lock_guard lkOp(pimpl_->opMtx_);
+ pimpl_->resetHard();
auto account = pimpl_->account_.lock();
if (!account)
return {};
@@ -3890,7 +3948,7 @@ ConversationRepository::updateInfos(const std::map<std::string, std::string>& pr
wbuilder["commentStyle"] = "None";
wbuilder["indentation"] = "";
- return commitMessage(Json::writeString(wbuilder, json));
+ return pimpl_->commitMessage(Json::writeString(wbuilder, json));
}
std::map<std::string, std::string>
diff --git a/src/jamidht/conversationrepository.h b/src/jamidht/conversationrepository.h
index 41c0872d7a..7cb8f81672 100644
--- a/src/jamidht/conversationrepository.h
+++ b/src/jamidht/conversationrepository.h
@@ -141,6 +141,9 @@ using PostConditionCb
class LIBJAMI_TESTABLE ConversationRepository
{
public:
+ #ifdef LIBJAMI_TESTABLE
+ static bool DISABLE_RESET; // Some tests inject bad files so resetHard() will break the test
+ #endif
/**
* Creates a new repository, with initial files, where the first commit hash is the conversation id
* @param account The related account
diff --git a/test/unitTest/conversation/conversationcommon.cpp b/test/unitTest/conversation/conversationcommon.cpp
index ecfe4c85c9..31c7f5284f 100644
--- a/test/unitTest/conversation/conversationcommon.cpp
+++ b/test/unitTest/conversation/conversationcommon.cpp
@@ -51,6 +51,7 @@ addVote(std::shared_ptr<JamiAccount> account,
const std::string& votedUri,
const std::string& content)
{
+ ConversationRepository::DISABLE_RESET = true;
auto repoPath = fileutils::get_data_dir() / account->getAccountID()
/ "conversations" / convId;
auto voteDirectory = repoPath / "votes" / "members";
@@ -80,6 +81,7 @@ simulateRemoval(std::shared_ptr<JamiAccount> account,
const std::string& convId,
const std::string& votedUri)
{
+ ConversationRepository::DISABLE_RESET = true;
auto repoPath = fileutils::get_data_dir() / account->getAccountID()
/ "conversations" / convId;
auto memberFile = repoPath / "members" / (votedUri + ".crt");
@@ -125,6 +127,7 @@ addFile(std::shared_ptr<JamiAccount> account,
const std::string& relativePath,
const std::string& content)
{
+ ConversationRepository::DISABLE_RESET = true;
auto repoPath = fileutils::get_data_dir() / account->getAccountID()
/ "conversations" / convId;
// Add file
@@ -155,6 +158,7 @@ addFile(std::shared_ptr<JamiAccount> account,
void
addAll(std::shared_ptr<JamiAccount> account, const std::string& convId)
{
+ ConversationRepository::DISABLE_RESET = true;
auto repoPath = fileutils::get_data_dir() / account->getAccountID()
/ "conversations" / convId;
@@ -177,6 +181,7 @@ addAll(std::shared_ptr<JamiAccount> account, const std::string& convId)
void
commit(std::shared_ptr<JamiAccount> account, const std::string& convId, Json::Value& message)
{
+ ConversationRepository::DISABLE_RESET = true;
ConversationRepository cr(account->weak(), convId);
Json::StreamWriterBuilder wbuilder;
@@ -188,6 +193,7 @@ commit(std::shared_ptr<JamiAccount> account, const std::string& convId, Json::Va
std::string
commitInRepo(const std::string& path, std::shared_ptr<JamiAccount> account, const std::string& msg)
{
+ ConversationRepository::DISABLE_RESET = true;
auto deviceId = std::string(account->currentDeviceId());
auto name = account->getDisplayName();
if (name.empty())
--
GitLab