Skip to content

SonarQube Code Coverage (Uncovered Conditions, Second Pass) #146

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

Merged
Jean-Lessa merged 7 commits into from
Jan 15, 2025
Merged

SonarQube Code Coverage (Uncovered Conditions, Second Pass) #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9abf98f
Add tests for ReentrancyGuard
Jean-Lessa Jan 6, 2025
b7295e9
Force gcovr to exclude throw branches for better coverage
Jean-Lessa Jan 6, 2025
8e0406e
Revise condition coverage for some units (everywhere)
Jean-Lessa Jan 8, 2025
4e5a550
Revise condition coverage for some more units (everywhere)
Jean-Lessa Jan 11, 2025
d5b7e4a
Fix conflict with development branch in ERC20 tests
Jean-Lessa Jan 11, 2025
e64579f
Fix some gas tests failing due to not being aware of a07c817
Jean-Lessa Jan 11, 2025
c59eb6d
Revise condition coverage for yet some more units (contract + utils)
Jean-Lessa Jan 12, 2025
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
2 changes: 1 addition & 1 deletion .github/workflows/c-cpp.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ jobs:
run: ./scripts/auto.sh -s bdk exec 'rm coverage.xml || true'
- name: Collect coverage into one XML report
run: ./scripts/auto.sh -s bdk exec \
'gcovr -d --gcov-ignore-parse-errors --sonarqube -o coverage.xml'
'gcovr -d --gcov-ignore-parse-errors --exclude-throw-branches --sonarqube -o coverage.xml'
- name: Run SonarQube Scanner
run: ./scripts/auto.sh -s bdk exec \
'env SONAR_TOKEN=${{ env.SONAR_TOKEN }}
Expand Down
7 changes: 4 additions & 3 deletions src/bins/bdkd-discovery/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ See the LICENSE.txt file in the project root for more information.

#include <csignal>
#include <condition_variable>
#include <iostream>

#include "src/net/p2p/managerdiscovery.h"

#include "net/p2p/managerdiscovery.h"
#include "utils/options.h"
#include "iostream"
#include "src/utils/clargs.h"
#include "src/utils/options.h"

std::condition_variable cv;
std::mutex cv_m;
Expand Down
14 changes: 7 additions & 7 deletions src/contract/contract.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,19 +106,19 @@ class BaseContract : public ContractLocals, public Dumpable {
BaseContract(const Address &address, const DB& db) :
contractAddress_(address),
dbPrefix_([&]() {
Bytes prefix = DBPrefix::contracts;
prefix.reserve(prefix.size() + address.size());
prefix.insert(prefix.end(), address.cbegin(), address.cend());
return prefix;
Bytes prefix = DBPrefix::contracts;
prefix.reserve(prefix.size() + address.size());
prefix.insert(prefix.end(), address.cbegin(), address.cend());
return prefix;
}()),
contractName_([&]() {
return StrConv::bytesToString(db.get(std::string("contractName_"), dbPrefix_));
return StrConv::bytesToString(db.get(std::string("contractName_"), dbPrefix_));
}()),
contractCreator_([&]() {
return Address(db.get(std::string("contractCreator_"), dbPrefix_));
return Address(db.get(std::string("contractCreator_"), dbPrefix_));
}()),
contractChainId_([&]() {
return UintConv::bytesToUint64(db.get(std::string("contractChainId_"), dbPrefix_));
return UintConv::bytesToUint64(db.get(std::string("contractChainId_"), dbPrefix_));
}())
{}

Expand Down
5 changes: 2 additions & 3 deletions src/contract/templates/erc20wrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,9 @@ class ERC20Wrapper : public DynamicContract {
void registerContractFunctions() override;

public:

/**
* ConstructorArguments is a tuple of the contract constructor arguments in the order they appear in the constructor.
*/
* ConstructorArguments is a tuple of the contract constructor arguments in the order they appear in the constructor.
*/
using ConstructorArguments = std::tuple<>;

/**
Expand Down
14 changes: 3 additions & 11 deletions src/contract/templates/erc721.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ void ERC721::mint_(const Address& to, const uint256_t& tokenId) {
}

void ERC721::burn_(const uint256_t& tokenId) {
Address prevOwner = this->update_(Address(), tokenId, Address());
Address prevOwner = this->update_(Address(), tokenId, this->getCaller());
if (prevOwner == Address()) {
throw DynamicException("ERC721::burn_: inexistent token");
}
Expand All @@ -174,7 +174,7 @@ void ERC721::transfer_(const Address& from, const Address& to, const uint256_t&
throw DynamicException("ERC721::transfer_: transfer to the zero address");
}

Address prevOwner = this->update_(to, tokenId, Address());
Address prevOwner = this->update_(to, tokenId, this->getCaller());
if (prevOwner == Address()) {
throw DynamicException("ERC721::transfer_: inexistent token");
} else if (prevOwner != from) {
Expand Down Expand Up @@ -257,15 +257,7 @@ bool ERC721::isApprovedForAll(const Address& owner, const Address& operatorAddre
}

void ERC721::transferFrom(const Address& from, const Address& to, const uint256_t& tokenId) {
if (to == Address()) {
throw DynamicException("ERC721::transferFrom: transfer to the zero address");
}
Address prevOwner = this->update_(to, tokenId, this->getCaller());
if (prevOwner == Address()) {
throw DynamicException("ERC721::transferFrom: inexistent token");
} else if (prevOwner != from) {
throw DynamicException("ERC721::transferFrom: incorrect owner");
}
this->transfer_(from, to, tokenId);
}

DBBatch ERC721::dump() const {
Expand Down
2 changes: 1 addition & 1 deletion src/contract/templates/erc721test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ void ERC721Test::mint(const Address& to) {
}

void ERC721Test::burn(const uint256_t& tokenId) {
this->update_(Address(), tokenId, this->getCaller());
this->burn_(tokenId);
--totalSupply_;
}

20 changes: 4 additions & 16 deletions src/contract/templates/erc721test.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ class ERC721Test : public ERC721 {
* ConstructorArguments is a tuple of the contract constructor arguments in
* the order they appear in the constructor.
*/
using ConstructorArguments =
std::tuple<const std::string &, const std::string &, const uint64_t&>;
using ConstructorArguments = std::tuple<const std::string &, const std::string &, const uint64_t&>;

/**
* Constructor for loading contract from DB.
Expand Down Expand Up @@ -77,19 +76,13 @@ class ERC721Test : public ERC721 {
void burn(const uint256_t& tokenId);

/// Getter for the tokenIdCounter_
uint64_t tokenIdCounter() const {
return tokenIdCounter_.get();
}
uint64_t tokenIdCounter() const { return tokenIdCounter_.get(); }

/// Getter for the maxTokens_
uint64_t maxTokens() const {
return maxTokens_.get();
}
uint64_t maxTokens() const { return maxTokens_.get(); }

/// Getter for the totalSupply_
uint64_t totalSupply() const {
return totalSupply_.get();
}
uint64_t totalSupply() const { return totalSupply_.get(); }

/// Register contract class via ContractReflectionInterface.
static void registerContract() {
Expand All @@ -111,9 +104,4 @@ class ERC721Test : public ERC721 {
DBBatch dump() const override;
};






#endif // ERC721_TEST
15 changes: 6 additions & 9 deletions src/contract/templates/ownable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,17 +61,15 @@ void Ownable::checkOwner_() const {
}

void Ownable::transferOwnership_(const Address& newOwner) {
Address prevOwner = this->owner_.get();
this->owner_ = newOwner;
this->ownershipTransferred(prevOwner, newOwner);
}


void Ownable::onlyOwner() const {
this->checkOwner_();
}
void Ownable::onlyOwner() const { this->checkOwner_(); }

Address Ownable::owner() const {
return this->owner_.get();
}
Address Ownable::owner() const { return this->owner_.get(); }

void Ownable::renounceOwnership() {
this->onlyOwner();
Expand All @@ -80,8 +78,7 @@ void Ownable::renounceOwnership() {

void Ownable::transferOwnership(const Address& newOwner) {
this->onlyOwner();
if (newOwner == Address()) {
throw DynamicException("Ownable: new owner is the zero address");
}
if (newOwner == Address()) throw DynamicException("Ownable: new owner is the zero address");
this->transferOwnership_(newOwner);
}

2 changes: 1 addition & 1 deletion src/contract/templates/pebble.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ std::string Pebble::rarityToString(const Rarity& rarity) const {
return ret;
}

void Pebble::onlyAuthorizer() const {
void Pebble::onlyAuthorizer() {
if (this->getCaller() != this->authorizer_) {
throw DynamicException("Pebble: caller is not the authorizer");
}
Expand Down
2 changes: 1 addition & 1 deletion src/contract/templates/pebble.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ class Pebble : public virtual ERC721URIStorage, public virtual Ownable {
* Check if contract caller is the authorizer.
* @throw DynamicException if caller is not the authorizer.
*/
void onlyAuthorizer() const;
void onlyAuthorizer();

/**
* Change the authorizer address.
Expand Down
6 changes: 5 additions & 1 deletion src/contract/templates/simplecontract.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ void SimpleContract::setNumber(const uint256_t& argNumber) {
}

void SimpleContract::setNumbers(const std::vector<uint256_t>& argNumber) {
if (this->getCaller() != this->getContractCreator()) {
throw DynamicException("Only contract creator can call this function.");
}
this->number_ = 0;
for (const auto& number : argNumber) this->number_ += number;
this->numberChanged(this->number_.get());
Expand Down Expand Up @@ -222,10 +225,11 @@ void SimpleContract::registerContractFunctions() {
}

DBBatch SimpleContract::dump() const {
DBBatch dbBatch;
DBBatch dbBatch = BaseContract::dump();
dbBatch.push_back(StrConv::stringToBytes("name_"), StrConv::stringToBytes(this->name_.get()), this->getDBPrefix());
dbBatch.push_back(StrConv::stringToBytes("number_"), UintConv::uint256ToBytes(this->number_.get()), this->getDBPrefix());
dbBatch.push_back(StrConv::stringToBytes("tuple_name"), StrConv::stringToBytes(get<0>(this->tuple_)), this->getDBPrefix());
dbBatch.push_back(StrConv::stringToBytes("tuple_number"), UintConv::uint256ToBytes(get<1>(this->tuple_)), this->getDBPrefix());
return dbBatch;
}

2 changes: 2 additions & 0 deletions src/contract/variables/reentrancyguard.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ See the LICENSE.txt file in the project root for more information.
#ifndef REENTRANCY_GUARD_H
#define REENTRANCY_GUARD_H

#include "../../utils/dynamicexception.h"

/**
* RAII object used to prevent reentrancy attacks, similar to std::unique_lock or std::shared_lock.
* It is meant to be used within the first line of the function you want to protect against reentrancy attacks.
Expand Down
7 changes: 4 additions & 3 deletions src/net/p2p/encoding.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,9 +556,10 @@ namespace P2P {
*/
class Message {
private:
/// The internal message data to be read/written, stored as bytes.
/// Sessions has directly access to it
/// As it can use the vector for its buffer.
/**
* The internal message data to be read/written, stored as bytes.
* Sessions has directly access to it as it can use the vector for its buffer.
*/
Bytes rawMessage_;

/// Assignment operator.
Expand Down
2 changes: 2 additions & 0 deletions src/utils/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
set(UTILS_HEADERS
${CMAKE_SOURCE_DIR}/src/utils/clargs.h
${CMAKE_SOURCE_DIR}/src/utils/db.h
${CMAKE_SOURCE_DIR}/src/utils/utils.h
${CMAKE_SOURCE_DIR}/src/utils/strings.h
Expand All @@ -21,6 +22,7 @@ set(UTILS_HEADERS
)

set(UTILS_SOURCES
${CMAKE_SOURCE_DIR}/src/utils/clargs.cpp
${CMAKE_SOURCE_DIR}/src/utils/db.cpp
${CMAKE_SOURCE_DIR}/src/utils/utils.cpp
${CMAKE_SOURCE_DIR}/src/utils/strings.cpp
Expand Down
127 changes: 127 additions & 0 deletions src/utils/clargs.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
/*
Copyright (c) [2023-2024] [AppLayer Developers]

This software is distributed under the MIT License.
See the LICENSE.txt file in the project root for more information.
*/

#include "clargs.h"

ProcessOptions parseCommandLineArgs(int argc, char* argv[], [[maybe_unused]] BDKTool tool) {
ProcessOptions opt;
try {
boost::program_options::options_description desc("Allowed options");
desc.add_options()
("help,h",
"Print help message and exit")
("loglevel,l", boost::program_options::value<std::string>(),
"Set the log level ([X]TRACE, [T]RACE, [D]EBUG, [I]NFO, [W]ARNING, [E]RROR, [F]ATAL, [N]ONE)")
("loglinelimit", boost::program_options::value<int>(),
"Set the log line limit (# of lines per file); 0 = no limit")
("logfilelimit", boost::program_options::value<int>(),
"Set the log file limit (# of files); 0 = no limit")
("netthreads", boost::program_options::value<int>(),
"Set ManagerBase::netThreads_ (main IO thread count)")
;

boost::program_options::variables_map vm;
boost::program_options::store(boost::program_options::parse_command_line(argc, argv, desc), vm);
boost::program_options::notify(vm);

if (vm.count("help")) {
std::cout << desc << "\n";
exit(0);
}

if (vm.count("loglevel")) {
opt.logLevel = vm["loglevel"].as<std::string>();
}

if (vm.count("loglinelimit")) {
opt.logLineLimit = vm["loglinelimit"].as<int>();
if (opt.logLineLimit < 0) {
std::cerr << "ERROR: --loglinelimit must be >= 0\n";
return {};
}
}

if (vm.count("logfilelimit")) {
opt.logFileLimit = vm["logfilelimit"].as<int>();
if (opt.logFileLimit < 0) {
std::cerr << "ERROR: --logfilelimit must be >= 0\n";
return {};
}
}

if (vm.count("netthreads")) {
opt.netThreads = vm["netthreads"].as<int>();
if (opt.netThreads < 1) {
std::cerr << "ERROR: --netthreads must be >= 1\n";
return {};
}
}
} catch (std::exception& e) {
std::cout << "ERROR: parseCommandLineArgs(): " << e.what() << "\n";
return {};
} catch (...) {
std::cout << "ERROR: parseCommandLineArgs(): Unknown exception\n";
return {};
}

opt.valid = true;
return opt;
}

bool applyProcessOptions(ProcessOptions& opt) {
if (!opt.valid) {
std::cout << "ERROR: Invalid command-line arguments" << std::endl;
return false;
}

std::transform(opt.logLevel.begin(), opt.logLevel.end(), opt.logLevel.begin(), ::toupper);

if (opt.logLevel == "X") { opt.logLevel = "XTRACE"; }
else if (opt.logLevel == "T") { opt.logLevel = "TRACE"; }
else if (opt.logLevel == "D") { opt.logLevel = "DEBUG"; }
else if (opt.logLevel == "I") { opt.logLevel = "INFO"; }
else if (opt.logLevel == "W") { opt.logLevel = "WARNING"; }
else if (opt.logLevel == "E") { opt.logLevel = "ERROR"; }
else if (opt.logLevel == "F") { opt.logLevel = "FATAL"; }
else if (opt.logLevel == "N") { opt.logLevel = "NONE"; }

if (opt.logLevel == "") { /* Do nothing */ }
else if (opt.logLevel == "XTRACE") { Logger::setLogLevel(LogType::XTRACE); }
else if (opt.logLevel == "TRACE") { Logger::setLogLevel(LogType::TRACE); }
else if (opt.logLevel == "DEBUG") { Logger::setLogLevel(LogType::DEBUG); }
else if (opt.logLevel == "INFO") { Logger::setLogLevel(LogType::INFO); }
else if (opt.logLevel == "WARNING") { Logger::setLogLevel(LogType::WARNING); }
else if (opt.logLevel == "ERROR") { Logger::setLogLevel(LogType::ERROR); }
else if (opt.logLevel == "FATAL") { Logger::setLogLevel(LogType::FATAL); }
else if (opt.logLevel == "NONE") { Logger::setLogLevel(LogType::NONE); }
else {
std::cout << "ERROR: Invalid log level requested: " << opt.logLevel << std::endl;
return false;
}

if (opt.logLevel != "") {
std::cout << "Log level set to " << opt.logLevel << std::endl;
}

if (opt.logLineLimit >= 0) {
Logger::setLogLineLimit(opt.logLineLimit);
std::cout << "Log line limit set to " << opt.logLineLimit << std::endl;
}

if (opt.logFileLimit >= 0) {
Logger::setLogFileLimit(opt.logFileLimit);
std::cout << "Log file limit set to " << opt.logFileLimit << std::endl;
}

if (opt.netThreads >= 0) { // negative number signals unset; 0 is invalid, but somehow it was set to that value
P2P::ManagerBase::setNetThreads(opt.netThreads);
std::cout << "ManagerBase::netThreads_ set to " << opt.netThreads << std::endl;
}

return true;
}

Loading