Testing for `object_none` is insufficient. Games saved before the fix
for 14cdf1b352 may have a `last_hitobj` of
`0x1ff`, which triggers an exception when constructing `icobjidx_t`.
Treat any invalid object index as `object_none`.
Reported-by: Quix0r <https://github.com/dxx-rebirth/dxx-rebirth/issues/716>
The index of the guidebot is set by loading the level data, and this is
usually sufficient. However, if the user kills the guidebot, then uses
cheats to create a new one, the new guidebot will often have a different
index than the original guidebot. If such a game is saved and then
reloaded, then after the reload, the computed guidebot index will be
reverted to the index of the original dead guidebot. This causes
various problems, including potentially a crash. Recompute the guidebot
index after loading the objects from the save file, so that it matches
the live guidebot.
Reported-by: GitInMotion <https://github.com/dxx-rebirth/dxx-rebirth/issues/713>
Various code assumes that Buddy_objnum maintains the invariants:
- If the guidebot exists, Buddy_objnum must refer to it.
- If no guidebot exists, Buddy_objnum must be object_none.
This was not enforced, so if the guidebot is killed, Buddy_objnum can
continue to refer to its last index. That can cause spurious errors
later. For example, when the player enters an energy center, if the
guidebot is dead and the guidebot's last goal was "Find energy center",
then the console reports:
```
BUG: buddy is object 28, but that object is type 255.
```
Fix that by clearing the guidebot index when the guidebot is killed.
Prefer non-static data member initializers over leaving the member
undefined in the constructor and relying on the caller to zero the
member afterward.
The pointer is only used by the constructor, so there is no need to
store it in the object.
Rework the allocation of the trailing storage to avoid the use of
placement new on a uint8_t[].
Pass a mutable buffer from the caller, and allow
PHYSFSX_addRelToSearchPath to adjust the capitalization in that buffer,
rather than creating a copy in a stack local. This can slightly affect
status messages that use the name, but otherwise should have no effect
on the game.
Store an owned pointer in MVEFILE instead of storing an unowned pointer
there and requiring the caller to keep the owned pointer alive for the
lifetime of the MVEFILE.
Due to original game bugs, which Rebirth intentionally replicates in
order to match the balance of the original game, the logic looks odd.
Add comments explaining that the oddity is intentional.
clang-15 warns for a write-only parameter. The parameter is only used
in a debug print, which has been commented out since the code was
originally imported. Remove the parameter and the commented print.
gcc-12 can issue `-Warray-bounds` warnings for code inlined from system
headers, and warns about `std::sort()` on a small array (
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107986> split from
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104165>). Per a comment
in a related Fedora report
<https://bugzilla.redhat.com/show_bug.cgi?id=2051783#c3>, and local
testing, adding an explicit unreachable on the impossible case that the
end iterator exceeds the end of the array will discourage gcc from
warning without needing to disable -Warray-bounds.
This failure was previously observed in early testing with gcc-12, and
was set aside in the hope that gcc-12 would be fixed before it came into
widespread use. That has not happened, and JoeNotCharles reported
<2644e0cb93>
this independently. JoeNotCharles proposed disabling the warning for
the call to `std::sort`. This commit instead leaves the warning
enabled, but encourages gcc to see that the bad path is impossible,
allowing gcc to delete the bad path before the warning is issued.
JoeNotCharles reports that gcc-12 (architecture unspecified) warns
because `newmenu::process_until_closed` stores the address of a stack
local into a heap-allocated structure. Switch to a less efficient
implementation that does not provoke a compiler warning:
- Store `shared_ptr<int>`, not `int *`, in the `newmenu`
- `shared_ptr::operator*()` and `shared_ptr::operator bool()` allow most
use sites to be unchanged relative to a bare pointer
- Load the return value from the `shared_ptr<int>`. If the newmenu
terminated before return, as should always happen, this is a slightly
less efficient version of the same code as before. If the newmenu was
still open despite its `exists` flag claiming otherwise, then the
returned value may be incorrect, but the read will be well-formed, the
`newmenu`'s eventual write will write to the heap-backed int (rather
than writing into the stack allocated to
`newmenu::process_until_closed`), and the memory will be freed when
both `process_until_closed` has returned and the `newmenu` has been
destroyed.
Reported-by: JoeNotCharles <10a2b2d337>
gcc-12 does enough escape analysis to notice that
newmenu::process_until_closed stores the address of a stack local into a
heap-allocated structure, but not enough analysis to notice that the
stack variable always outlives the escaped address. The compiler then
warns about the address escaping the local scope. Reworking the calling
code not to do this is somewhat invasive, and gcc seems unlikely to
change behavior. Switch to a less efficient implementation that does
not provoke a compiler warning:
- Store a shared_ptr<bool> in the object
- In the object's destructor, write through the pointer to clear the
shared boolean
- In the caller, store a copy of the shared_ptr<bool> as a local, and
use that copy to monitor the shared boolean
This is similar to a change proposed by JoeNotCharles
<10a2b2d337>,
but differs in its details. Among other things, this version takes the
opportunity to move the variable out to a mixin, so that only windows
which expect to be tracked can be tracked. Previously, all windows were
capable of this, even though most never needed it.
JoeNotCharles reported
<1f1903a3b9>
an overflow in `net_udp_send_mdata_direct`. This overflow is impossible
as currently written, because it can occur only if
`multi_send_data_direct` passes an oversized buffer to
`net_udp_send_mdata_direct`, and no messages are large enough to trigger
this. JoeNotCharles proposed adding a runtime check to abort the
program if this happens. Instead, this commit adds a compile-time check
to detect use of an excessively large input buffer.
JoeNotCharles reports that gcc-12 (architecture unspecified) warns about
a possible truncation when constructing a path. This would only occur
if the user had a path that was almost PATH_MAX deep (4096 bytes on most
platforms). Add safety checks around this, and around an underflow if
this path were somehow reached while `newpath` was shorter than `sep`.
This is loosely based on a proposed commit by JoeNotCharles, but
rewritten to update comments and adjust the code flow.
Reported-by: JoeNotCharles <edfb3c4b77>
Construct the nm_messagebox_tie directly, without use of a macro. This
produces simpler compiler error messages when nm_messagebox is called
incorrectly.
Per SDL documentation, SDL will generate an SDL_QUIT event when
Command-Q is used. Rebirth already handles SDL_QUIT, so there is no
need to explicitly detect Command-Q.
