Skip to content

Commit

Permalink
Add clang-tidy check for overflow in constructing energy quantity fro…
Browse files Browse the repository at this point in the history
…m int (#71425)

* Add clang-tidy check for overflow in constructing energy quantity from int

* Avoid int overflow in calling unit::from_joule and unit::from_kilojoule

* Suppress Clang Static Analyzer temporarily
  • Loading branch information
BrettDong authored Feb 15, 2024
1 parent 5433b56 commit 4598b15
Show file tree
Hide file tree
Showing 21 changed files with 211 additions and 30 deletions.
3 changes: 2 additions & 1 deletion .clang-tidy
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
# We have lots of memory allocations in static variable declarations, and
# that's fine.
#
# * clang-analyzer-core.{DivideZero,NonNullParamChecker}
# * clang-analyzer-core.{DivideZero,NonNullParamChecker,UndefinedBinaryOperatorResult}
# * clang-analyzer-cplusplus.NewDelete
# They report too many false positives.
#
Expand Down Expand Up @@ -56,6 +56,7 @@ clang-diagnostic-*,\
-clang-analyzer-core.CallAndMessage,\
-clang-analyzer-core.DivideZero,\
-clang-analyzer-core.NonNullParamChecker,\
-clang-analyzer-core.UndefinedBinaryOperatorResult,\
-clang-analyzer-cplusplus.NewDelete,\
cppcoreguidelines-slicing,\
google-explicit-constructor,\
Expand Down
2 changes: 1 addition & 1 deletion src/activity_handlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3892,7 +3892,7 @@ void activity_handlers::spellcasting_finish( player_activity *act, Character *yo
you->mod_stamina( -cost );
break;
case magic_energy_type::bionic:
you->mod_power_level( -units::from_kilojoule( cost ) );
you->mod_power_level( -units::from_kilojoule( static_cast<std::int64_t>( cost ) ) );
break;
case magic_energy_type::hp:
blood_magic( you, cost );
Expand Down
4 changes: 2 additions & 2 deletions src/bionics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ void npc::check_or_use_weapon_cbm( const bionic_id &cbm_id )
int ammo_count = weap.ammo_remaining( this );
const units::energy ups_drain = weap.get_gun_ups_drain();
if( ups_drain > 0_kJ ) {
ammo_count = units::from_kilojoule( ammo_count ) / ups_drain;
ammo_count = units::from_kilojoule( static_cast<std::int64_t>( ammo_count ) ) / ups_drain;
}

// the weapon value from `weapon_value` may be different from `npc_attack_rating`
Expand Down Expand Up @@ -1193,7 +1193,7 @@ bool Character::activate_bionic( bionic &bio, bool eff_only, bool *close_bionics
}
ctr.charges = units::to_kilojoule( get_power_level() );
int power_use = invoke_item( &ctr );
mod_power_level( units::from_kilojoule( -power_use ) );
mod_power_level( units::from_kilojoule( static_cast<std::int64_t>( -power_use ) ) );
bio.powered = ctr.active;
} else {
bio.powered = g->remoteveh() != nullptr || !get_value( "remote_controlling" ).empty();
Expand Down
11 changes: 6 additions & 5 deletions src/character.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7206,7 +7206,7 @@ void Character::update_stamina( int turns )
stamina_recovery += bonus;
bonus /= 10;
bonus = std::max( bonus, 1 );
mod_power_level( units::from_kilojoule( -bonus ) );
mod_power_level( units::from_kilojoule( static_cast<std::int64_t>( -bonus ) ) );
}
}

Expand Down Expand Up @@ -9794,7 +9794,8 @@ units::energy Character::available_ups() const

if( is_mounted() && mounted_creature.get()->has_flag( mon_flag_RIDEABLE_MECH ) ) {
auto *mons = mounted_creature.get();
available_charges += units::from_kilojoule( mons->battery_item->ammo_remaining() );
available_charges += units::from_kilojoule( static_cast<std::int64_t>
( mons->battery_item->ammo_remaining() ) );
}

bool has_bio_powered_ups = false;
Expand All @@ -9808,7 +9809,7 @@ units::energy Character::available_ups() const
}

cache_visit_items_with( flag_IS_UPS, [&available_charges]( const item & it ) {
available_charges += units::from_kilojoule( it.ammo_remaining() );
available_charges += units::from_kilojoule( static_cast<std::int64_t>( it.ammo_remaining() ) );
} );

return available_charges;
Expand Down Expand Up @@ -9875,7 +9876,7 @@ std::list<item> Character::use_charges( const itype_id &what, int qty, const int
} else if( what == itype_UPS ) {
// Fairly sure that nothing comes here. But handle it anyways.
debugmsg( _( "This UPS use needs updating. Create issue on github." ) );
consume_ups( units::from_kilojoule( qty ), radius );
consume_ups( units::from_kilojoule( static_cast<std::int64_t>( qty ) ), radius );
return res;
}

Expand Down Expand Up @@ -9908,7 +9909,7 @@ std::list<item> Character::use_charges( const itype_id &what, int qty, const int
}

if( has_tool_with_UPS ) {
consume_ups( units::from_kilojoule( qty ), radius );
consume_ups( units::from_kilojoule( static_cast<std::int64_t>( qty ) ), radius );
}

return res;
Expand Down
9 changes: 6 additions & 3 deletions src/consumption.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,8 @@ std::pair<int, int> Character::fun_for( const item &comest, bool ignore_already_
}

if( fun < 0 && has_active_bionic( bio_taste_blocker ) &&
get_power_level() > units::from_kilojoule( std::abs( comest.get_comestible_fun() ) ) ) {
get_power_level() > units::from_kilojoule( static_cast<std::int64_t>( std::abs(
comest.get_comestible_fun() ) ) ) ) {
fun = 0;
}

Expand Down Expand Up @@ -1123,8 +1124,10 @@ static bool eat( item &food, Character &you, bool force )
}

if( you.has_active_bionic( bio_taste_blocker ) && food.get_comestible_fun() < 0 &&
you.get_power_level() > units::from_kilojoule( std::abs( food.get_comestible_fun() ) ) ) {
you.mod_power_level( units::from_kilojoule( food.get_comestible_fun() ) );
you.get_power_level() > units::from_kilojoule( static_cast<std::int64_t>( std::abs(
food.get_comestible_fun() ) ) ) ) {
you.mod_power_level( units::from_kilojoule( static_cast<std::int64_t>
( food.get_comestible_fun() ) ) );
}

if( food.has_flag( flag_FUNGAL_VECTOR ) && !you.has_trait( trait_M_IMMUNE ) ) {
Expand Down
2 changes: 1 addition & 1 deletion src/crafting.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2373,7 +2373,7 @@ void Character::consume_tools( map &m, const comp_selection<tool_comp> &tool, in
m.use_charges( reachable_pts, tool.comp.type, quantity, return_true<item>, bcp );
// Map::use_charges() does not handle UPS charges.
if( quantity > 0 ) {
m.consume_ups( reachable_pts, units::from_kilojoule( quantity ) );
m.consume_ups( reachable_pts, units::from_kilojoule( static_cast<std::int64_t>( quantity ) ) );
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/explosion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,8 @@ void emp_blast( const tripoint &p )
add_msg( m_bad, _( "The EMP blast drains your power." ) );
int max_drain = ( player_character.get_power_level() > 1000_kJ ? 1000 : units::to_kilojoule(
player_character.get_power_level() ) );
player_character.mod_power_level( units::from_kilojoule( -rng( 1 + max_drain / 3, max_drain ) ) );
player_character.mod_power_level( units::from_kilojoule( static_cast<std::int64_t>( -rng(
1 + max_drain / 3, max_drain ) ) ) );
}
// TODO: More effects?
//e-handcuffs effects
Expand Down
8 changes: 4 additions & 4 deletions src/item.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10672,7 +10672,7 @@ units::energy item::energy_remaining( const Character *carrier ) const
if( is_magazine() ) {
for( const item *e : contents.all_items_top( pocket_type::MAGAZINE ) ) {
if( e->typeId() == itype_battery ) {
ret += units::from_kilojoule( e->charges );
ret += units::from_kilojoule( static_cast<std::int64_t>( e->charges ) );
}
}
}
Expand Down Expand Up @@ -10817,7 +10817,7 @@ int item::ammo_consume( int qty, const tripoint &pos, Character *carrier )
// Guns handle energy in energy_consume()
if( carrier != nullptr && type->tool &&
( has_flag( flag_USE_UPS ) || has_flag( flag_USES_BIONIC_POWER ) ) ) {
units::energy wanted_energy = units::from_kilojoule( qty );
units::energy wanted_energy = units::from_kilojoule( static_cast<std::int64_t>( qty ) );

if( has_flag( flag_USE_UPS ) ) {
wanted_energy -= carrier->consume_ups( wanted_energy );
Expand Down Expand Up @@ -10849,7 +10849,7 @@ units::energy item::energy_consume( units::energy qty, const tripoint &pos, Char
// Consume battery(ammo) and other fuel (if allowed)
if( is_battery() || fuel_efficiency >= 0 ) {
int consumed_kj = contents.ammo_consume( units::to_kilojoule( qty ), pos, fuel_efficiency );
qty -= units::from_kilojoule( consumed_kj );
qty -= units::from_kilojoule( static_cast<std::int64_t>( consumed_kj ) );
// fix negative quantity
if( qty < 0_J ) {
qty = 0_J;
Expand Down Expand Up @@ -10877,7 +10877,7 @@ units::energy item::energy_consume( units::energy qty, const tripoint &pos, Char
// Should happen only if battery powered and energy per shot is not integer kJ.
if( qty > 0_kJ && is_battery() ) {
int consumed_kj = contents.ammo_consume( 1, pos );
qty -= units::from_kilojoule( consumed_kj );
qty -= units::from_kilojoule( static_cast<std::int64_t>( consumed_kj ) );
}

return wanted_energy - qty;
Expand Down
3 changes: 2 additions & 1 deletion src/item_contents.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1233,7 +1233,8 @@ int item_contents::ammo_consume( int qty, const tripoint &pos, float fuel_effici
if( !pocket.empty() && pocket.front().is_fuel() && fuel_efficiency >= 0 ) {
// if using fuel instead of battery, everything is in kJ
// charges is going to be the energy needed over the energy in 1 unit of fuel * the efficiency of the generator
int charges_used = ceil( static_cast<float>( units::from_kilojoule( qty ).value() ) / (
int charges_used = ceil( static_cast<float>( units::from_kilojoule( static_cast<std::int64_t>
( qty ) ).value() ) / (
static_cast<float>( pocket.front().fuel_energy().value() ) * fuel_efficiency ) );

const int res = pocket.ammo_consume( charges_used );
Expand Down
2 changes: 1 addition & 1 deletion src/magic.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2188,7 +2188,7 @@ bool known_magic::has_enough_energy( const Character &guy, const spell &sp ) con
case magic_energy_type::mana:
return available_mana() >= cost;
case magic_energy_type::bionic:
return guy.get_power_level() >= units::from_kilojoule( cost );
return guy.get_power_level() >= units::from_kilojoule( static_cast<std::int64_t>( cost ) );
case magic_energy_type::stamina:
return guy.get_stamina() >= cost;
case magic_energy_type::hp:
Expand Down
2 changes: 1 addition & 1 deletion src/magic_spell_effect.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1146,7 +1146,7 @@ void spell_effect::recover_energy( const spell &sp, Creature &caster, const trip
you->mod_fatigue( -healing );
} else if( energy_source == "BIONIC" ) {
if( healing > 0 ) {
you->mod_power_level( units::from_kilojoule( healing ) );
you->mod_power_level( units::from_kilojoule( static_cast<std::int64_t>( healing ) ) );
} else {
you->mod_stamina( healing );
}
Expand Down
2 changes: 1 addition & 1 deletion src/monster.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2906,7 +2906,7 @@ units::energy monster::use_mech_power( units::energy amt )
const int max_drain = battery_item->ammo_remaining();
const int consumption = std::min( static_cast<int>( units::to_kilojoule( amt ) ), max_drain );
battery_item->ammo_consume( consumption, pos(), nullptr );
return units::from_kilojoule( consumption );
return units::from_kilojoule( static_cast<std::int64_t>( consumption ) );
}

int monster::mech_str_addition() const
Expand Down
2 changes: 1 addition & 1 deletion src/move_mode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ float move_mode::move_speed_mult() const

units::energy move_mode::mech_power_use() const
{
return units::from_kilojoule( _mech_power_use );
return units::from_kilojoule( static_cast<std::int64_t>( _mech_power_use ) );
}

int move_mode::swim_speed_mod() const
Expand Down
3 changes: 2 additions & 1 deletion src/vehicle.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4843,7 +4843,8 @@ void vehicle::consume_fuel( int load, bool idling )
}
// decreased stamina burn scalable with load
if( player_character.has_active_bionic( bio_jointservo ) ) {
player_character.mod_power_level( units::from_kilojoule( -std::max( eff_load / 20, 1 ) ) );
player_character.mod_power_level( units::from_kilojoule( static_cast<std::int64_t>( -std::max(
eff_load / 20, 1 ) ) ) );
mod -= std::max( eff_load / 5, 5 );
}

Expand Down
8 changes: 4 additions & 4 deletions src/wish.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ void debug_menu::wishbionics( Character *you )
int new_value = 0;
if( query_int( new_value, _( "Set the value to (in kJ)? Currently: %s" ),
units::display( power_max ) ) ) {
you->set_max_power_level( units::from_kilojoule( new_value ) );
you->set_max_power_level( units::from_kilojoule( static_cast<std::int64_t>( new_value ) ) );
you->set_power_level( you->get_power_level() );
}
break;
Expand All @@ -438,7 +438,7 @@ void debug_menu::wishbionics( Character *you )
int new_value = 0;
if( query_int( new_value, _( "Set the value to (in J)? Currently: %s" ),
units::display( power_max ) ) ) {
you->set_max_power_level( units::from_joule( new_value ) );
you->set_max_power_level( units::from_joule( static_cast<std::int64_t>( new_value ) ) );
you->set_power_level( you->get_power_level() );
}
break;
Expand All @@ -447,15 +447,15 @@ void debug_menu::wishbionics( Character *you )
int new_value = 0;
if( query_int( new_value, _( "Set the value to (in kJ)? Currently: %s" ),
units::display( power_level ) ) ) {
you->set_power_level( units::from_kilojoule( new_value ) );
you->set_power_level( units::from_kilojoule( static_cast<std::int64_t>( new_value ) ) );
}
break;
}
case 6: {
int new_value = 0;
if( query_int( new_value, _( "Set the value to (in J)? Currently: %s" ),
units::display( power_level ) ) ) {
you->set_power_level( units::from_joule( new_value ) );
you->set_power_level( units::from_joule( static_cast<std::int64_t>( new_value ) ) );
}
break;
}
Expand Down
6 changes: 4 additions & 2 deletions tests/food_fun_for_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,8 @@ TEST_CASE( "fun_for_bionic_bio_taste_blocker", "[fun_for][food][bionic]" )
// Needs 1 kJ per negative fun unit to nullify bad taste
dummy.set_power_level( 10_kJ );
REQUIRE( garlic_fun < -10 );
REQUIRE_FALSE( dummy.get_power_level() > units::from_kilojoule( std::abs( garlic_fun ) ) );
REQUIRE_FALSE( dummy.get_power_level() > units::from_kilojoule( static_cast<std::int64_t>( std::abs(
garlic_fun ) ) ) );

THEN( "the bad taste remains" ) {
actual_fun = dummy.fun_for( garlic );
Expand All @@ -438,7 +439,8 @@ TEST_CASE( "fun_for_bionic_bio_taste_blocker", "[fun_for][food][bionic]" )
WHEN( "it has enough power" ) {
REQUIRE( garlic_fun >= -20 );
dummy.set_power_level( 20_kJ );
REQUIRE( dummy.get_power_level() > units::from_kilojoule( std::abs( garlic_fun ) ) );
REQUIRE( dummy.get_power_level() > units::from_kilojoule( static_cast<std::int64_t>( std::abs(
garlic_fun ) ) ) );

THEN( "the bad taste is nullified" ) {
actual_fun = dummy.fun_for( garlic );
Expand Down
1 change: 1 addition & 0 deletions tools/clang-tidy-plugin/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ set(CataAnalyzerSrc
TranslationsInDebugMessagesCheck.cpp
TranslatorCommentsCheck.cpp
U8PathCheck.cpp
UnitOverflowCheck.cpp
UnsequencedCallsCheck.cpp
UnusedStaticsCheck.cpp
UseLocalizedSortingCheck.cpp
Expand Down
2 changes: 2 additions & 0 deletions tools/clang-tidy-plugin/CataTidyModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include "TranslationsInDebugMessagesCheck.h"
#include "TranslatorCommentsCheck.h"
#include "U8PathCheck.h"
#include "UnitOverflowCheck.h"
#include "UnsequencedCallsCheck.h"
#include "UnusedStaticsCheck.h"
#include "UseLocalizedSortingCheck.h"
Expand Down Expand Up @@ -100,6 +101,7 @@ class CataModule : public ClangTidyModule
"cata-translations-in-debug-messages" );
CheckFactories.registerCheck<TranslatorCommentsCheck>( "cata-translator-comments" );
CheckFactories.registerCheck<U8PathCheck>( "cata-u8-path" );
CheckFactories.registerCheck<UnitOverflowCheck>( "cata-unit-overflow" );
CheckFactories.registerCheck<UnsequencedCallsCheck>( "cata-unsequenced-calls" );
CheckFactories.registerCheck<UnusedStaticsCheck>( "cata-unused-statics" );
CheckFactories.registerCheck<UseLocalizedSortingCheck>( "cata-use-localized-sorting" );
Expand Down
86 changes: 86 additions & 0 deletions tools/clang-tidy-plugin/UnitOverflowCheck.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
#include "UnitOverflowCheck.h"
#include "Utils.h"

#include <clang/ASTMatchers/ASTMatchers.h>
#include <clang/Basic/Diagnostic.h>

#include <map>

using namespace clang::ast_matchers;

namespace clang::tidy::cata
{

struct QuantityUnit {
std::string_view Unit;
std::int64_t ConversionFactor;
};

static const std::map<std::string_view, QuantityUnit> FunctionAndQuantityTypes {
{"from_joule", {"energy", 1'000LL}},
{"from_kilojoule", {"energy", 1'000'000LL}},
};

void UnitOverflowCheck::registerMatchers( ast_matchers::MatchFinder *Finder )
{
for( const auto &[functionName, unit] : FunctionAndQuantityTypes ) {
Finder->addMatcher(
callExpr( callee( functionDecl( hasName( functionName ) ).bind( "func" ) ),
hasArgument( 0, expr( hasType( isInteger() ) ).bind( "arg" ) ) ),
this
);
}
}

void UnitOverflowCheck::check( const ast_matchers::MatchFinder::MatchResult &Result )
{
const FunctionDecl *func = Result.Nodes.getNodeAs<FunctionDecl>( "func" );
const Expr *arg = Result.Nodes.getNodeAs<Expr>( "arg" );
if( !func || !arg ) {
return;
}
const QualType type = arg->getType();
const std::int64_t width = Result.Context->getTypeInfo( type ).Width;
if( width >= 64 ) {
return;
}
const SourceManager &sourceManager = *Result.SourceManager;
if( sourceManager.getFilename( arg->getBeginLoc() ).ends_with( "src/units.h" ) ) {
return;
}
const std::string functionName = func->getNameAsString();
if( const IntegerLiteral *literal = dyn_cast<IntegerLiteral>( arg ) ) {
const bool isSigned = literal->getType()->isSignedIntegerType();
std::int64_t minVal = 0;
std::int64_t maxVal = 0;
if( isSigned ) {
minVal = llvm::APInt::getSignedMinValue( width ).getSExtValue();
maxVal = llvm::APInt::getSignedMaxValue( width ).getSExtValue();
} else {
minVal = llvm::APInt::getMinValue( width ).getSExtValue();
maxVal = llvm::APInt::getMaxValue( width ).getSExtValue();
}
const std::int64_t multiplier = FunctionAndQuantityTypes.at( functionName ).ConversionFactor;
const std::int64_t val = literal->getValue().getSExtValue() * multiplier;
if( val < minVal || val > maxVal ) {
emitDiag( arg->getBeginLoc(), FunctionAndQuantityTypes.at( functionName ).Unit, type.getAsString(),
functionName, FixItHint::CreateReplacement( arg->getSourceRange(),
( getText( Result, arg ) + Twine( "LL" ) ).str() ) );
}
} else {
emitDiag( arg->getBeginLoc(), FunctionAndQuantityTypes.at( functionName ).Unit, type.getAsString(),
functionName, FixItHint::CreateReplacement( arg->getSourceRange(),
( Twine( "static_cast<std::int64_t>( " ) + getText( Result, arg ) + " )" ).str() ) );
}
}

void UnitOverflowCheck::emitDiag( const SourceLocation &loc, const std::string_view QuantityType,
const std::string_view ValueType,
const std::string_view FunctionName, const clang::FixItHint &fix )
{
diag( loc,
"constructing %0 quantity from '%1' can overflow in '%2' in multiplying with the conversion factor" )
<< QuantityType << ValueType << FunctionName << fix;
}

} // namespace clang::tidy::cata
Loading

0 comments on commit 4598b15

Please sign in to comment.