GCC 6 `std::sort` sometimes compares an element to itself. For a normal
implementation of comparison, this is useless, but not harmful. The
render comparison predicate relies on accessing A[B[a][b]] when
comparing `a` and `b`. Array `B` has `-1` in positions where `a == b`,
which causes an access to `A[-1]`, which is undefined behavior. This
crashes when using _GLIBCXX_DEBUG:
Error: attempt to subscript container with out-of-bounds index -1, but
container only holds 8 elements.
Objects involved in the operation:
sequence "this" @ 0x0x335adf0 {
type = std::__debug::array<int, 8ul>::_Array_check_subscript<8ul>;
}
Since this is undefined behavior, non-debug builds might also misbehave.
Current data layouts make it likely that the failure would not have
externally observable consequences.
Prevent the invalid access by short-circuiting the result if `a == b`.
Rebirth built with `gcc -fsanitize=undefined` warns when binding a
reference to nullptr, even if that reference is never followed. This
could be reproduced using a guided missile against the first PIG in
Descent 2: Counterstrike level 1.
This object pointer is used only to test for address equality, so
nullptr is safe here. Switch to pass it as a pointer to prevent the
warning.
Macro LINE_SPACING previously used global grd_curcanv implicitly.
Change it to take a canvas argument. Change all callers to pass
grd_curcanv, so that usage is explicit.
As a macro, it always refers to the global grd_curcanv. This interferes
with converting canvas handling to be an argument. Expand GHEIGHT so
that uses of grd_curcanv can be changed individually.
As a macro, it always refers to the global grd_curcanv. This interferes
with converting canvas handling to be an argument. Expand GWIDTH so
that uses of grd_curcanv can be changed individually.
This allows the player to press a movement key to respawn after death, and the same keypress will cause the ship to move. Now this works with the option 'when dead, respawn by pressing any key' as well as 'when dead, respawn by pressing the Fire key'.
This is so game_flush_inputs() isn't called - part of change allowing player to respawn and begin moving with the same keypress. With this commit, you can use this feature with the 'when dead, respawn by pressing the Fire key' option - i.e. hold down movement key, then press fire key to respawn.
Later commits will enable the player to press a movement key to respawn on death, then move with the same keypress. This commit makes sure the player stops moving when the key is released.
Movement handling has an ugly hack that tries to grab powerups near the
console player, but it reuses general collision handling and fails to
check whether the player is alive.
Add a liveness check. Place the check so that it happens once, before
the objects are scanned, rather than being needlessly repeated for every
object.
Refactor the collision code to let the movement hack skip the parts it
does not need.
Reported-by: ryusei117 <https://github.com/dxx-rebirth/dxx-rebirth/issues/302#issuecomment-275816259>
Kreator proposed restoring the Descent 2 cheat that grants homing
capability to all weapons. This commit implements that proposition,
with some changes to the implementation details.
Based-on-patch-by: Chris Taylor <chris@icculus.org>
Requested-by: Chris Taylor <https://github.com/dxx-rebirth/dxx-rebirth/pull/318>
Typing "BALDGUY" after enabling cheats will activate the 'baldguy' easter egg for D1X-Rebirth, which will show next time a briefing is shown featuring Dravis (Mac D1 data only). As with the original, it just plays the 'Cheater!' sound with no HUD message.
Pressing ALT-B when in the briefings using Mac Descent 1 data in D1X-Rebirth works again - showing Dravis wearing a silly hat. Before it would exit load_briefing_screen prematurely.
Function `object_create_explosion_sub` is supposed to be given a weapon
and an object on which to explode it. However, the original game abused
`object_create_explosion_sub` by also calling it from
`object_create_badass_explosion`, which can be called from
`explode_badass_object`, which can be called from
`explode_badass_player`. Chained together, this lets
`object_create_explosion_sub` check the weapon ID of a dying player,
which is not a valid operation. This causes a diagnostic from
`get_weapon_id`. Add an explicit test that the object is a weapon so
that non-weapons do not cause a diagnostic.
Reported-by: ryusei117 <https://github.com/dxx-rebirth/dxx-rebirth/issues/302#issuecomment-272048330>
Analyzed-by: zicodxx <https://github.com/dxx-rebirth/dxx-rebirth/issues/302#issuecomment-272710279>
Commit e36abb25cb fixed one problem with demo access (invalid bits in
the high byte of index values), but created another. That commit
switched from direct loading of `front_wall_num` to instead load into
`type`, then move the value to `front_wall_num`. However, `type` is
`sbyte` (an archaic spelling of `int8_t`), so assignments from `type` to
`front_wall_num` were implemented as a sign-extending move, rather than
a zero-extending move. When the wall number was 0x80 or greater, the
sign-extending move produced an incorrect result, which led to a crash
when valptridx trapped the invalid index.
Fix this by changing the types of all three byte-sized variables to
`uint8_t`. None of them need to be signed.
Reported-by: Dosgamer <http://www.dxx-rebirth.com/frm/index.php/topic,2151.0.html>
Fixes: e36abb25cb ("Fix invalid access reading demos")
`get_weapon_id` must be called only on objects of type `OBJ_WEAPON`.
One path in `multi_compute_kill` could call `get_weapon_id` on an object
that is either a robot or a weapon. The code tested for a robot, but
only after performing an access that assumed it had a weapon. Reorder
the tests to prevent the type mismatch diagnostic.
Reported-by: ryusei117 <https://github.com/dxx-rebirth/dxx-rebirth/issues/302#issuecomment-275546550>
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.
Contains 2 calls - to obj_create and init_ai_object. For safety reasons and tidiness compared to using obj_create directly. The call to init_ai_object in recreate_thief was already redundant.
All releases to date have a bug where they treat certain segment number
fields as an int, not a segment number. Storing segment_none (0xffff)
into the save file causes affected releases to crash in various places
because it fails to recognize that this is segment_none.
Current code correctly treats segment_none as a non-segment and works
correctly without this hack. The hack is only required to get past
releases to work correctly after loading a saved game written by current
code.
All releases to date have a bug where they treat MarkerObject as an int,
not an object number. Storing object_none (0xffff) into the save file
causes affected releases to exhibit several problems. Most obviously,
they crash if the automap is opened, because they try to show
Objects[0xffff] as a marker, but no such object exists. Additionally,
they refuse to let the player drop a marker because none of the elements
of MarkerObject are -1, so they incorrectly tell the player that all
marker slots are busy. Finally, they will crash if the guidebot is
killed, because the guidebot death code deletes marker 0xffff because it
fails to recognize that this is object_none.
Current code correctly treats object_none as a non-object and works
correctly without this hack. The hack is only required to get past
releases to work correctly after loading a saved game written by current
code.
Fixes: 9125ae32cd ("Make objnum unsigned")
All releases to date have a bug where they treat
Dead_controlcen_object_num as an int, not an object number. Storing
object_none (0xffff) into the save file causes affected releases to
crash when treating object_none as a valid object number.
Current code correctly treats object_none as a non-object and works
correctly without this hack. The hack is only required to get past
releases to survive destroying the reactor after loading a saved game
written by current code.
Fixes: 9125ae32cd ("Make objnum unsigned")
Fixes bug where in the editor, you insert a robot with normal behaviour then play the level - it fails the check on the hide_segment index in init_ai_objects(). This is because hide_segment was written as the poisoned value of 0xfdfd if the behaviour of the robot is AIB_NORMAL - because hide_segment wasn't initialised. Therefore, always setting hide_segment to suppress this exception (it's an inexpensive operation seldom called).
zicodxx reported a problem where poison=overwrite builds caused robots
dropped by other robots not to spawn. I diagnosed it as a problem
caused by not setting `dying_start_time` and proposed using
`init_ai_object` to set this (and other) fields. Kreatordxx posted pull
request #311 to implement this for dropped robots. However, his pull
had the unfortunate side effect of zeroing the dropped robot's velocity.
This change is based on his pull, but with the `init_ai_object` call
moved higher so that the velocity is (unnecessarily) zeroed by
`init_ai_object`, then initialized to `new_velocity` by `drop_powerup`
(as it always was).
Reported-by: zicodxx <https://github.com/dxx-rebirth/dxx-rebirth/issues/293>
Patch-inspired-by: kreatordxx <https://github.com/dxx-rebirth/dxx-rebirth/pull/311>
This is where a client in a multiplayer game hitting a wall fails an assert. Initialise obj->mtype.phys_info.flags in multi_reset_player_object() instead of adding flags. (Tested and still works OK without specifying PF_THRUST.)
Initialise ConsoleObject->mtype.phys_info.flags in reset_player_object() instead of adding flags. The only place this was initialised properly was when reading the player object from disk.
Use std::equal_range to find the upper and lower bounds in a single
binary search, rather than relying on a linear search to find the first
sought element.
Also happened when a client to a multiplayer game dropped out due to some network error. Delay call of multi_leave_game() until responding to EVENT_WINDOW_CLOSE, so the game isn't in an unstable state between handling the network event and the game closing.
Commit 4cc801f changed `object_move_one` to return `window_event_result`
instead of `void` and added a default return value at the bottom.
However, it added the value inside a `#if D2` block, so the D1 build now
fails with:
similar/main/object.cpp: In function 'dcx::window_event_result d1x::object_move_one(d1x::vobjptridx_t)':
similar/main/object.cpp:1627:7: error: variable 'result' set but not used [-Werror=unused-but-set-variable]
similar/main/object.cpp:1866:1: error: control reaches end of non-void function [-Werror=return-type]
Move the return statement out of the conditional block to fix both these
errors.
Fixes: 4cc801f42f ("Remove calls to window_close(Game_wind) when game finished or over")
In multi_do_frame(), replace call to window_close(Game_wind) with returning window_event_result::close whenever multi_quit_game is true. Only using this return value where multi_do_frame() is directly called by GameProcessFrame(). multi_quit_game will only be set back to 0 when a new multi game is started.
Closing a window within its handler is problematic - it can result in an unstable state.
Replace call to window_close(Game_wind) with returning window_event_result::close to game_handler. Applies to when there is a failure in net_udp_level_sync(). Closing a window within its handler is problematic - it can result in an unstable state.
Replace call to window_close(Game_wind) with returning window_event_result::close to game_handler. Applies to whenever newdemo_stop_playback() is called. Closing a window within its handler is problematic - it can result in an unstable state.
`struct object_rw` is poisoned prior to initializing and sending it.
However, some fields are legitimately unininitialized (other than their
memset or poison value) at send time. Add and use a poison variant that
can clear those fields, without marking them unreadable.
This is in a redundant check, as we shouldn't (and don't) call newdemo_record_start_frame if nd_record_v_no_space is true, i.e. we ran out of disk space recording a demo. We want to return window_event_result::close with every call to newdemo_stop_playback() to close the game, as this is safer than calling window_close on the game window within its handler. In this case, it's much simpler (and safe) to just remove it.
Even if newdemo_record_start_frame is called when nd_record_v_no_space is true, we don't want to close the game, just exit the function.
Replace calls to window_close(Game_wind) with returning window_event_result::close to game handler. Applies to when DoEndGame() is called, DoGameOver() is called, aborting in the kmatrix screen (multiplayer game) during AdvanceLevel() and playing one demo frame causes playback to stop in GameProcessFrame(). Closing a window within its handler is problematic - it can result in an unstable state.
Old versions of Rebirth sometimes saved games with bogus object data,
such that `obj->type == OBJ_NONE`, but `obj->movement_type`,
`obj->control_type`, or `obj->render_type` were not their respective
*_NONE constants. This confused the game loader code such that it would
load invalid data from the `mtype`, `ctype`, or `rtype` unions and use
it without checking.
Try to counter some of this by exiting early if the object has type
OBJ_NONE.
Valgrind warns when writing uninitialized data to a file. The Descent
savegame format requires writing certain fields that are no longer used.
Set those fields to 0 to prevent leaking stack data into the file.
Levels which fit in MAX_SEGMENTS_ORIGINAL read exactly
MAX_SEGMENTS_ORIGINAL segments worth of light_subtracted, even though
fewer segments were defined. Prepare light_subtracted into a temporary
stack buffer instead of doing byte-at-a-time writes. Initialize this
stack buffer to 0 when necessary. Write the buffer once it is fully
prepared.
player_info::cloak_time is only defined if the player is cloaked.
player_info::invulnerable_time is only defined if the player is
invulnerable.
In both cases, the game save code tried to read the ::*_time variable
even when it was undefined. Modify the saving code to check player
flags before reading the associated timer.
zicodxx reports that f7d0c85 made the thief bot passive and timid.
His analysis suggests that the problem is because f7d0c85 changed
find_connected_distance to return vm_distance::maximum_value() in places
where it previously returned magic values that were not maximum (caching
a distance of F1_0*1000 and returning a distance of -1). Rather than
try to fix the underlying code that relied on these magic values, revert
those return paths to return these unusual values. Move the unusual
values to named constants in file scope so that they are easier to find
and correlate.
Reported-by: zicodxx <https://github.com/dxx-rebirth/dxx-rebirth/issues/286>
Analyzed-by: zicodxx
Fixes: f7d0c853ba ("Use special types for distance/magnitude")
init_player_stats_new_ship used select_primary_weapon and
select_secondary_weapon to assign the player's weapons. However, those
functions read the current weapon and jumped according to its value. A
new ship has no defined value for current weapons, so the jump triggered
an uninitialized value warning from Valgrind.
Add new functions set_primary_weapon, set_secondary_weapon that work
like the previous select_* functions, but always take the path used
for weapons not equal, without checking. This prevents the warnings
from Valgrind, as well as a theoretical risk of initializing the ship
improperly.
An optimization meant to allow change_light to stop early caused it to
stop much too early, so it rarely did any work. Flip the sense of the
test so it does not stop early.
Fixes: 2bd538f353 ("Sort delta light indices")
Demos do not record difficulty level or track hostages correctly. Do
not show these fields in the demo pause dialog, since they reflect
the state of the player's last played game, not the state of the player
recording the demo.
Past releases, when rendering an invalid primary texture, would
Int3() and then reset the texture to zero. Commit d767f7c changed the
logic to return without resetting the texture, since the reset seemed to
be unnecessary. Unfortunately, it is necessary. Some levels, including
those shipped with the retail game data, specify bogus primary textures
on some surfaces. After d767f7c, rendering a surface with an invalid
primary texture causes the surface to be invisible, even if it has a
valid secondary texture.
Remove the return statement added in d767f7c. Extend
validate_segment_side to validate the primary texture on the tested
side. When an invalid texture is found, reset it and log a diagnostic.
For built-in levels, log at level CON_VERBOSE since players cannot
readily fix the level. For external levels, log at level CON_URGENT so
that level authors know to fix their level before releasing it.
Fixes: d767f7cd5e ("Pass vcsegptridx to render_face")
The previous attempt to assign defaults for values that the savegame
lacks was ignored because it overwrote a variable after the last read.
An early draft of that patch worked, but a last minute reorganization to
keep similar purpose code together made the change ineffective.
Narrow the scope of that variable and write to the correct variable.
Fixes: f62ed80205 ("Reset values that should have been in the savegame, but are not")
std::remove_if can be called on an iterator that returns a proxy object,
but the results of that call are not generally useful. When remove_if
attempts to move-assign over removed elements, it instead assigns over
the temporary proxy object. If the proxy object has typical move
semantics, move-assigning over it has no effect on the underlying
container.
Revert to using partial_range on the underlying container.
Reported-by: Mako88 <https://github.com/dxx-rebirth/dxx-rebirth/issues/283>
Fixes: 1f434f98ad ("Use valptridx for ActiveDoors")
When the reactor is forcibly destroyed by the kill goal code, print to
the console and the game log the data that players could otherwise
capture with a screenshot of the network player information.
scores_maybe_add_player might close Game_wind, if it does not return
early. All two callers unconditionally close Game_wind. Remove the
unnecessary closure in scores_maybe_add_player.
