From c46a0ce27cd1470475565c7beefaaa5b09d8e2c0 Mon Sep 17 00:00:00 2001 From: Kp Date: Sat, 28 Jan 2017 18:12:20 +0000 Subject: [PATCH] Remove some remotely-triggerable fatal exceptions By design, valptridx will throw an exception on invalid input. This is better than silently permitting invalid input to corrupt program state. Past releases blindly trusted that multiplayer peers would not send invalid input. Conversion to the valptridx design eliminated the undefined behavior when peers send invalid input, but still allowed multiplayer peers to remotely crash the game by sending invalid inputs. Add a mechanism to trap invalid inputs and gracefully ignore those messages. This may cause game consistency issues, but will not allow data corruption. --- common/include/fwd-valptridx.h | 3 + common/include/valptridx.h | 132 +++++++++++++++++++++++++++++++++ similar/main/gamesave.cpp | 8 +- similar/main/multi.cpp | 33 +++++++-- similar/main/switch.cpp | 8 +- 5 files changed, 168 insertions(+), 16 deletions(-) diff --git a/common/include/fwd-valptridx.h b/common/include/fwd-valptridx.h index 7419f4bb9..5161f8e81 100644 --- a/common/include/fwd-valptridx.h +++ b/common/include/fwd-valptridx.h @@ -47,6 +47,8 @@ class valptridx : class ic; /* allow_invalid + const_policy */ class vm; /* require_valid + mutable_policy */ class im; /* allow_invalid + mutable_policy */ + template + class guarded; public: class array_managed_type; @@ -75,6 +77,7 @@ protected: template class basic_ptridx; class allow_end_construction; + class assume_nothrow_index; static constexpr const array_managed_type &get_array(const_pointer_type p) { diff --git a/common/include/valptridx.h b/common/include/valptridx.h index 206f43c55..388abff45 100644 --- a/common/include/valptridx.h +++ b/common/include/valptridx.h @@ -154,6 +154,10 @@ class valptridx::partial_policy::require_valid public: static constexpr tt::false_type allow_nullptr{}; static constexpr tt::false_type check_allowed_invalid_index(index_type) { return {}; } + static constexpr bool check_nothrow_index(index_type i) + { + return std::less()(i, array_size); + } }; template @@ -165,6 +169,10 @@ public: { return i == static_cast(~0); } + static constexpr bool check_nothrow_index(index_type i) + { + return check_allowed_invalid_index(i) || require_valid::check_nothrow_index(i); + } }; template @@ -275,6 +283,10 @@ protected: m_idx(check_index_range(DXX_VALPTRIDX_REPORT_STANDARD_LEADER_COMMA_R_PASS_VARS i, &a)) { } + basic_idx(index_type i, array_managed_type &, const assume_nothrow_index *) : + m_idx(i) + { + } public: template basic_idx(const magic_constant &) : @@ -385,6 +397,10 @@ public: m_ptr(&a[check_index_range(DXX_VALPTRIDX_REPORT_STANDARD_LEADER_COMMA_R_PASS_VARS i, &a)]) { } + basic_ptr(index_type i, array_managed_type &a, const assume_nothrow_index *) : + m_ptr(&a[i]) + { + } basic_ptr(pointer_type p) = delete; basic_ptr(DXX_VALPTRIDX_REPORT_STANDARD_LEADER_COMMA_R_DEFN_VARS pointer_type p, array_managed_type &a) : /* No array consistency check here, since some code incorrectly @@ -551,6 +567,11 @@ public: vidx_type(DXX_VALPTRIDX_REPORT_STANDARD_LEADER_COMMA_R_PASS_VARS i, a, e) { } + basic_ptridx(index_type i, array_managed_type &a, const assume_nothrow_index *e) : + vptr_type(i, a, e), + vidx_type(i, a, e) + { + } basic_ptridx(DXX_VALPTRIDX_REPORT_STANDARD_LEADER_COMMA_R_DEFN_VARS pointer_type p, array_managed_type &a) : /* Null pointer is never allowed when an index must be computed. * Check for null, then use the reference constructor for @@ -592,6 +613,107 @@ protected: } }; +template +template +class valptridx::guarded +{ + static_assert(std::is_trivially_destructible::value, "non-trivial destructor found for guarded_type"); + enum state : uint8_t + { + /* empty - the untrusted input was invalid, so no guarded_type + * exists + */ + empty, + /* initialized - the untrusted input was valid, so a + * guarded_type type exists, but the calling code has not yet + * tested the state of this guarded

+ */ + initialized, + /* checked - the untrusted input was valid, and the calling code + * has called operator bool() + */ + checked, + }; + union { + state m_dummy; + guarded_type m_value; + }; + mutable state m_state; +public: + guarded(std::nullptr_t) : + m_dummy(), m_state(empty) + { + } + guarded(guarded_type &&v) : + m_value(std::move(v)), m_state(initialized) + { + } + __attribute_warn_unused_result + explicit operator bool() const + { + /* + * If no contained guarded_type exists, return false. + * Otherwise, record that the result has been tested and then + * return true. operator*() uses m_state to enforce that the + * result is tested. + */ + if (m_state == empty) + return false; + m_state = checked; + return true; + } + __attribute_warn_unused_result + guarded_type operator*() const & + { + /* + * Correct code will always execute as if this method was just + * the return statement, with none of the sanity checks. The + * checks are present to catch misuse of this type, preferably + * at compile-time, but at least at runtime. + */ +#define DXX_VALPTRIDX_GUARDED_OBJECT_NO "access to guarded object that does not exist" +#define DXX_VALPTRIDX_GUARDED_OBJECT_MAYBE "access to guarded object that may not exist" +#ifdef DXX_CONSTANT_TRUE + /* If the contained object might not exist: */ + if (!DXX_CONSTANT_TRUE(m_state == checked)) + { + /* + * Always fail. Choose an error message and function name + * based on whether the contained type provably does not + * exist. It provably does not exist if this call is on a + * path where operator bool() returned false. It + * conditionally might not exist if this call is on a path + * where operator bool() has not been called. + */ + if (DXX_CONSTANT_TRUE(m_state == empty)) + DXX_ALWAYS_ERROR_FUNCTION(guarded_accessed_empty, DXX_VALPTRIDX_GUARDED_OBJECT_NO); + else + DXX_ALWAYS_ERROR_FUNCTION(guarded_accessed_unchecked, DXX_VALPTRIDX_GUARDED_OBJECT_MAYBE); + } +#else + /* + * If the compiler does not offer constant truth analysis + * (perhaps because of insufficient optimization), then emit a + * runtime check for whether the guarded_type exists. + * + * This test can throw even if the contained object is valid, if + * the caller did not first validate that the contained object + * is valid. This restriction is necessary since inputs are + * usually valid even when untested, so throwing only on state + * `empty` would allow incorrect usage to persist in the code + * until someone happened to receive an invalid input from an + * untrusted source. + */ + if (m_state != checked) + throw std::logic_error(m_state == empty ? DXX_VALPTRIDX_GUARDED_OBJECT_NO : DXX_VALPTRIDX_GUARDED_OBJECT_MAYBE); +#endif +#undef DXX_VALPTRIDX_GUARDED_OBJECT_MAYBE +#undef DXX_VALPTRIDX_GUARDED_OBJECT_NO + return m_value; + } + guarded_type operator*() const && = delete; +}; + template class valptridx::array_managed_type : public detail::valptridx_array_type_count, @@ -633,6 +755,16 @@ public: basic_ival_global_factory(const basic_ival_global_factory &) = delete; basic_ival_global_factory &operator=(const basic_ival_global_factory &) = delete; __attribute_warn_unused_result + guarded

check_untrusted(index_type i) const + { + if (P::check_nothrow_index(i)) + return P(i, get_array(), static_cast(nullptr)); + else + return nullptr; + } + template + guarded

check_untrusted(T &&) const = delete; + __attribute_warn_unused_result P operator()(typename P::index_type i DXX_VALPTRIDX_REPORT_STANDARD_LEADER_COMMA_L_DECL_VARS) const { return P(DXX_VALPTRIDX_REPORT_STANDARD_LEADER_COMMA_R_PASS_VARS i, get_array()); diff --git a/similar/main/gamesave.cpp b/similar/main/gamesave.cpp index 291f19a43..9f2c6d859 100644 --- a/similar/main/gamesave.cpp +++ b/similar/main/gamesave.cpp @@ -1107,10 +1107,10 @@ static int load_game_data(PHYSFS_File *LoadFile) { //light triggers don't require walls const auto side_num = tr.side[l]; auto wall_num = vsegptr(seg_num)->sides[side_num].wall_num; - try { - vwallptr(wall_num)->controlling_trigger = t; - } catch (const valptridx::index_range_exception &e) { - con_puts(CON_URGENT, e.what()); + if (const auto &&uwall = vwallptr.check_untrusted(wall_num)) + (*uwall)->controlling_trigger = t; + else + { LevelError("trigger %u link %u type %u references segment %hu, side %u which is an invalid wall; ignoring.", static_cast(t), l, tr.type, seg_num, side_num); } } diff --git a/similar/main/multi.cpp b/similar/main/multi.cpp index 48b799c64..85f20d7ad 100644 --- a/similar/main/multi.cpp +++ b/similar/main/multi.cpp @@ -1769,9 +1769,12 @@ static void multi_do_position(const playernum_t pnum, const ubyte *buf) static void multi_do_reappear(const playernum_t pnum, const ubyte *buf) { - const objnum_t objnum = GET_INTEL_SHORT(buf + 2); + const objnum_t objnum = GET_INTEL_SHORT(&buf[2]); - auto &obj = *vcobjptr(objnum); + const auto &&uobj = vcobjptr.check_untrusted(objnum); + if (!uobj) + return; + auto &obj = **uobj; if (obj.type != OBJ_PLAYER && obj.type != OBJ_GHOST) { con_printf(CON_URGENT, "%s:%u: BUG: object %hu has type %u, expected %u or %u", __FILE__, __LINE__, objnum, obj.type, OBJ_PLAYER, OBJ_GHOST); @@ -2165,7 +2168,10 @@ static void multi_do_door_open(const ubyte *buf) return; } - const auto &&seg = vsegptridx(segnum); + const auto &&useg = vsegptridx.check_untrusted(segnum); + if (!useg) + return; + const auto &&seg = *useg; if (seg->sides[side].wall_num == wall_none) { //Opening door on illegal wall Int3(); @@ -2214,7 +2220,10 @@ static void multi_do_controlcen_fire(const ubyte *buf) gun_num = buf[count]; count += 1; objnum = GET_INTEL_SHORT(buf + count); count += 2; - auto objp = vobjptridx(objnum); + const auto &&uobj = vobjptridx.check_untrusted(objnum); + if (!uobj) + return; + const auto &&objp = *uobj; Laser_create_new_easy(to_target, objp->ctype.reactor_info.gun_pos[gun_num], objp, weapon_id_type::CONTROLCEN_WEAPON_NUM, 1); } @@ -2229,7 +2238,10 @@ static void multi_do_create_powerup(const playernum_t pnum, const ubyte *buf) count++; powerup_type = buf[count++]; - const auto &&segnum = vsegptridx(GET_INTEL_SHORT(&buf[count])); + const auto &&useg = vsegptridx.check_untrusted(GET_INTEL_SHORT(&buf[count])); + if (!useg) + return; + const auto &&segnum = *useg; count += 2; objnum_t objnum = GET_INTEL_SHORT(buf + count); count += 2; memcpy(&new_pos, buf+count, sizeof(vms_vector)); count+=sizeof(vms_vector); @@ -2324,7 +2336,9 @@ static void multi_do_effect_blowup(const playernum_t pnum, const ubyte *buf) multi_do_protocol_frame(1, 0); // force packets to be sent, ensuring this packet will be attached to following MULTI_TRIGGER - segnum_t segnum = GET_INTEL_SHORT(buf + 2); + const auto &&useg = vsegptridx.check_untrusted(GET_INTEL_SHORT(&buf[2])); + if (!useg) + return; side = buf[4]; hitpnt.x = GET_INTEL_INT(buf + 5); hitpnt.y = GET_INTEL_INT(buf + 9); @@ -2337,7 +2351,7 @@ static void multi_do_effect_blowup(const playernum_t pnum, const ubyte *buf) laser.parent_type = OBJ_PLAYER; laser.parent_num = pnum; - check_effect_blowup(vsegptridx(segnum), side, hitpnt, laser, 0, 1); + check_effect_blowup(*useg, side, hitpnt, laser, 0, 1); } static void multi_do_drop_marker (const playernum_t pnum, const ubyte *buf) @@ -3949,7 +3963,10 @@ static void multi_do_light (const ubyte *buf) const auto sides = buf[3]; const segnum_t seg = GET_INTEL_SHORT(&buf[1]); - const auto &&segp = vsegptridx(seg); + const auto &&usegp = vsegptridx.check_untrusted(seg); + if (!usegp) + return; + const auto &&segp = *usegp; auto &side_array = segp->sides; for (i=0;i<6;i++) { diff --git a/similar/main/switch.cpp b/similar/main/switch.cpp index cb05abf82..2d11642e8 100644 --- a/similar/main/switch.cpp +++ b/similar/main/switch.cpp @@ -174,11 +174,11 @@ static int do_change_walls(const trigger &t, const uint8_t new_wall_type) } wall *w0p; - try { const auto w0num = segp->sides[side].wall_num; - w0p = vwallptr(w0num); - } catch (const valptridx::index_range_exception &e) { - con_puts(CON_URGENT, e.what()); + if (const auto &&uw0p = vwallptr.check_untrusted(w0num)) + w0p = *uw0p; + else + { LevelError("trigger %p link %u tried to open segment %hu, side %u which is an invalid wall; ignoring.", addressof(t), i, static_cast(segp), side); continue; }