“CONSTANT VIGILANCE!” — Mad-Eye Moody, probably staring at a DBI statement handle that was never closed.
The previous audit session fixed the obvious statement handle leaks. This one went deeper — into the nested my $sth shadows, the early return paths that quietly skipped ->finish, and the functions that looked correct at a glance but were leaving handles open every single time they ran. Eight functions. Eight leaks. Some of them on code paths that execute dozens of times per minute on an active IRC server.
DBI statement handles accumulate on the server side until either ->finish is called or the handle goes out of scope and gets garbage-collected. In a long-running event loop like Net::Async::IRC, nothing goes out of scope. The handles pile up silently.
Helpers.pm — getIdUser()The simplest case: one prepare, zero finish. Every call to getIdUser() — which happens indirectly on most commands — left a cursor open on MariaDB. Fixed by adding $sth->finish before the return.
Helpers.pm — logBotAction()More subtle. The function has two early return paths inside its channel lookup block: one on SQL error, one when the channel isn’t found. Both returned without calling finish on the $sth from SELECT id_channel FROM CHANNEL. A finish was added to each exit path.
Quotes.pm — mbQuoteDel()A classic shadow bug. The outer $sth (SELECT, checking if the quote exists) was shadowed by an inner my $sth (DELETE). The $sth->finish at the end of the block closed the DELETE handle — but the SELECT handle was never touched. Fixed by renaming to $sth_sel / $sth_del and adding explicit finish calls in every branch, including the SQL error path.
Quotes.pm — mbQuoteStats() — also an improvement, see belowThree prepare calls, one finish at the very end. If either of the two inner queries failed and returned early, the first handle was never closed. Addressed as part of the consolidation refactor.
DBCommands.pm — mbDbCommand()Exact same shadow pattern as mbQuoteDel: the SELECT handle was overwritten by an UPDATE $sth = $self->{dbh}->prepare(...), so the final $sth->finish closed the UPDATE, not the SELECT. Additionally, the SQL error path and the “command not found” path both returned without any finish at all. Refactored to $sth_sel / $sth_upd with immediate fetchrow + finish on the SELECT, then flat processing of the result.
DBCommands.pm — doResponder()Identical pattern. Two handles, one finish, the wrong one closed. Same refactor applied: $sth_sel fetches the responder row and is finished immediately; $sth_upd increments the hit counter and is finished after.
DBCommands.pm — mbDbAddCommand_ctx()The INSERT path had a return on SQL error without calling finish on the prepared statement. One line fix: $sth->finish if $sth before the return.
UserCommands.pm — dbLogoutUsers() and setUserLevel()Two UPDATE statements with no finish at all. Both now call finish in all exit paths.
mbQuoteStats() — Three Queries Become OneThe original implementation ran three sequential queries to compute the quote statistics for a channel:
SELECT COUNT(*) FROM QUOTES WHERE channel = ?SELECT UNIX_TIMESTAMP(ts) FROM QUOTES WHERE channel = ? ORDER BY ts LIMIT 1 (oldest)SELECT UNIX_TIMESTAMP(ts) FROM QUOTES WHERE channel = ? ORDER BY ts DESC LIMIT 1 (newest)All three hit the same table with the same filter. They were also nested seven levels deep — if/else inside if/else inside unless inside else — making the control flow genuinely hard to follow. And the elapsed-time formatting code was duplicated verbatim for the min date and the max date.
The rewrite collapses all three into a single query:
SELECT COUNT(*) AS nbQuotes,
UNIX_TIMESTAMP(MIN(ts)) AS minDate,
UNIX_TIMESTAMP(MAX(ts)) AS maxDate
FROM QUOTES
JOIN CHANNEL ON CHANNEL.id_channel = QUOTES.id_channel
WHERE CHANNEL.name = ?
The formatting logic is extracted into a local $elapsed_to_human closure used twice. The function goes from 85 lines with 7 nesting levels to 68 lines with 2 nesting levels, one prepare, and two finish calls covering both exit paths.
The Partyline is a TCP telnet-style admin interface for direct bot control. It already had rate limiting on commands (10 per 5 seconds). What it didn’t have was any protection on the login itself.
An attacker with TCP access could simply loop through password guesses indefinitely. The rate limiter doesn’t apply until after authentication.
A login_failures counter is now tracked per connection in the session hash, initialized to 0. Every failed authentication attempt — unknown user, wrong password, or insufficient level — increments it. At 5 failures, the connection receives a final message and is closed with close_now. A successful login resets the counter to 0.
curl — External.pm Goes Full HTTP::TinyExternal.pm already used HTTP::Tiny for 9 out of 12 HTTP calls. The remaining three used open $fh, "-|", "curl", ... pipe calls to fetch YouTube API responses:
getYoutubeDetails() — YouTube v3 videos endpointyoutubeSearch_ctx() — YouTube search endpointyoutubeSearch_ctx() — YouTube videos endpoint (second call)The pipe form is safe (list arguments, no shell expansion), but it requires an external binary, adds a process fork per call, and is inconsistent with the rest of the module. All three are now replaced with HTTP::Tiny->new(timeout => 10)->get($url). The stale log message referencing the curl command string was updated at the same time.
AdminCommands.pm — Retiring the Last getNickInfo Ghostsub update was a relic from the pre-Context era. It called getNickInfo($self, $message) — a function that returns 8 positional variables in a flat list — to perform its own authentication check manually. Its entire body was:
$self->{logger}->log(4, "Update TBD ;)");
It was wrapped by update_ctx, which called it by unpacking the Context object back into individual arguments, negating the entire point of the Context refactor.
Both functions have been replaced by a single update_ctx that calls $ctx->require_level('Owner') directly. The getNickInfo dependency in AdminCommands.pm is gone. The remaining getNickInfo calls live in the WHOIS handlers in mediabot.pl — those are next.
| File | Change |
|---|---|
Helpers.pm |
finish in getIdUser and logBotAction |
Quotes.pm |
finish fixes in mbQuoteDel; mbQuoteStats refactored (3 queries → 1, 85L → 68L) |
DBCommands.pm |
finish fixes in mbDbCommand, doResponder, mbDbAddCommand_ctx |
UserCommands.pm |
finish in dbLogoutUsers and setUserLevel |
Partyline.pm |
Brute-force lockout after 5 failed login attempts |
External.pm |
3× curl pipe → HTTP::Tiny, module now curl-free |
AdminCommands.pm |
getNickInfo legacy removed, update_ctx uses Context natively |
You must be logged in to reply.