Skip to content

Commit

Permalink
More strict user input checking.
Browse files Browse the repository at this point in the history
* Redesign most init_ functions to return type void instead of bool,
using exceptions to indicate failures.
* Use some nested exceptions to add more failure info for the most
common cases (player, action, buff).
* Catch exceptions in child threads and properly propagate to parent
through sim->error & cancel.
* Remove std::terminate from event_t::delete functions so they can be
properly destructed. Users nowadays use make_event<> uniformly anyway,
so there is less risk of misusing them.
  • Loading branch information
scamille committed Jul 3, 2018
1 parent e9b59e0 commit 2497cce
Show file tree
Hide file tree
Showing 43 changed files with 966 additions and 1,204 deletions.
65 changes: 36 additions & 29 deletions engine/action/sc_action.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -830,7 +830,7 @@ void action_t::parse_target_str()
{
if ( option.target_str[ 0 ] >= '0' && option.target_str[ 0 ] <= '9' )
{
option.target_number = atoi( option.target_str.c_str() );
option.target_number = std::stoi( option.target_str );
player_t* p = find_target_by_number( option.target_number );
// Numerical targeting is intended to be dynamic, so don't give an error message if we can't find the target yet
if ( p )
Expand Down Expand Up @@ -858,14 +858,13 @@ void action_t::parse_options( const std::string& options_str )
try
{
opts::parse( sim, name(), options, options_str );
parse_target_str();
}
catch ( const std::exception& e )
{
sim->errorf( "%s %s: Unable to parse options str '%s': %s", player->name(), name(), options_str.c_str(), e.what() );
sim->cancel();
}

parse_target_str();
}

bool action_t::verify_actor_level() const
Expand Down Expand Up @@ -2171,8 +2170,8 @@ void action_t::init()

if ( !sync_action )
{
sim->errorf( "Unable to find sync action '%s' for primary action '%s'\n", option.sync_str.c_str(), name() );
sim->cancel();
throw std::runtime_error(fmt::format("Unable to find sync action '{}' for primary action.",
option.sync_str ));
}
}

Expand Down Expand Up @@ -2278,10 +2277,9 @@ void action_t::init()
player->precombat_action_list.push_back( this );
}
else
sim->errorf(
"Player %s attempted to add harmful action %s to precombat action list. Only spells with travel time/cast "
"time may be cast here.",
player->name(), name() );
{
throw std::runtime_error("Can only add harmful action with travel or cast-time to precombat action list.");
}
}
else
player->precombat_action_list.push_back( this );
Expand Down Expand Up @@ -2326,10 +2324,8 @@ void action_t::init()
assert( ( !impact_action || impact_action->background ) && "Impact action needs to be set to background." );
}

bool action_t::init_finished()
void action_t::init_finished()
{
bool ret = true;

if ( !option.target_if_str.empty() )
{
std::string::size_type offset = option.target_if_str.find( ':' );
Expand All @@ -2351,40 +2347,51 @@ bool action_t::init_finished()
}
else
{
sim->errorf( "%s unknown target_if mode '%s' for choose_target. Valid values are 'min', 'max', 'first'.",
player->name(), target_if_type_str.c_str() );
background = true;
throw std::invalid_argument(fmt::format("Unknown target_if mode '{}' for choose_target. Valid values are 'min', 'max', 'first'.",
target_if_type_str ));
}
}
else if ( !option.target_if_str.empty() )
{
target_if_mode = TARGET_IF_FIRST;
}

if ( !option.target_if_str.empty() &&
( target_if_expr = expr_t::parse( this, option.target_if_str, sim->optimize_expressions ) ) == 0 )
ret = false;
if ( !option.target_if_str.empty() )
{
target_if_expr = expr_t::parse( this, option.target_if_str, sim->optimize_expressions );
if ( target_if_expr == 0 )
{
throw std::invalid_argument(fmt::format("Could not parse target if expression from '{}'", option.target_if_str));
}
}
}

if ( !option.if_expr_str.empty() &&
( if_expr = expr_t::parse( this, option.if_expr_str, sim->optimize_expressions ) ) == 0 )
if ( !option.if_expr_str.empty() )
{
ret = false;
if_expr = expr_t::parse( this, option.if_expr_str, sim->optimize_expressions );
if ( if_expr == 0 )
{
throw std::invalid_argument(fmt::format("Could not parse if expression from '{}'", option.if_expr_str));
}
}

if ( !option.interrupt_if_expr_str.empty() &&
( interrupt_if_expr = expr_t::parse( this, option.interrupt_if_expr_str, sim->optimize_expressions ) ) == 0 )
if ( !option.interrupt_if_expr_str.empty() )
{
ret = false;
interrupt_if_expr = expr_t::parse( this, option.interrupt_if_expr_str, sim->optimize_expressions );
if ( interrupt_if_expr == 0 )
{
throw std::invalid_argument(fmt::format("Could not parse interrupt if expression from '{}'", option.interrupt_if_expr_str));
}
}

if ( !option.early_chain_if_expr_str.empty() &&
( early_chain_if_expr = expr_t::parse( this, option.early_chain_if_expr_str, sim->optimize_expressions ) ) == 0 )
if ( !option.early_chain_if_expr_str.empty() )
{
ret = false;
early_chain_if_expr = expr_t::parse( this, option.early_chain_if_expr_str, sim->optimize_expressions );
if ( early_chain_if_expr == 0 )
{
throw std::invalid_argument(fmt::format("Could not parse chain if expression from '{}'", option.early_chain_if_expr_str));
}
}

return ret;
}

// action_t::reset ==========================================================
Expand Down
3 changes: 1 addition & 2 deletions engine/buff/sc_buff.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1346,8 +1346,7 @@ void buff_t::extend_duration( player_t* p, timespan_t extra_seconds )

if ( stack_behavior == buff_stack_behavior::ASYNCHRONOUS )
{
sim->errorf( "%s attempts to extend asynchronous buff %s.", p->name(), name() );
sim->cancel();
throw std::runtime_error(fmt::format("'{}' attempts to extend asynchronous buff '{}'.", p->name(), name() ));
}

assert( expiration.size() == 1 );
Expand Down
4 changes: 2 additions & 2 deletions engine/class_modules/priest/sc_priest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -286,11 +286,11 @@ struct summon_pet_t : public priest_spell_t
harmful = false;
}

bool init_finished() override
void init_finished() override
{
pet = player->find_pet( pet_name );

return priest_spell_t::init_finished();
priest_spell_t::init_finished();
}

void execute() override
Expand Down
26 changes: 11 additions & 15 deletions engine/class_modules/sc_death_knight.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -826,15 +826,15 @@ struct death_knight_t : public player_t {
// Character Definition
void init_spells() override;
void init_action_list() override;
bool init_actions() override;
void init_actions() override;
void init_rng() override;
void init_base_stats() override;
void init_scaling() override;
void create_buffs() override;
void init_gains() override;
void init_procs() override;
void init_absorb_priority() override;
bool init_finished() override;
void init_finished() override;
double composite_armor_multiplier() const override;
double composite_bonus_armor() const override;
double composite_melee_attack_power() const override;
Expand Down Expand Up @@ -866,7 +866,7 @@ struct death_knight_t : public player_t {
void assess_damage_imminent( school_e, dmg_e, action_state_t* ) override;
void target_mitigation( school_e, dmg_e, action_state_t* ) override;
void do_damage( action_state_t* ) override;
bool create_actions() override;
void create_actions() override;
action_t* create_action( const std::string& name, const std::string& options ) override;
expr_t* create_expression( const std::string& name ) override;
void create_pets() override;
Expand Down Expand Up @@ -2556,9 +2556,9 @@ struct death_knight_action_t : public Base
}
}

bool init_finished() override
void init_finished() override
{
bool ret = action_base_t::init_finished();
action_base_t::init_finished();

if ( this -> base_costs[ RESOURCE_RUNE ] || this -> base_costs[ RESOURCE_RUNIC_POWER ] )
{
Expand All @@ -2574,8 +2574,6 @@ struct death_knight_action_t : public Base
{
this -> gcd_haste = HASTE_ATTACK;
}

return ret;
}

timespan_t gcd() const override
Expand Down Expand Up @@ -6667,7 +6665,7 @@ void death_knight_t::start_cold_heart_talent()

// death_knight_t::create_actions ===========================================

bool death_knight_t::create_actions()
void death_knight_t::create_actions()
{
if ( spec.festering_wound -> ok() )
{
Expand Down Expand Up @@ -6704,7 +6702,7 @@ bool death_knight_t::create_actions()
active_spells.cold_heart_talent = new cold_heart_talent_damage_t( this );
}

return player_t::create_actions();
player_t::create_actions();
}

// death_knight_t::create_action ===========================================
Expand Down Expand Up @@ -7532,12 +7530,12 @@ void death_knight_t::init_action_list()
player_t::init_action_list();
}

bool death_knight_t::init_actions()
void death_knight_t::init_actions()
{
active_spells.blood_plague = new blood_plague_t( this );
active_spells.frost_fever = new frost_fever_t( this );

return player_t::init_actions();
player_t::init_actions();
}

// death_knight_t::init_scaling =============================================
Expand Down Expand Up @@ -7787,18 +7785,16 @@ void death_knight_t::init_absorb_priority()

// death_knight_t::init_finished ============================================

bool death_knight_t::init_finished()
void death_knight_t::init_finished()
{
auto ret = player_t::init_finished();
player_t::init_finished();

if ( deprecated_dnd_expression )
{
sim -> errorf( "Player %s, Death and Decay and Defile expressions of the form "
"'dot.death_and_decay.X' have been deprecated. Use 'death_and_decay.ticking' "
"or death_and_decay.remains' instead.", name() );
}

return ret;
}

// death_knight_t::reset ====================================================
Expand Down
16 changes: 6 additions & 10 deletions engine/class_modules/sc_demon_hunter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1106,13 +1106,13 @@ class demon_hunter_action_t : public Base
return g;
}

virtual bool init_finished() override
void init_finished() override
{
// For reporting purposes only, as the game displays this as SCHOOL_CHAOS
if ( ab::stats->school == SCHOOL_CHROMATIC )
ab::stats->school = SCHOOL_CHAOS;

return ab::init_finished();
ab::init_finished();
}

virtual double composite_target_multiplier( player_t* target ) const override
Expand Down Expand Up @@ -3006,9 +3006,9 @@ struct blade_dance_base_t : public demon_hunter_attack_t
return c;
}

virtual bool init_finished() override
void init_finished() override
{
bool f = demon_hunter_attack_t::init_finished();
demon_hunter_attack_t::init_finished();

for ( size_t i = 0; i < attacks.size(); i++ )
{
Expand All @@ -3025,8 +3025,6 @@ struct blade_dance_base_t : public demon_hunter_attack_t
attacks.back()->trail_of_ruin_dot = trail_of_ruin_dot;
}
}

return f;
}

virtual void execute() override
Expand Down Expand Up @@ -3216,17 +3214,15 @@ struct chaos_strike_base_t : public demon_hunter_attack_t
return c;
}

virtual bool init_finished() override
void init_finished() override
{
bool f = demon_hunter_attack_t::init_finished();
demon_hunter_attack_t::init_finished();

// Use one stats object for all parts of the attack.
for ( size_t i = 0; i < attacks.size(); i++ )
{
attacks[ i ]->stats = stats;
}

return f;
}

virtual void execute() override
Expand Down
5 changes: 5 additions & 0 deletions engine/class_modules/sc_enemy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1183,6 +1183,11 @@ void enemy_t::init_base_stats()
{
race = util::parse_race_type( sim -> target_race );
race_str = util::race_type_string( race );
if (race == RACE_NONE)
{
throw std::invalid_argument(
fmt::format( "{} could not parse race from sim target race '{}'.", name(), sim->target_race ) );
}
}

base.attack_crit_chance = 0.05;
Expand Down
Loading

0 comments on commit 2497cce

Please sign in to comment.