do_unit_move() reads the ships into a list. It re-reads them when it
prompts for sub-commands. shp_nav_one_sector() writes them back when
it moves ships.
Mine-laying (sub-command 'd') updates the minelayer, invalidating the
copy in the list. Any movement sub-command before the next prompt for
sub-commands wiped out this update, triggering a seno mismatch oops.
Happens only if 'd' is used without arguments, because remaining
sub-commands are discarded when there are arguments.
Broken when mine-laying was added in commits 2438fe7c, v4.3.7.
Same for march, commit 274c8e42, v4.3.7.
Fix by stopping after 'd' regardless of arguments.
When sub-command 'd' was used without arguments, do_unit_move() failed
to supply the second argument to mine(), which duly prompted for it.
This contracticted info, and could trigger a generation oops.
do_unit_move() reads the ships into a list. It re-reads them when it
prompts for sub-commands. shp_nav_one_sector() writes them back when
it moves ships.
The mine prompt made the list stale. Movement sub-commands before the
next prompt for sub-commands wrote back stale ships, triggering a
generation oops. Example: "nav 15 dg".
Broken when mine-laying was added in commits 2438fe7c, v4.3.7.
Same for march, commit 274c8e42, v4.3.7.
Commit a269cdd7 (v4.3.23) removed the nuclear damage. But it left the
nuke on the missile, which made pln_damage() oops and return zero
damage.
Fix by destroying the nuke separately.
Maps are generally drawn into static scratch buffers. Each command
has its own buffers.
Static scratch buffers are safe as long as they're never used across
yields. Player output can yield unless the command has flag C_MOD
set. Commands lradar, path, radar, route, satellite, sect, survey
hadn't. If such a command yields while using scratch buffers, another
instance of the command can clobber them.
Abuse seems tricky, but possible: if a malicious player stalls output
just right, a command yields while printing a map from the scratch
buffer. It resumes only when the malicious player reads some output.
If another player runs the same command before that, it overwrites the
same static scratch buffer with its map. The malicious player
receives the last such run's map.
4.2.8 fixed the same bug for bmap, lbmap, lmap, map, nmap, pbmap,
pmap, sbmap and smap.
All were broken in 4.2.0. Except radar maps (lradar and radar) were
already broken in Empire 2 for AIX.
fgets() fails on read error or EOF. When it fails, it doesn't touch
the buffer. Since files didn't check its return value, it read an
uninitialized character from the buffer. If that happened to be 'y'
or 'Y', it happily clobbered the game.
Many commands, compiler invocations in particular, are rather long,
and warnings tend to get lost in the noise. Suppress the command
details. Run make with a V=1 parameter for full output.
orde() uses a "clever" trick to get away with just one
check_ship_ok(): it makes a copy of the ship in the beginning, and
checks it at the end. Generation numbers (commit 2fa5f652) break this
trick. It could be fixed by manually marking the ship fresh.
Instead, check the ship the stupid, straightforward way, without a
copy.
The call to WSAEnumNetworkEvents() was reseting network
events in particular WSAECONNRESET was sometimes being lost
causing the client to not exit. The WSAEnumNetworkEvents()
was called from fd_is_socket() and has been replace with
getsockopt() to determine if the fd is socket or not.
Broken in commit 4d40a275, v4.3.23
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.
