Package: bsdgames Version: 2.17-35 Severity: important Tags: patch upstream
Dear Maintainer,
sail(6) cannot be played on any architecture where plain "char" is unsigned.
I found this on arm64 (Raspberry Pi 5, Debian 13.7); by the same reasoning it
affects armel, armhf, ppc64el, riscv64 and s390x, and not amd64, i386,
loong64 or mips64el.
What happens
Start "sail", take scenario 0 (Ranger vs. Drake) and either ship, give a
move such as "m" "3" and wait a few turns. On amd64 the Drake closes and the
two sloops fight within half a dozen turns. On arm64:
* the other ship disappears from the view on the first turn and is never
seen again -- I played 278 turns once, moving towards her all the time,
while she got further away;
* the wind vane at the right-hand edge of the view is wrong from the first
screen: the wind speed digit is there, but the '+' is missing and the tail
('|', '/', '-' or '\') sits two rows lower and one column to the left of
where it belongs;
* when broadsides do bear (ships can still meet if they only ever move
north or west), every one of them does the maximum damage in the tables.
Why
sail/globals.c keeps small signed numbers in plain char:
const char dr[] = { 0, 1, 1, 0, -1, -1, -1, 0, 1 };
const char dc[] = { 0, 0, -1, -1, -1, 0, 1, 1, 1 };
and likewise the gunnery modifier tables AMMO[], HDT[], HDTrake[] and QUAL[],
which hold values from -1 to -4. With unsigned char each -1 is 255, so
sp->file->row -= dr[sp->file->dir] * dist; (step(), dr_3.c)
moves a ship heading south by -255 rows instead of +1; any heading with a
southerly or easterly component, and drifting before a northerly or westerly
wind, does the same. Sterns are computed 255 squares from their bows, so
range(), gunsbear() and portside() in misc.c are wrong as well. In pl_7.c
draw_slot() does wmove(slot_w, 7 - dr[winddir], ...), which fails for row
-248, so the vane's tail is drawn where the cursor happened to be and the
mvwaddch() of the '+' fails outright. In dr_1.c and pl_3.c the hit value
"HDT[..] + QUAL[..] + AMMO[..]" comes out around +250 instead of slightly
negative, passes the "hit >= 0" test and is clamped to the top row of the
damage tables (grape shot is not clamped at all).
The attached chartest.c shows the arithmetic in ten lines; on arm64 it prints
plain char is unsigned: dr[5] = 255, the ship is now in row -248 (should be
8)
A second instance of the same assumption is in sail/sync.c:
switch (*p++ = getc(sync_fp)) {
...
case EOF:
The switch is on a char, so with unsigned char "case EOF" can never match
(gcc: "case label value is less than minimum value for type"), and a string
record that is cut short at the end of the synchronization file makes every
player and the driver loop for ever with the file locked. NetBSD has since
fixed this one, but its current sail still has the "const char" tables, so
the main bug is present upstream as well (hence the "upstream" tag).
The fix
The attached patch (DEP-3 header, applies with -p1 after the 22 patches in
2.17-35, as debian/patches/0023-...) declares the six tables "signed char"
in globals.c and extern.h, and reads the synchronization file through an
int. It changes nothing on architectures where char is signed.
I went through the other plain-char members of sail's structures (struct
File, shipspecs, scenario, windeffects, Tables): none of them ever holds a
negative value (the -1 "no fourth mast" marker is in rig4, which is an int),
so the six tables and the getc() are all there is.
Testing
I built sail from the 2.17-35 source on arm64 with and without the patch,
with gcc's default (unsigned) char in both cases. Without it: the vane as
described, and by turn 4 neither ship was in the view any more. With it: the
vane is drawn correctly ('+', speed, tail in one line) and by turn 3 the Drake
had closed with the Ranger, collided and opened fire. Building the unpatched
source with -fsigned-char gives the same result as the patch, which would be
a one-line alternative (for sail only) if you prefer not to touch the source.
I have also been playing, and testing under gcc's address and undefined
behaviour sanitizers, a build with this fix (and a good many other changes
that are not part of this report) without seeing any other signedness
problem.
-- System Information:
Debian Release: 13.7
Architecture: arm64 (aarch64)
Kernel: Linux 6.18.50+rpt-rpi-2712 (Raspberry Pi 5)
Versions of packages bsdgames depends on:
ii libncurses6 6.5+20250216-2
0023-sail-plain-char-is-unsigned-on-arm.patch
Description: Binary data
