The output queue flush can block indefinitely. Permits a client to
hog the thread indefinitely by not reading output.
Broken in commit 08b94556 (v4.3.20) "Reimplement max_idle without a
separate thread". Until then, the idle thread aborted a stuck attempt
to flush output.
Denial of service seems possible.
recvclient() flushes the output queue before receiving input. The
receive obeys max_idle, the flush doesn't.
Broken in commit 08b94556 (v4.3.20) "Reimplement max_idle without a
separate thread". Until then, the idle thread aborted a stuck attempt
to flush output.
We must not block in io_input() after command abortion unblocked
io_output(). Instead of checking player->aborted, compute the
deadline according to player->may_sleep, like we do for io_output().
player_login() flushes the output queue before receiving input. The
receive obeys max_idle, the flush doesn't. Which means a client could
hog the thread indefinitely.
Broken in commit 08b94556 (v4.3.20) "Reimplement max_idle without a
separate thread". Until then, the idle thread aborted a stuck attempt
to flush output.
Denial of service seems possible.
A client can delay thread exit indefinitely by not reading output.
Broken in commit 08b94556 (v4.3.20) "Reimplement max_idle without a
separate thread". Until then, the idle thread aborted a stuck attempt
to flush output.
Denial of service seems possible.
Note that commit 904822e3 moved flushing the output queue from
player_login() to io_close(). It also made io_close() wait for the
client to close the connection. That wait obeys the deadline.
The nightly build uses a gross hack to keep timestamps stable: it
replaces the system's time() by emp_time(). It doesn't replace other
time-related functions such as gettimeofday(). Works as long as we
don't mix hacked time with unhacked time.
The previous commit compares time gotten from gettimeofday() with time
gotten from time(). The nightly build's I/O timeouts become zero,
which makes login impossible.
Replace gettimeofday(), too.
Idle timeout used to expire max_idle minutes after the last
player->curup update. When we got rid of the idle thread in commit
08b94556 (v4.3.20), this got changed to "wait no more than max_idle
minutes for input". Time spent computing and time spent blocked on
output no longer counts. In particular, a connection can block
indefinitely on output since then. Let's fix that.
Start with basing the input timeout on player->curup again. The
missing output timeout will be added shortly.
Aside: since status() updates player->curup, the idle timer gets reset
when the update aborts a command. Left for another day.
Commit 08b94556 (v4.3.20) added io_open() parameter input_timeout. It
applies to io_input() and, since commit 904822e3, to io_close(). Add
timeout parameters to these functions instead.
Return zero when no input is available, regardless of parameter
waitforinput. Before, it returned -1 with errno set to EAGAIN or
EWOULDBLOCK when not waiting for input. Current callers all wait.
Drop errno from the function's contract, for consistency with
io_output().
Remote hole, can smash the stack. Additionally, the confirmation
prompt is misleading when the player supplies conditionals. Redesign
the flawed prompt.
Broken when Chainsaw added the confirmation prompt. Reported by Scott
C. Zielinski.
If execute's argument was read interactively, it prints the argument
instead of the command. Else, it prints uninitialized garbage. The
latter can theoretically crash the server, or leak information.
Broken in commit 3de1e8be, v4.3.28
lnd_take_casualty() uses uninitialized rsect to compute the mobility
cost of retreating a defending land unit. This can charge incorrect
mobility, prevent retreat, or, if the stars align just right, crash
the server when sector_mcost() subscripts dchr[] with it.
Broken in commit 4e7c993a, v4.3.6. Reported by Scott C. Zielinski.
play_cmd() needs to return in a state that makes player_login() break
the login command loop.
play_cmd() assumes player_main() always returns in such a state:
connection's EOF indicator set. Unfortunately, the assumption is
wrong. Fortunately, play_cmd() checks it, oopses and recovers.
player_main() can return with neiter error nor EOF indicator set,
e.g. when the game is down.
player_main() can return with just the error indicator set. For
instance, when the client dies, io_input() detects ECONNRESET, sets
the error indicator and fails.
Broken in commit 8549efbc. Fix by setting the EOF indicator silently
(without oopsing) when necessary.
C_MOD prevents print functions from blocking. The common reason for
wanting non-blocking prints are "read, print, write back" patterns.
There might be other reasons lurking in the code, and that's why
messing with C_MOD is scary.
Nevertheless, drop it from cutoff, headlines, land, lstat, motd,
neweff, payoff, qorder, sorder, and wall. These commands have had
C_MOD "forever", even though they clearly don't modify game state.
They're all pretty simple. Similar commands such as census, news,
ship, plane, nuke, sstat, pstat, flash never had C_MOD.
arm and disarm no longer require money, for consistency with the other
commands to move stuff around.
satellite no longer requires money, for consistency with lookout,
radar, sonar and skywatch.
qorder, sorder, survey and test no longer require a capital, for
consistency with the other commands that report on stuff the player
owns.
start and stop no longer require a capital, for consistency with the
other commands to control production.
fortify now requires a capital, for consistency with mission, morale
and range.
Create new command capability NONVIS. Give it to players in any state
except visitors (and STAT_UNUSED, but those must not exist). This
makes it possible to have commands available to anyone but visitors.
Command change fails when the player is a visitor. Simply make it
unavailable instead, by requiring NONVIS.
Make read unavailable to visitors, because it's useless: visitors
can't receive telegrams (typed_wu() fails).
Make census, commodity and sinfra unavailable to visitors. Visitors
don't normally have sectors.
Make map and nmap unavailable to visitors. Visitors don't have sectors,
so their maps are always empty.
Make them unavailable to new players (between add and newcap) and
players in sanctuary, too. This is consistent with all the other
commands to examine the environment. It also prevents people from
trying multiple unbroken countries in a blitz to find the one with the
nicest vicinity.
Make resource available to new players, for consistency with census
and commodity.
Make country, echo and financial available to anyone.
A player may execute a command when his player->nstat has all the bits
in the command's c_permit.
Normal player commands require bit(2). Command break requires bit(1),
and execute requires bit(5). Deity commands require both bit(2) and
bit(3). Works, because deities always have both bits set in nstat, as
they may execute normal player commands, too. But it's a bit
confusing. Change them to only require their own bit(3).
A player may execute a command when his player->nstat has all the bits
in the command's c_permit.
All commands require bit(0). All players always have it. Silly; get
rid of it.
Don't say selected sectors "show up in reverse video", that depends on
the client. Just say "will be highlighted", like we do for the other
commands that highlight, e.g. info survey.
rea() loops if more telegrams arrive while we wait for the player to
confirm deletion. If the first new one is a continuation of the last
old one, its header is suppressed. Don't do that.
Broken in commit 17223e8f, v4.3.29.
Two related bugs:
* It moans about deprecated argument syntax ('m' without a space
before its argument) even when there's no argument.
* It uses the third instead of second argument for map flags (second
argument is ignored): "m# s" doesn't show ships, and "m# s p" shows
planes instead of ships.
Broken in commit 28d48474, v4.3.27.
The market command executes all trades that have become ready, by
calling check_market() and check_trade(). This modifies game state,
but market lacks C_MOD. That's wrong. But can it do harm?
Turns out yes. check_trade() looks safe, but check_market() telexes
seller and buyer while holding a copy of the commodity struct. If
this telexes the player who sent the market command, and he has
NF_INFORM on, the telegram notification may yield the processor. It
then writes back its copy, triggering a generation oops. Any updates
made by other threads meanwhile are wiped out, triggering a seqno
mismatch oops.
This can cause commodity trades to be executed multiple times,
multiplying the sold commodities. Abuse seems tricky, but possible:
conspiring trade partners trade commodities back and forth to multiply
them. One of them needs to get the output backlog just right to make
the telegram notification yield, and the timing right so that the
MarketUpdate thread or another player's buy, market, reset, sell, set
or trade command runs check_market() before his thread resumes.
Closely related bug: visitors can trigger execution of trades by means
of command market. That's clearly inappropriate.
Broken in Empire 3. Reported by Scott C. Zielinski.
When a ship, plane, land unit or nuke is sold, the seller is replaced
by POGO: POGO gets the money, the telegrams and makes the news.
Likewise when a sale fails because the buyer can't pay.
Broken in commit 94a3108b, v4.3.17. Reported by Scott C. Zielinski.
Commit 1e1dfc86 (v4.3.23) attempted to do this, but it's flawed.
Server shutdown makes the player command loops terminate. Each player
thread then flushes buffered output and closes the server's end of the
connection. The client eventually gets EOF, and closes its end of the
connection. All is well.
However, if the client sends more input after the server closed its
end of the connection, but before it completed receiving server
output, it gets ECONNRESET, and the remaining output is lost.
Instead of closing the server's end of the connection, we need to shut
down its transmission direction, then wait for the client to close its
end, by receiving client input until EOF. Do that in io_close().
The output flushing in player_login() is now superfluous. Remove it.
Make shutdwn() wait for the io_close() to complete instead of output
queues to drain. Without that, we could still close the server's end
of the connection prematurely, through program termination. Change
player_delete() to keep the player in Players until after io_close()
completes, so that shutdwn() can detect completion.
Simply set the player connection's EOF indicator. Cleaner than
setting player->state to PS_SHUTDOWN from random places.
Move the assignment of PS_PLAYING from player_main() to its caller
play_cmd(), so that player->state is exclusively controlled in
login.c.
quit_cmd() calls io_shutdown() to make player_login()'s next command
read detect EOF. io_shutdown() drains the input queue and shuts down
the socket with shutdown(). player_login()'s next io_gets() fails all
right, but then io_input() *can* read more from the socket on my Linux
box, at least when I send plenty of input fast. Thus, we ignore
whatever input after quit was already queued, then resume reading
commands, not necessarily at the beginning of a line.
Fix by setting the EOF indicator instead.
Timeout during execute gets handled just like an EOF cookie: end the
batch file, resume reading normal commands. That's wrong, we need to
close the connection.
A real EOF is recorded in the player's connection's EOF indicator.
Let's use that for all "connection needs to be closed" conditions, so
they all work the same. Create io_set_eof() to provide access.
Make recvclient() set the player connection's EOF indicator on
timeout. This makes the timeout "stick". Record receipt of an EOF
cookie in new struct player member got_ctld. Also abort the command,
as before. This leaves further interpretation of the EOF cookie to
the command loops.
Make player_main() set the player connection's EOF indicator on
got_ctld. Player connection gets closed on on EOF cookie, as before.
Change execute() to break the batch command loop when got_ctld is set,
then reset it. Ends the batch file on EOF cookie, as before.
Change status() back to checking EOF and error indicators (partial
revert of commit 9c5854c8, v4.3.16), and drop struct player member
eof.
A player sending execute script contents quickly could theoretically
get an unfair share of the server. Missed in commit db6fd8da
(v4.2.22), which made only the normal command loop yield.
player_login() skips sending C_EXIT and flushing server output when
io_eof() is true. That's the case after a read from the socket
returned zero, which means the client has shut down transmission on
his socket, or closed it. If it's the former, then dropping output
like that is bad. Our client never does that, but others might.
Condition was introduced in Empire 2, don't know why.
status() informs the player of non-trivial command costs and earnings,
and when he goes broke or becomes solvent. However, this is skipped
when the command gets aborted by the player signalling EOF, or by
server shutdown, and after a quit command.
Fix by moving the check for EOF or shutdown down to the may_play_now()
check.
This looks a bit like it would also fix charging of play time. But
that's not broken, because player_main() charges, too.
Commit 3cceb59b (v4.3.26) fixed the client to abort commands reliably
on ^C, even when it arrives at the beginning of an input line. Except
it didn't work at the beginning of the first input line, because
input_eol was initialized to zero.
Easily fixed, but "end of line" isn't quite right there. Revert sense
and rename to partial_line_sent.
journal_entry_pr(S, N) writes up to N characters from zero-terminated
string S. journal_input() passes -1 for N to write all characters.
Unclean. SIZE_MAX would do, but it's C99, and MSC doesn't provide it.
Simplify journal_entry_pr() to write exactly N characters. This makes
it more similar to write() than to pr(), therefore rename.
pln_equip() refuses to abandon its base sector. Unfortunately, it
checks even when flying off carriers, and refuses to load the last
civilian or military depending on what happens to be in uninitialized
variable sect.
Broken in commit 91139692, v4.3.0.
