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; }