navigate march: Fix abort not to wipe out concurrent updates
When the player aborts the command at the movement prompt, we write
back stale ships or land units, triggering a generation oops. Any
updates made by other threads meanwhile are wiped out, triggering a
seqno mismatch oops.
Broken in commit 24000b4, v4.3.33. Fix by restoring the lost
shp_nav_stay_behind() and lnd_mar_stay_behind() calls.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
march: Fix concurrent updates at sector abandon prompt
When the player declines to abandon a sector, we write back stale land
units, triggering a generation oops. Any updates made by other
threads meanwhile are wiped out, triggering a seqno mismatch oops.
The culprit is lnd_abandon_askyn(): when the player declines, it
returns without calling check_sect_ok(), check_land_ok(). Broken in
commit 7c1b166, v4.3.33. Fix it.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
recvclient() calls ef_make_stale() only when it does actual I/O, via
io_output() and io_input(). Missed in commit 2fa5f652, v4.3.24. Call
it directly when it doesn't do actual I/O.
This makes navi-march-test expose a bug in march: when the player
declines to abandon a sector, we write back stale land units,
triggering a generation oops.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
configure: Use -fstack-protector-strong when available
Testing whether the compiler supports it is a bit tricky.
The obvious AX_APPEND_COMPILE_FLAGS([-fstack-protector-strong])
doesn't suffice, since some ports of the GNU toolchain reportedly pass
this test, then fail to link. That's because the compiler accepts the
flag, duly emits references to helper code in libc, but libc doesn't
provide, and linking fails.
Instead, use AX_APPEND_LINK_FLAGS with an input source that makes the
compiler emit the extra stack checking code. This requires the latest
version from the autoconf-archive, so update m4/ax* to commit e3d948b.
Also update m4/my_append_compile_flags.m4 to keep it in sync with
upstream's ax_append_compile_flags.m4.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
configure: Use -fno-strict-aliasing -fno-strict-overflow
Contemporary compilers can squeeze out some extra performance by
assuming the program never executes code that has undefined behavior
according to the C standard. Unfortunately, this can break programs.
Pointing out that these programs are non-conforming is as correct as
it is unhelpful, at least as long as the compiler is unable to
diagnose the non-conformingness.
Since keeping our programs working is a lot more important to us than
running them as fast as possible, forbid some assumptions that are
known to break real-world programs:
* Aliasing: perfectly clean programs don't engage in type-punning, and
perfectly conforming programs do it only in full accordance with the
standard's (subtle!) aliasing rules. Neither kind of perfection is
realistic for us, therefore -fno-strict-aliasing.
* Signed integer overflow: perfectly clean programs won't ever do
signed integer arithmetic that overflows. This is an imperfect
program, therefore -fno-strict-overflow.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
tests: Enable GNU libc memory allocation error checking
MALLOC_CHECK_=3 makes glibc check for memory allocation programming
errors. It's the factory default, but set it anyway just in case
someone disabled it for speed.
Non-zero MALLOC_PERTURB_ makes glibc wipe memory value on allocation
and deallocation. The actual value determines the bit pattern. Set
it to the value of environment variable EMPIRE_CHECK_MALLOC_PERTURB or
else a pseudo-random number, and record it in sandbox/malloc-perturb.
See mallopt(3) for more information.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
When the player aborts the command at the movement prompt, or declines
to abandon a sector, unit_move() returns without freeing the list.
Found with valgrind. Broken in commit 24000b4 and commit 7c1b166,
both v4.3.33.
Free the list on these returns, too.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
navigate march retreat lretreat: Fix read beyond buffer
shp_nav_gauntlet() and lnd_mar_gauntlet() read beyond the list head
when the list is empty. The values read aren't used then. Could
conceivably crash the server anyway, but it's unlikely.
Empty list happens when shp_nav_dir(), lnd_mar_dir() empty the list
and return zero. Broken in commit beedf8d, v4.3.33. Occurs in
navi-march-test (since the last commit) and in retreat-test.
Change shp_nav_dir() and lnd_mar_dir() to return one then. For
additional safety, make shp_nav_gauntlet() and lnd_mar_gauntlet() oops
on empty list and recover safely.
I think I originally found this bug with -fsanitize, but I've since
upgraded, and I can't diagnose it that way anymore.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
bomb drop fly paradrop recon sweep: Fix read before array
The code computing the length of the flight path checks whether the
path ends with 'h'. When getpath() returns an empty path, it accesses
flightpath[-1]. This could set the length to -1 (unlikely), or crash
(even less likely). The former could be abused to gain mobility for
sufficiently inefficient or short-ranged planes. Found with valgrind.
Historically, getpath() could return paths with or without 'h', and
the check was necessary. It returned an empty path only when the
player gave no input, aborting the command. When the player entered
the assembly point's coordinates, it returned "h".
Commit 404a76f7 accidentally changed it to return "" then. Also broke
flying to the assembly point's coordinates. Commit 0f1e14f (v4.3.31)
fixed that part by changing getpath()'s contract: always return paths
without 'h' ("" simply means empty path), and return NULL on invalid
input, including no input.
The flawed check is superfluous since then. Drop it.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Provide proper build-time assertions for NSC_SITYPE()
We want to cause a diagnostic when NSC_SITYPE()'s argument isn't
implemented. Commit aa6ad9d's solution is to have the macro expand
into 1/0 then. Works with GCC, but Clang always warns "division by
zero is undefined".
The better, portable way to conditionally break the build is an array
type with a size that's negative when the build should fail, else
positive. Implement that wrapped in a sizeof() to make it an
expression as macro BUILD_ASSERT_ONE(), and use it in NSC_SITYPE().
No more warnings from Clang 3.5.0. GCC still produces its "may be
used uninitialized" false positives.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
subs: Don't squash telegrams together when time goes backwards
We've always squashed them when the time difference is smaller than
TEL_SECONDS, regardless of sign. This involves passing the difference
to abs(), implicitly casting from time_t to int, which triggers a
Clang warning.
I could clean this up to get rid of the warning, but time should never
go backwards, and trying to make things prettier when it does isn't
worthwhile. Simply drop the abs().
While there, drop the function comment. It's been inaccurate since
Empire 3 dropped mail.c, and bogus since commit 17223e8 (v4.3.29)
added tel_cont.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
info: Belatedly update for change of stop prefix to '!'
Commit eb1512d (v4.3.6) added the '=' if stopped before efficiency.
Commit 016249c (v4.3.6) changed it to '!' without updating info ship,
plane, land, nuke.
Reported-by: Harald Katzer Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
... when referring to a function's parameter or a struct/union's
member.
The idea of using FOO comes from the GNU coding standards:
The comment on a function is much clearer if you use the argument
names to speak about the argument values. The variable name
itself should be lower case, but write it in upper case when you
are speaking about the value rather than the variable itself.
Thus, "the inode number NODE_NUM" rather than "an inode".
Upcasing names is problematic for a case-sensitive language like C,
because it can create ambiguity. Moreover, it's too much shouting for
my taste.
GTK-Doc's convention to prefix the identifier with @ makes references
to variables stand out nicely. The rest of the GTK-Doc conventions
make no sense for us, however.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
When AC_PROG_CC detects GCC, Make.mk adds a bunch of flags to CFLAGS.
Works only for flags that any version of gcc in use accepts.
Instead, make configure add the flags that actually work to CFLAGS.
This will let us add flags that work only for some compilers.
The new autoconf macros are from autoconf-archive v2015.02.24.
Unfortunately, AX_APPEND_COMPILE_FLAGS doesn't work reliably for
-Wno-*: gcc complains about unknown -Wno-foo only when other
diagnostics are being produced. Test -Wfoo instead of -Wno-foo, and
rename to MY_APPEND_COMPILE_FLAGS.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Breaks retreat after ship got sunk by bombs or missile.
ship_bomb() and launch_missile() pass .shp_own to retreat_ship().
Wrong after putship(), because putship() resets the owner when the
ship got sunk. retreat_ship() then oopses and fails to retreat the
surviving members of the group.
Other callers save the owner before putting the ship, and pass that.
We could change these two to do the same. But since we're trying to
get a release out, simply revert the broken commit instead.
The __UNCONST() stolen from NetBSD assumes unsigned long can hold a
pointer. Not true with Win64's LLP64 data model. There, we cast the
64 bit pointer to 32 bits and back. Works only because Windows puts
the stack at a very low address, and the casts don't actually change
the pointer.
Dumb it down to a straight cast to void * for safety.
Thanks to Harald Katzer and Ron Koenderink for their help figuring out
the bug's impact.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Redirections and the execute command let the user read and write files
and run programs on the local system.
Restricted mode prevents such access. This is useful when you want to
grant somebody access to just Empire, but not to the host system's
user account that runs the client.
Signed-off-by: Marisa Giancarla <fstltna@me.com> Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
build: Fix inexact calculation of required materials
sector_can_build() computes mat[i] * (effic / 100.0). The division is
inexact. The result gets randomly rounded, so errors are vanishingly
unlikely to screw up material consumption.
However, we require the amount rounded up to be present since commit 1227d2c. Errors *can* screw that up. Fix by avoiding inexact
computation for that part.
We should probably review rounding of inexact values in general.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
tests: Fix for builds outside git-controlled source tree
We run "git ls-files" in the build tree. Doesn't work when the source
directory isn't a git repository, or the build directory is outside
the source directory. Broken in commit 71cb2d8.
Find source files like Make.mk does: if the source tree is a git
repository, use git ls-files, else use sources.mk.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Deprecated in commit a00f9e2: 'r' with flags, and bad flags after 't'.
Affects flags argument of bmap, sbmap, pbmap, lbmap, nbmap, and
navigate and march sub-command 'B'.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
emp_config() silently truncates WORLD_X to even. Drop that. We could
flag odd WORLD_X as error, but we don't validate the other
configuration values, so why this one? Instead document it needs to
be even. WORLD_Y, too.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
torpedo: Let torpedo hit land only when target is in range
Telling the player his torpedo "slams into land" can give a clue on
the direction to the target. No good when the target is out of range,
because we shouldn't tell the player more than that then.
Screwed up in 4.2.2. Fix by checking range before line of sight.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
info/torpedo: Fix misinformation on submarine identification
Claims the victim of a torpedo attack gets told the attacking ship's
number. This hasn't been the case for submarines since Empire 2.3.
Recent commits again reveal the attacking submarine's number, but only
when it gets hit by return fire. Update info accordingly.
Reported-by: Neeraj Jain <thisisfranz@gmail.com> Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
torpedo fire: Reveal sub hit by return fire or depth charge
This partly reverts a change made in Empire 2.3 to tell a submarine's
opponent only that he's dealing with a "sub" instead of the
submarine's UID and type. Hiding submarines is done by prsub().
Uses:
* Command torpedo: defender depth charges or torpedoes an attacking
submarine
If you can attack a submarine reactively, you should be able to
attack it actively, too. But that requires its UID. Reveal it
again, but keep the type hidden.
* Command fire: defender fires back at a submarine using its deck gun
Submarines need to surface to fire deck guns, so they shouldn't be
treated any different than surface ships. Revert Empire 2.3's
change entirely there, i.e. defender learns type as well as UID.
* Command torpedo: attacking submarine hits its target
Keep the submarine hidden.
* Commands torpedo and fire: attacking ship hits a submarine
The attacker passed the UID as command argument, so it doesn't
matter whether we print it or not. Printing it is simpler to code,
so do that.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
fire: Clean up damage sanity check and printing of range
Repeated for ship, sector and land unit firing. The latter prints
range only when the sanity check succeeds.
Factor out, changing ship and sector to behave like land unit firing.
When the sanity check fails, print "Jammed!" instead of "Klick!",
because "Klick!" suggests no shells. Used to be printed exactly then,
but the condition first became impossible (Chainsaw), then generalized
to "can't fire for whatever reason" (commit 22c6fd8, v4.3.12).
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
The fire command drops depth charges when the target is a submarine in
range and firing ship has the capability. Else, it blindly fires
guns. It used to reject ships that can't use guns, even when they
could use depth charges, but commit 9b0b0dc (v4.3.31) lifted that
restruction. No such ships exist in the stock game.
If the firing ship can't fire guns, shp_fire() returns -1, triggering
an oops. Broken in commit 0757042.
Avoiding dependence of depth charge on gun fire capability is
pleasing, but nevertheless a bad idea without test coverage. Creating
the necessary tests isn't worth it, so put back the traditional
restriction instead.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Mission is cleared only when firing at a target that is out of range.
Screwed up when missions were added in Chainsaw. Always clear it when
firing. Matches torpedo.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
fire: Fix artillery splashing the bridge span under itself
multifire() writes back the firing sector after applying damage. When
an artillery unit on a bridge span commits suicide by shelling down
the supporting bridge heads, this writeback puts the bridge span right
back (less the land units and planes on it), triggering a seqno oops.
On the next update of the bridge head, the bridge span falls again.
Broken in commit fe5b266, v4.3.14.
The problematic write back is superfluous. Remove it along with a few
equally superfluous ones.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
convert: Drop broken code to charge security unit mobility
Conversion is easier when land units with capability security are
present. Each such land unit is charged 10 mobility. The mobility
charge is undocumented.
Land unit mobility is charged even when conversion turns out to be
impossible, say because the sector has no mobility. I call this a
bug. Has been that way since security land units were added in
Chainsaw 3.
Except the mobility charge doesn't actually work anymore: the changed
land unit is never written back. Broken in commit 82c9166, v4.3.16.
Fix this bug would be trivial, but would bring back the bug described
above, and fixing that one is harder, and doesn't feel worthwhile.
Remove the broken charging of land unit mobility instead.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
A bombed plane's mobility is multiplied by dam/100.0, i.e. the higher
the damage, the lower the mobility loss. Has always been broken. Fix
by computing the new mobility with damage(), like we do elsewhere.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
commands: Always put ship or land unit before retreating it
boar() puts before retreating, the other callers afterwards. Subtle
difference, because putting resets the owner of the dead to POGO.
Until the commit before previous, retreat didn't fully work after put.
Now it does. The subtle difference between boar() and the other
callers still exists. It's better to do it the same everywhere, as
subtle differences invite bugs. Since changing boar() is not
practical, change the others.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
The old retreat_ship() took care not to put its ship argument (it
still put other ships in a group retreat). Callers put it
unconditionally to make the change to the ship permanent.
The current retreat code puts all ships it changes, rendering sona()'s
putship() redundant. Drop it.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
retreat: Fix group retreat after failed board sinks ship
Group retreat still doesn't work, because when boar() passes a sunk
ship to retreat_ship(), its owner has been reset to POGO already.
This makes it impossible to find the group to retreat. Instead, it
attempts to retreat ships that sank in the same sector with group
retreat orders and with the same fleet letter assigned. If any exist,
shp_may_nav() oopses, and prevents actual retreat of these ghosts.
The other retreat conditions don't have this problem, because they
call putship(), which resets the owner, only after retreat_ship().
Making boar() work the same is not practical. Instead, add an owner
parameter to retreat_ship(), and for symmetry also to retreat_land().
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
board: Don't retreat ship#0 after failed board sinks ship
The root cause is in put_combat(): after it sinks the ship, it calls
att_get_combat(), which treats a combat object with a dead ship as an
error, tells the attacker "not in the same sector!", and "recovers" by
putting the combat object into an error state. Too hard for me to fix
right now, so put in a FIXME comment.
The error state trips up retreat. boar() uses the victim's ship
number in the combat object to find the ship it may have to retreat.
Putting the combat object into an error state sets this number to
zero. If that ship exists, and isn't owned by the attacker, and has
RET_BOARDED set, it retreats. Oops. Broken when Empire 2 factored
out common combat code.
Fix by saving the ship number while it's still valid.
This uncovers the next bug: we now pass a dead ship to retreat_ship().
Oopses since commit f743f37. Its commit message says "Harmless, but
avoid it anyway." Going to revert.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
projects / empserver / log