A concatenation written in javascript style, with "+" instead of the php ".".
On php 5 and 7 it evaluated to 0+0 with a warning and printed "0"; since
php 8 adding two non numeric strings is a TypeError, and with no catch
anywhere the brisk-spush daemon died on the spot.
The branch is trivial to reach: a request to index_wr.php with an
unrecognised session is enough, an expired cookie for instance. Found by
sending a getchallenge after a daemon restart.
All similar cases were looked for: this is the only one in php code, the
other "+" between strings are inside javascript embedded in the html, or in
shell scripts quoted in comments.
Found by playing a real game in the container: five authenticated users,
table 4, the auction, 40 cards played, score saved.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014M1jiEq9cHdE5SE5j6vFuE
fixes that showed up by actually running the application on debian 13
Found by bringing the whole stack up in a container: apache 2.4.68 with
mod_proxy_fdpass2, the brisk-spush daemon on php 8.4 with the ancillary
extension, postgresql 17. None of these was visible with the lint, with
loading the include chain, or with the tests on the objects: they only show
up by starting the daemon and serving a real request.
sac-a-push.phh: fatal when the daemon starts
sig_handler() was registered with pcntl_signal() as
array("Sac_a_push", "sig_handler"), that is in static form, but declared
non static. Since php 8 that is no longer a valid callable and
pcntl_signal() raises a TypeError: the daemon died before opening a socket.
It is the same class of problem as the 15 static calls already fixed, but
with the array() syntax: the check I had written looked for "Class::method"
and did not see it. The other two callables in that form were checked as
well (IPClassItem::compare and Cookie::create): both already static.
INSTALL.sh: Etc/ was born exposed on the web
The Etc directory holds the configuration with $G_dbauth, that is the
database credentials in clear, and it falls inside the DocumentRoot. The
.pho extension is not associated with php, so the file was served as plain
text: checked, HTTP 200 with the content. In production it is protected only
because someone added a .htaccess by hand; a fresh installation was born
without one. INSTALL.sh now creates it, in the apache 2.4 form with a 2.2
fallback. After the change: HTTP 403.
WARNING.txt: the suggested ProxyPass lines did not work
It is the text INSTALL.sh prints to the administrator as the configuration
to write, and it was wrong in three ways:
- "fd:///path" is refused at configuration time by apache 2.4.68
("ProxyPass URL must be absolute!"); "fd://localhost/path" is needed
- it mentioned a single "brisk.sock", from before the pool existed: the
path is the prefix and the module appends "<N>.sock" to it
- the hardcoded path /var/www/brisk-priv ignored the -U option
Rewritten with the form verified to work, plus the note that the first
argument must be an exact path and not a prefix with a trailing slash.
The file also lists, and this part was already right, which urls go to the
daemon: index.php, index_wr.php, index_rd.php, index_rd_wss.php and the
matching ones under briskin5. Everything else is served by apache.
Final check in the container: GET /brisk/index.php answers 200 with the game
page (19725 bytes), the .css are served by apache, Obj/, spush/ and
briskin5/Obj/ answer 403, Etc/ answers 403, and the daemon does not emit a
single warning or deprecation while serving the requests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014M1jiEq9cHdE5SE5j6vFuE
Found by running the code against a real database: neither the lint nor
loading the sources could see them.
UPDATE ... SET (col) = (val)
Since postgresql 10 the parenthesised form on a SINGLE column is an error
("source for a multiple-column UPDATE item must be a sub-SELECT or ROW()
expression"): (val) is not a ROW but a parenthesised expression. The multi
column form is still valid, checked on the server: of the 11 parenthesised
UPDATEs in the project only 4 need fixing, the other 7 are left alone.
dbase_pgsql.phh SET (lintm) user_update_login_time()
SET (pass) user_update_passwd()
SET (tos_vers) user_tos_update()
SET (game_cnt) bin5_points_save()
sql.d/085-tourn-update.sql two SET (name)
This is not a consequence of the php 8 port: they were already broken on any
postgresql >= 10. They cover password recovery and the acceptance of the
terms of service.
int2four()
The literal 0xffffffff00000000 is above PHP_INT_MAX, so php treats it as a
float and the or converts it back to int: since 8.1 that is the "Implicit
conversion from float to int loses precision" deprecation, emitted on every
call (the function sits in the self-registration check path). Rewritten with
~0xffffffff, same bit pattern but an integer. Identical values, compared on
0, 1, 0x7fffffff, 0x80000000, 0xc0a80001 and 0xffffffff.
Checked against a real database (postgresql 17, schema rebuilt from scratch
with sql/builder.sh: 18 files, 12 tables, 6 views, 0 errors): connection,
queries, user_add, login_exists, getrecord_bylogin, the three fixed UPDATEs,
the two multi column ones, transactions and selfreg. No warnings, no
deprecations. The error branch of BriskDB::query() was checked too, by
forcing a query on a non existing table: it logs with pg_last_error(), does
not raise a TypeError, and the connection survives the recovery.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014M1jiEq9cHdE5SE5j6vFuE
The code was written for php 5. Minimal changes to make it run cleanly on
8.4, with no restructuring.
Fatal errors
- split() -> explode() (removed in 7.0), 5 places
- "$x =& new Class()" -> "= new" (removed in 7.0), 7 places
- 41 php4 style constructors -> __construct(). A non obvious case: Bin5_user
defined "function User() {}", which on php5 was its constructor because it
overrode the slot inherited from User; that one was renamed too.
- 15 static calls to non static methods (Challenges::load_data(),
Hardbans::add(), Table::create(), ...): E_STRICT on php5, Error since 8.0.
"static" added to the 8 declarations, none of them uses $this.
- 5 overrides with incompatible signatures (spawn, copy, load_step,
unproxy_step, page_sync): E_STRICT on php5, fatal since 8.0. The useless
"&" on objects were dropped and the parameters of three methods reordered,
with the two call sites adjusted.
- dbase_pgsql.phh: pg_result_status($res) was called in the branch where
$res is FALSE. Since 8.0 results are \PgSql\Result objects and no longer
resources, so it is not a warning any more but a fatal TypeError - and in
the connection recovery path, of all places. Replaced with pg_last_error().
- dbase_pgsql.phh: "${rules_name}::game_description(...)" was a variable
variable whose name came from an undefined constant; on php5 it degraded to
a string with a notice and resolved to $rules_name by accident, on php8 it
is a fatal Error.
- dbase_file.phh: define() with an unquoted constant name, same mechanism.
- usermgmt.php: "break" outside any loop. On php5 it was a runtime fatal,
since 7.0 it is a compile time one: the file did not load any more.
Deprecations
- 245 "var $prop" -> public
- 29 occurrences of "${var}" inside strings -> "{$var}" (8.2)
- 29 dynamic properties declared (8.2). User declared $brisk but the code
always uses $room: renamed, nobody reads $user->brisk.
- 53 pg_numrows() -> pg_num_rows(): the alias is deprecated in 8.4
- strftime() -> date(), shmop_close() -> unset()
- room_join_wakeup(): removed a default followed by a mandatory parameter
mbstring.func_overload
It was set to 7 in the .htaccess files and was removed in 8.0. All 62 call
sites of strlen/substr/strpos were examined: they are either pure ASCII or
deliberately byte oriented, and moving to php8 fixes them, given that the
websocket frame parsing in transports.phh and the fwrite accounting in
sac-a-push.phh would have been wrong under overload. No change needed: where
character semantics were required the author already used explicit mb_*.
The only exception is index_wr.php, where mail() is no longer remapped onto
mb_send_mail(): mb_encode_mimeheader() was added on the subject and on the
user name, which otherwise ended up as raw UTF-8 in the headers.
Configuration (debian 13)
- .htaccess: func_overload removed, internal_encoding/http_input replaced by
default_charset; the php_value block now sits inside <IfModule mod_php.c>
because with PHP-FPM apache would answer 500
- the three .htaccess that protect the sources used the apache 2.2 syntax
(Order/Deny), which needs mod_access_compat: now "Require all denied" with
a fallback
- system/etc_php5_conf.d_mbstring.ini -> etc_php8.4_conf.d_brisk.ini
- INSTALL.sh: "php5 -l" -> "php -l"
Checked: php -l clean on 64 files; the whole include chain of the daemon
loads with E_ALL without warnings or deprecations; the tests in test/ pass.
Not yet verified against a database: dbase_pgsql.phh was only checked
statically.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014M1jiEq9cHdE5SE5j6vFuE
The class.phpmailer.php in web/Obj/ was the 5.1 release from 2009 and does
not run on php 8: it uses each() (removed in 8.0),
get_magic_quotes_runtime() and set_magic_quotes_runtime() (removed in 8.0)
and php4 style constructors.
Replaced by PHPMailer 7.1.1 in web/Obj/PHPMailer/ (src/ plus the italian
language file and the LICENSE). 7.x was chosen over 6.x because 7.0.0 is
identical to 6.11.1: the major bump only signals a compatibility break for
those who extend the class (lang(), setLanguage() and $language became
static), and here PHPMailer is not extended. It is the line maintained for
php 8.4. No composer: the project does not use it, and INSTALL.sh already
copies files recursively - only LICENSE and VENDOR.txt had to be added to
the list of copied names.
mail.phh adjusted: namespace PHPMailer\PHPMailer, explicit require of the
three files (no autoloader), setFrom() instead of assigning From/FromName
directly.
A missing catch was added too: brisk_mail() builds PHPMailer with
exceptions=TRUE, so send() throws instead of returning FALSE, but none of
the 7 callers catches and all of them test for "== FALSE". A delivery error
killed the spush daemon. The exception is now logged and reported as FALSE,
which is what the callers already expected.
NOTE: msgHTML() overwrites AltBody with its own conversion of the html, so
the text passed to brisk_mail() is discarded. This was already the case
with 5.1, and the behaviour is left untouched (see the comment in the file).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014M1jiEq9cHdE5SE5j6vFuE
projects / brisk.git / log