For an argument consisting of a valid path plus whitespace,
do_unit_move() eventually passed the whitespace suffix to parse(),
then dereferenced player->argp[0]. But that was null.
Broken in commit 0c12d837, v4.3.7. Trivial for players to trigger.
When the input line contains only whitespace, player->argp[0] is null
after parse(). This condition was not checked, comtch() returned
M_IGNORE, and code handling that printed the null pointer. Some
systems (GNU, Windows) deal gracefully with printing null strings,
others crash. Trivial to trigger remotely.
Before the fix, the value of parse() was assigned, but never used, and
that was spotted by the Clang Static Analyzer.
If there is no runnable thread, we're deadlocked. lwpReschedule() has
an assertion to catch this. But it didn't work, and the code
following it would crash then. Can't actually happen, because the
EventHandler thread is always runnable. Spotted by the Clang Static
Analyzer.
Local analysis can now easily find out what's up. Before, inter-
procedural analysis was required. The Clang Static Analyzer
complained about a dereference of res that is actually fine.
Local analysis can now easily find out what's up. Before,
whole-program analysis was required. The Clang Static Analyzer
complained about code that is actually fine.
Movement stops when shp_interdict() or lnd_interdict() report
interdiction. However, they reported it only when there was
interdiction damage.
Zero interdiction damage commonly happens when interdicting missiles
miss, or all bombers abort. Stopping regardless of damage makes more
sense there.
Moreover, not stopping is buggy: do_unit_move() needs to take care not
to wipe out updates made by interdiction to the moving ships or land
units. It does so only when it stops. Updates made by interdiction
without interdiction damage could get wiped out, triggering a seqno
mismatch oops.
Known ways moving ships and land units can get updated by interdiction
despite there is no interdiction damage:
* Interdicting bombers get intercepted by planes based on a navigating
carrier, carrier gets charged petrol. The bug wipes out the petrol
use.
* Marching land units get interdicted by planes, but all planes miss.
Sufficiently large collateral damage to the sector can still damage
the land units. The bug wipes out the damage to land units.
To make shp_interdict() and lnd_interdict() report interdiction
regardless of damage, change lnd_missile_interdiction(),
lnd_fort_interdiction(), lnd_mission_interdiction(),
shp_missile_interdiction(), shp_fort_interdiction(),
shp_mission_interdiction() to return whether there was interdiction.
Before, they returned whether there was damage.
Change unit_interdict(), perform_mission(), perform_mission_land(),
perform_mission_ship(), perform_mission_msl(), and
perform_mission_bomb() to return -1 for no interdiction, so that
callers can distinguish no interdiction from interdiction with no
damage.
getland() is obviously redundant in get_dlist(), because the land unit
can't have changed since the previous read.
What's up with ask_olist() is less obvious. For each capable land
unit, ask_olist() asks the player whether to attack with it. If it
attacks, it gets copied into olist. Since asking the player yields
the processor, these copies can become stale. However, re-reading
fixes that just for the last one. Moreover, the code copes with stale
copies just fine: ask_olist() is always directly followed by
att_get_offense() or att_move_in_off(), and both replace the
potentially stale copy in olist.
ask_olist() and get_dlist() called get_land() with llp->chrp still
null, which made it initialize parts of llp instead of performing its
usual integrity check. att_reacting_units() had the initialization
inline. Change the other two to match, and simplify get_land(). No
functional change.
shp_missile_interdiction() does nothing when there are no suitable
targets. Its users avoid to call it when there are none, probably to
save a few cycles. Optimize shp_missile_interdiction() for that case,
and simplify the callers.
Oops when a stale copy is written back, i.e. the processor was yielded
since the copy was made. Such bugs are difficult to spot. Sequence
numbers catch them when they do actual harm (they also catch different
bugs). Generation numbers catch them even when they don't.
New ef_generation to count generations. Call new ef_make_stale() to
increment it whenever the processor may be yielded.
New struct emptypedstr member generation. To conserve space, make it
a bit-field of twelve bits, i.e. generations are only recorded modulo
2^12. Make sure all members of unit empobj_storage share it. It is
only used in copies; its value on disk and in the cache is
meaningless. Copies with generation other than ef_generation are
stale. Stale copies that are a multiple of 2^12 generations old can't
be detected, but that is sufficiently improbable.
Set generation to ef_generation by calling new ef_mark_fresh() when
making copies in ef_read() and ef_blank(). nav_ship() and
fltp_to_list() make copies without going through ef_read(), and
therefore need to call ef_mark_fresh() as well. Also call it in
obj_changed() to make check_sect_ok() & friends freshen their argument
when it is unchanged.
New must_be_fresh() oopses when its argument is stale. Call it in
ef_write() to catch write back of stale copies.
Store them and ef_type in bit-fields.
Allocate eight bits for ef_type. Values range from 0 to EF_GAME
(currently 16), so this is plenty.
Allocate twelve bits for sequence numbers. Sequence number mismatches
are now missed when the numbers are equal modulo 2^12. Still
sufficiently improbable.
Common machines store the bit-fields in a 32 bit word. There are
twelve bits left in that word for future use. Total savings 16 bits,
which is exactly what the previous commit spent on wider uids on
common machines.
Before, they were stored as short. Wider uids use more space, but the
next commit will recover it by narrowing other members.
The use of short has always limited the number of ships, planes, land
units and nukes to SHRT_MAX (commonly 32768). Only the most extreme
games ever came close.
Commit 49780e2c (v4.3.12) added struct sctstr member sct_uid to make
struct empobj member uid work for sectors. This made the limit apply
to sectors as well. We've had games with more than 32768 sectors.
A sequence number mismatch occurs when an invalid copy (with a stale
sequence number) gets written. To continue after the error, we need
to use the current sequence number. But new_seqno() used the larger
one of the two, which is correct only as long as sequence numbers do
not overflow. Overflow is rather unlikely, though.
Member nws_uid is unused since the commit before previous. Remove it.
Member nws_seqno is of marginal value, because we write news only
through ncache[], and thus aren't prone to the errors sequence numbers
can catch. Remove it.
Make timestamp selector virtual, computing nws_when + nws_duration,
and remove member nws_timestamp. Impact:
* In ncache(), the removed timestamp equals nws_when + nws_duration,
both for new news and updated news. No change.
* delete_old_news() becomes invisible. Before, its move of unexpired
news to the beginning of the news file touched all the timestamps.
That was unwanted, because the move does not change news, only their
storage. Improvement.
* empdump no longer flags the imported news changed via the timestamp.
This is somewhat unfortunate. Document as bug.
With these members removed, struct nwsstr no longer matches struct
emptypedstr, so clear news table flag EFF_TYPED and remove union
empobj_storage member news. This loses the automatic maintenance of
member ef_type via struct emptypedstr. Remove ef_type as well.
This shrinks struct nwsstr from 20 to 12 bytes on common 32 bit
machines, and from 32 to 16 bytes on common 64 bit machines. Since
the server doesn't map the whole news file (EFF_MAP is off), this
reduces I/O, while the table's memory use remains the same.
Historical note: struct nwsstr is now pretty much what it was back in
BSD Empire 1.1. Members ef_type and nws_uid go back to Empire 3 (for
C_SYNC?). v4.3.12 added member nws_timestamp, which doubled the size
on common 64 bit machines. v4.3.15 added nws_seqno.
struct lndstr member lnd_flags is a leftover from Empire3's C_SYNC,
which was ripped out in 4.0.0.
struct lonstr member l_sel, struct nuk_str members nuk_ship, nuk_land,
and struct trtstr member trt_bond have been there basically forever
without any use.
Launching ABMs updates their base, in mission_pln_equip(). The update
got wiped out when launch_missile() wrote back the target sector,
triggering a seqno mismatch oops. Harmless with the stock game's
ABMs, because they use neither shells nor petrol.
Fix by re-reading the target sector.
Ships can expend shells to defend against missiles, in
shp_missile_defense(). Any shell use by the target ship got wiped out
when launch_missile() wrote back the target ship, triggering a seqno
mismatch oops.
Fix by re-reading the target ship.
