Merge upstream unrealircd 6.2.6-rc1#20
Merged
Merged
Conversation
so they actually work.
and refer to this as well. Suggested by PeGaSuS in https://bugs.unrealircd.org/view.php?id=6610 This also moves extras/tls.cnf to doc/conf/tls/tls.cnf which also gets installed in ~/unrealircd/conf/tls/ (or whatever CONFDIR is) And just to be clear: this means you can run "./unrealircd makecert" without needing to go into BUILDDIR (or even having it at all). At the same time, the generation commands have been modified slightly so two warnings during certificate generation are no longer there.
…dirs. Only for ~/unrealircd/lib/ we had this ommision, and for ~/unrealircd itself. I doubt this means a change for users, as all subdirs were already 0700 so then tightening of ~/unrealircd is not very important. And only upsides... making things safer..
[skip ci]
By default - assuming you don't set set::reject-message things by yourself - the *LINE id is appended at the end of the rejection that is shown to the user, like: [ID: G7K2MP9WQX3]. Also new is spamfilter to *LINE mapping, so you can see which *LINE was set by which SPAMFILTER. For this STATS gline and friends were enhanced. In fact, multiple fields were added there, including some that are 0 (zero) placeholders at the moment. These will be set in a future commit. Some things were combined here so we only have to break STATS and tkldb database format once (unless i made a mistake, then the follow up commit will correct that i guess :D). This was requested by Hero in https://bugs.unrealircd.org/view.php?id=4397 in 2015. Again by musk in https://bugs.unrealircd.org/view.php?id=4397 in 2022. And on IRC by Chris and others. As you can see it was not SUPER easy and a lot of thought went into this (and in terms of S2S traffic it is part of something bigger too)
…time of the last hit, eg in `STATS gline` for GLINEs. These counts happen on each individual server and are not network-wide. This allows IRCOps to see which entries never get any hits and can potentially be removed. * Important exception: config-based spamfilters/bans lose their counters on `REHASH` and restart atm. * For non-config TKLs, the hit count and last hit timestamp are preserved across reboots (via tkldb). * Again, see *Developers and protocol* for the exact STATS field. The spamfilter hits already existed but all the rest is new. Suggested by BlackBishop in https://bugs.unrealircd.org/view.php?id=6304 (in particular, time of the last hit)
…hashes. Unlike non-config-based TKLs - which go through tkldb - they are still not preserved through restarts. But at least they are not lost due to REHASH. This is done via a save+restore, a bit complicated, but we have little choice (other than not doing this at all). This also moves remove_config_tkls() from conf.c to tkl.c
means shun IDs now start with "H". Update release notes. This, after i realized that for like *LINEs that are added by spamfilter the two ID fields in "STATS gline" are a bit confusing as to which ID is what. Now the spamfilter one starts with "SPAM" so there can be no confusion. The gline one still starts with "G" as before. Since I kept the generated ID length the same, this means there is less bits available for the spamfilter ID, but there are rarely more than 1000 spamfilters, and in that scenario there's just as little birthday attack collision % as with 200k glines, just to illustrate (~0.0015% vs ~0.0018%)
…DEL, TKL_EXPIRE and SPAMFILTER_MATCH messages. This uses the newly added functions log_data_optional_string() and log_data_optional_name_value(). The first shows the optional string like "abc" and the second expands to "[name: value]". What's also new is that both of these will swallow a preceding space if there is no value. This so you can just use "Something. $optional_string" and it will expand to "Something." if $optional_string is empty. This makes things less hacky and more human readable :)
…ace. Basically if a $variable is empty, and there is a space before it in the template string then we delete that space. May seem (or is) a bit over the top but this way the template stays clean, and it may be used/useful in other places as well. This is a behavior change, but I think we can live with it. One can opt- out via BUILDVARSTRING_KEEP_SPACE_FOR_EMPTY_VAR.
Suggested by westid in https://bugs.unrealircd.org/view.php?id=6477 * New [crule function](https://www.unrealircd.org/docs/Crule) that return the number of times a flood was blocked for that user. For example, `server_flood_count('away')` returns the number of time away-flood was exceeded. Aslo available: `nick`, `join`, `invite`, `knock`, `vhost` and `conversations`. Plus, there is `all` for a total of all. * This can be used in a security-group::rule or spamfilter::rule. Eg: `spamfilter { rule "server_flood_count('nick')>4"; action gline; }` This also - internally - adds a mechanism to run spamfilter rule-only- filters after the command handler, whenever a tag value or other thing changed. That's part of this commit.
… exceeded
* `total_channel_flood_count('..setting..')` returns the number of
times `+f`/`+F` limits were exceeded by that user in all channels
the user is or was in. Available are: `nick`, `join`, `knock`, `msg`,
`ctcp`, `text`, `repeat` and `paste` (and `all` for the sum).
We were merging draft/multiline-concat lines together server-side before sending them to non-multiline clients. This could truncate oversized merged lines. We now simply send them as separate lines. Reported by ProgVal in https://bugs.unrealircd.org/view.php?id=6628
to the server where the user is actually on. Think of idle time etc.
* JSON-RPC: We can now route `user.get` requests to the server that user is
on. This so we can fetch all fields for that user (including flood
counters, idle time, snomask) that are normally not available remotely.
* We do this automatically in `user.get` when `object_detail_level` is 5+.
* You can force this explicitly with `object_remote_fetch` set to `true`.
So you can also use it with detail level 2 if you want, e.g. if you
don't need the flood counters but do want the idle time.
* When RRPC is not available we answer ourselves (so safe fallback, but
you won't have the local-only fields).
Oh and we deliberately don't do this in `user.list`, as doing it there
would mean a single request could result in hundreds of semi-`user.get`
calls across multiple servers.
…ssages. This was used by `server.rehash` and `server.module_list`. Plus, this release `user.get` under some circumstances. This is now fixed but requires the target server to be on UnrealIRCd 6.2.6. If the target server does not meet this condition then we error telling the server "does not support remote JSON-RPC". This was first reported by AdmiraL- in https://bugs.unrealircd.org/view.php?id=6611
`serversonly` (such as port 6900 in the example.conf) and link { } blocks
in a different way than regular listen { } blocks:
* If there are different certificates used in the serversonly listen block
vs link blocks, then this is almost always means server linking is broken,
so we now print a warning on boot and rehash.
* We also print an 'advice' if any of these are not using (long-lived)
self-signed certificate. This is because CA issued certificates are
typically not suitable because they typically rotate keys and thus change
the `spkifp`. Changing spkifp breaks server linking. We will now print
an advice along with command and config block instructions to fix it.
* We now use `set::server-linking::tls-options` for link { } blocks
and listen { } blocks that are `serversonly`. All the rest uses the
`set::tls` settings by default (eg the regular listen { } block on 6697).
* This means our guide on
[Using Let's Encrypt with UnrealIRCd](https://www.unrealircd.org/docs/Using_Let's_Encrypt_with_UnrealIRCd)
and generic usage is more intuitive. You just set both set settings
and then no longer need to use any tls-options in listen blocks or link
blocks. The example conf has also been updated with this.
* If `set::server-linking::tls-options` is not configured, it defaults
to `set::tls`, so there is no unexpected behavior change for anyone.
* In a future release we will make server linking with `spkifp` mandatory,
so all of this helps with getting people ready for that, making such
a future transition smooth.
TODO: Update wiki, better wording in release notes, etc.
This also changes the default example conf:
/* RECOMMENDED:
* Everyone should be using IRC over SSL/TLS on port 6697. However, to use
* it properly, you have to get a "real" certificate instead of the
* self-signed default certificate that was generated by the installer.
* The Let's Encrypt initiative allows you to get a free certificate that is
* issued by a trusted Certificate Authority. Instructions are at:
* https://www.unrealircd.org/docs/Using_Let's_Encrypt_with_UnrealIRCd
*
* When you follow that guide you will have a "dual certificate" setup:
* set::tls:
* Your trusted CA certificate, served to clients on port 6697.
* (key and certificate change and renew every xx days automatically)
* set::server-linking::tls-options
* A long-lived self-signed certificate for server linking, with
* a stable 'spkifp' signature that you use in link blocks.
* This certificate is used automatically in "serversonly" listen blocks
* (port 6900 in this configuration file) and automatically used for all
* link { } blocks.
*
*/
//set {
// tls {
// certificate "/etc/letsencrypt/live/irc.example.org/fullchain.pem";
// key "/etc/letsencrypt/live/irc.example.org/privkey.pem";
// }
// server-linking {
// tls-options {
// certificate "tls/server.cert.pem";
// key "tls/server.key.pem";
// }
// }
//}
…them
and "./unrealircd genlinkblock" outputs multiple password ".." { spkifp; }
lines in such a case.
Other than that some cleaning up of recently-added-functions that are
now no longer needed: we now create ctx_link_server and ctx_link_client
that represent set::server-linking::tls-options for incoming and outgoing
links. Which can be NULL, and then we use ctx_server / ctx_client (set::tls).
Also add proper documentation on this.
When using ./unrealircd spkifp, tell ./unrealircd genblock is cooler.
Nah.. it takes more factors into account, genlinkblock, so is preferred :D
print copy-pastable config blocks. Previously we used add_name_list(), which uses insert at beginning, which would reverse the order. Also changed duplicate_name_list() to preserve order. Previously it reversed the order of all items.
[skip ci]
(we should actually remove this one day :D)
Not in UnrealIRCd itself: it is only used in one place, STATS with a big buffer. And unrealircd-contrib 3rd party modules has no consumers.
…issue. This is unreachable in current code paths, but could be some day.
This fixes an OOB write that cannot be reached by users. Only a hostile server could cause it in some situations. Even then, in my tests this did not cause a crash (it goes into bss too, not heap or stack).
…rcd#336) The IRCv3 specifications for these have been ratified: - https://ircv3.net/specs/client-tags/reply - https://ircv3.net/specs/extensions/no-implicit-names Both the draft and ratified names are supported during a transition period.
Signals to the client that it has reached the end of the history and there are no more messages to fetch. The tag is attached to the BATCH opener when the server returns the last page of results. Only sent to clients that negotiated the draft/chathistory capability.
a directorion, but is a midpoint, and we send X lines above Y under, so end does not make sense there anyway (which of the two ends?). We simply avoid sending it.
And attach draft/chathistory-end when exactly 'limit' targets exist and nothing more.
The previous mechanism (from yesterday) was a bit too simple at the chathistory.c where returned_lines < limit would set the end tag but it would not deal with the situation where returned_lines == limit which is ambigious. So we had to move up a layer (or is it down?), don't handle this in chathistory.c but in the backend. A new struct field r->reached_end was added for this (set by backend).
Eg on failed oper attempts, that sort of things. Previously it was still adding fake lag. This complicated unrealircd-tests :). As always, nofakelag should never be used in normal conditions, it disables the most important protection we have (fake lag bumping). If you want lower lag for a group of users, the right tool is set::anti-flood::name-of-security-group::lag-penalty and ::lag-penalty-bytes See https://www.unrealircd.org/docs/Special_users
And a minor README.md update to add a few more links.
1) obviously only provide ASan report if relevant (eg memory issue), not for like priv escalation :) 2) "If you are submitting issues and fail to follow the procedure above" was in the AI/tooling paragraph, but just in case someone reads it out of that context we now scope AGAIN it to that ONLY. This so normal users (that have nothing to do with AI/tooling) are not scared off in reporting real issues. [skip ci]
… unaffected. When not using version 302, such as with "CAP LS", the specification does not allow us to use continuation lines. This means all advertised caps must fit into one line. That is no longer always the case, especially if you load 3rd party capabilities. So we need to scratch advertising some capabilities to <302 clients. "CAP LS 302" is unaffected. Note that version 302 in the specification exists since at least November 2017, so most clients use that one. According to https://ircv3.net/software/clients the following clients are affected by this change: Desktop Clients * KVIrc * Circe * catgirl * BitchX * Pidgin * LimeChat Mobile Clients * IRC for Android * LimeChat And various older versions of other clients (obviously). NOTE: The source is only that IRCv3 page. I did not check manually. For this particular commit. We filter out various unrealircd.org informative CAPs and the vendor specific json-log. So that isn't much of a problem. However, in the future we may be forced to filter out more capabilities to make room. It would be much better if all clients are on >=302. Also, I should mention we are not the only IRCd out there, so I can't vouch on what other IRCds (will) do when hitting this non-302-limit. Reported by ProgVal in https://bugs.unrealircd.org/view.php?id=6630
TKL hits/id taken from upstream (dropping the fork's parallel reimplementation); account-aware banexception kept. Version stays ObbyIRCd 6.2.6.
📝 WalkthroughWalkthroughThis PR prepares UnrealIRCd 6.2.6-rc1 with the following changes: replaces ChangesUnrealIRCd 6.2.6-rc1 feature release
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ValwareIRC
approved these changes
Jun 23, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (2)
src/conf.c (1)
7725-7726: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse block comments in C sources.
These new
//comments violate the C comment-style rule; switch them to/* ... */.As per coding guidelines, "
**/*.{c,h}: Use only/* */comments in C code, never//comments".Proposed fix
- // certificate_files: done at end of function - // key_files: done at end of function + /* certificate_files: done at end of function */ + /* key_files: done at end of function */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/conf.c` around lines 7725 - 7726, Convert the two C++ style comments (using //) to C style block comments (using /* */) in src/conf.c. Specifically, replace the comments about "certificate_files: done at end of function" and "key_files: done at end of function" by wrapping each comment text with /* and */ delimiters instead of using the // prefix. This ensures compliance with the coding guideline that C source files must use only /* */ style comments, never // comments.Source: Coding guidelines
src/modules/tkl.c (1)
197-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the new C declarations/comments in project style.
The new struct typedefs use K&R braces, and Line 6055 adds a
//comment. Please switch these to Allman braces and/* */comments.Proposed style cleanup
-struct ConfigTKLHits { +struct ConfigTKLHits +{ ConfigTKLHits *prev, *next; char key[256]; long long hits; time_t lasthit; long long hits_except; /* spamfilter only */ time_t lasthit_except; /* spamfilter only */ }; -typedef struct { +typedef struct +{ const char *name; const char *(*serialize)(TKL *tkl); /**< value to send, or NULL to omit the tag */ void (*unserialize)(TKL *tkl, const char *value); } TKLS2SField;- // Yeah we are re-running the text analysis. + /* Yeah we are re-running the text analysis. */As per coding guidelines,
**/*.{c,h}: “Use only/* */comments in C code, never//comments” and “Use Allman-style braces for C code (opening brace on new line), never K&R style.”Also applies to: 6055-6055
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/modules/tkl.c` around lines 197 - 215, Update the code style for the new declarations to match project guidelines. For the ConfigTKLHits struct and the TKLS2SField typedef, change from K&R-style braces (opening brace on the same line) to Allman-style braces (opening brace on a new line). Additionally, convert any C++ style comments (using //) to C-style comments (using /* */) as required by the project coding guidelines. This applies to the struct ConfigTKLHits definition, the typedef struct definition, and the comment on line 6055.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Config`:
- Around line 53-58: Quote all the path variables in the mkdir and chmod
commands (around lines 53-58) by wrapping $BASEPATH, $TMPDIR, and $CONFDIR in
double quotes to prevent word-splitting and glob expansion. Additionally, add
fail-checks after each mkdir command by appending || exit 1 to ensure the script
exits if any directory creation fails, rather than continuing with invalid
paths.
- Line 194: The ./unrealircd mkcert command does not have its exit status
checked before printing a success message, meaning a failed certificate
generation will still appear successful. After executing the ./unrealircd mkcert
command, check its exit status and only print the success message if the command
succeeds. If the command fails, the script should handle the error appropriately
(typically by exiting with a non-zero status or printing an error message).
In `@configure.ac`:
- Around line 561-565: The mkdir and chmod commands in the private library
directory creation block do not cause the configure script to exit if they fail,
which can lead to confusing errors later during bundled library installation.
Add error handling to the line containing mkdir -p "$PRIVATELIBDIR" && chmod
0700 "$PRIVATELIBDIR" so that the configure script immediately exits with a
fatal error if either the directory creation or permission change operation
fails. This ensures failures are caught early and reported clearly rather than
silently allowing the script to continue with an improperly configured
directory.
In `@doc/conf/help/help.fr.conf`:
- Around line 920-923: Fix the spelling errors in the ELINE bantype descriptions
block where handshake is misspelled as "handhsake", Exception is misspelled as
"Exeptmion", and restriction is misspelled as "restricion". Locate these three
typos in the lines containing the d, m, and r bantype entries respectively and
correct them to their proper spellings to ensure clarity in this
security-sensitive help documentation for French-speaking operators.
In `@doc/conf/modules.default.conf`:
- Line 261: Remove the duplicate loadmodule directive for "multiline" in the
configuration file. The multiline module is already being loaded earlier in the
configuration, so the second loadmodule "multiline" statement causes a duplicate
module configuration error during fresh installs. Delete the entire line
containing the duplicate loadmodule "multiline" directive with the
draft/multiline comment.
In `@doc/Config.header`:
- Line 10: The Config banner in doc/Config.header displays version 6.2.6-rc1
while the actual project version in configure.ac is declared as 6.2.6, causing a
mismatch that can confuse operators. Update the version string in the
Config.header banner where it says "for UnrealIRCd 6.2.6-rc1" to match the
official release version 6.2.6 declared in configure.ac, ensuring consistency
across the configuration files.
In `@doc/RELEASE-NOTES.md`:
- Around line 4-5: The release status text in doc/RELEASE-NOTES.md (lines 4-5
starting with "This is the Release Candidate for future version 6.2.6")
conflicts with the maintained 6.2.6 version referenced elsewhere in the PR.
Update this release status text to accurately reflect the current release status
of version 6.2.6 according to the versioning policy, removing the "RC for
future" language and replacing it with the appropriate status for the current
6.2.6 release.
- Line 12: The ATX style headings (### Enhancements:, and the same pattern at
lines 65, 107, and 122) need to be converted to setext style headings with
underlines to satisfy markdownlint requirements. Convert each of these four
headings by removing the ### prefix and adding an underline of dashes directly
below the heading text, maintaining consistent setext formatting throughout the
file to ensure they are properly recognized as one-level increments from the
top-level section.
In `@obbyircd.in`:
- Around line 321-338: The issue is that if the req command (certificate
generation) fails after the ecparam command (key generation) succeeds, the live
key will be overwritten while the certificate remains old, creating a mismatched
TLS pair. To fix this, modify the code to generate both the key and certificate
into temporary files using temp file names (e.g., appending .tmp to the paths)
instead of directly outputting to the $KEY and $CERT variables. After both the
ecparam and req commands complete successfully (which they already do with ||
exit 1), then move the temporary files to their final locations using mv
commands. This ensures atomicity where either both files are updated together or
neither is updated if any command fails.
In `@src/api-efunctions.c`:
- Line 444: The efunc_init_function call for
EFUNC_RUN_DEFERRED_RULE_ONLY_SPAMFILTERS is registered with NULL as the default
handler (third parameter), which can cause NULL pointer dereference when parse.c
calls run_deferred_rule_only_spamfilters from the core packet loop if the
provider is not loaded. Replace the NULL parameter with a reference to a no-op
default handler function that safely handles the case when no provider is
available, ensuring the function has a safe default behavior instead of being
NULL.
In `@src/list.c`:
- Around line 566-570: The _append_name_list function uses strcpy to copy the
name parameter, which violates the repository's C safety guidelines that require
bounded string copies. Replace the strcpy(e->name, name) call in
_append_name_list with strlcpy(e->name, name, strlen(name)+1) to use bounded
string copying. Additionally, locate and fix the similar strcpy call at lines
591-593 with the same strlcpy replacement. Ensure all unsafe string operations
use the repository's standard bounded copy functions (strlcpy, strlcat, or
ircsnprintf) as required by the coding guidelines.
In `@src/modules/tkl.c`:
- Line 6119: The assignment statement resetting
stop_processing_general_spamfilters and stop_processing_central_spamfilters
unconditionally erases any stop state that was set during normal spamfilter
processing when rule-only filters are subsequently executed. Modify the reset
logic to only reset these flags on the initial call before processing normal
spamfilters, and preserve the existing stop-processing state if it has already
been set by a prior stop action or stop-on-first-match condition. Apply the same
preservation logic at lines 6408-6411 where this pattern also occurs.
- Around line 1566-1570: The `client` parameter in the `_tkl_hit()` function is
unused and triggers compiler warnings. Add an explicit `(void) client;`
statement at the beginning of the `_tkl_hit()` function body to intentionally
mark the parameter as unused. Similarly, locate the `config_tkl_hits_free()`
function and add `(void) m;` at its beginning to mark the unused `m` parameter
in the same way. These explicit void casts inform the compiler that the unused
parameters are intentional and will suppress the warnings.
- Line 2753: The condition in the spamfilter deletion check is using strcmp()
for ID comparison, which is case-sensitive, but find_tkl_by_id() performs
case-insensitive matching. To fix this inconsistency and allow case-insensitive
deletion via the /SPAMFILTER del command, replace both strcmp() calls in the
condition (the one comparing tk->id with id and the one comparing
spamfilter_fallback_id(tk) with id) with strcasecmp() to ensure case-insensitive
string comparison.
- Around line 3304-3315: The issue is that valid local config spamfilter IDs are
being silently replaced with a fallback hash because tkl_config_run_spamfilter()
clears the id parameter for non-central spamfilters before calling this
function. Either ensure that the id parameter is preserved and passed through
correctly from tkl_config_run_spamfilter() for local config spamfilters (so the
condition checking !BadPtr(id) in the current code block actually receives the
intended id), or alternatively add validation logic outside this code block to
explicitly reject id values that are provided for non-central config spamfilters
to prevent silent discarding of user-provided IDs.
- Around line 1423-1438: The functions tkl_s2s_set_id and
tkl_s2s_set_spamfilter_id currently only validate the length of the input values
but do not validate the actual characters, allowing spaces and control
characters that can corrupt STATS/log fields. Locate the existing ID character
validation function that is used for config IDs (this validation likely checks
for valid ID characters), and apply that same validation check to both
tkl_s2s_set_id and tkl_s2s_set_spamfilter_id before calling strlcpy to copy the
value into tkl->id and tkl->spamfilter_id respectively. Ensure the value is only
copied if both the length check and the character validation pass.
In `@src/modules/tkldb.c`:
- Around line 569-583: For database entries with version less than 6260, the
tkl->id field is never populated and remains empty, which causes empty IDs to be
persisted when entries are later rewritten at lines 810-811. Add an else block
after the version check (after the closing brace of the if statement that checks
version >= 6260) to generate a fresh ID for tkl entries when the version is less
than 6260. This ensures that existing persistent server bans and name bans
receive the new operator-visible TKL identity before being saved as version 6260
during the first database rewrite.
In `@src/tls.c`:
- Around line 858-881: The server-linking TLS contexts are being activated
immediately after each context is built separately. If the second init_ctx call
fails to build the client context, the server context is already activated,
creating an inconsistent state where incoming server links use the new context
but outgoing links keep the old one. Instead, build both the server context and
client context using init_ctx into temporary variables first (without activating
them), verify that both init_ctx calls succeed, and only then free the old
contexts and assign the new temporaries to ctx_link_server and ctx_link_client.
This ensures both contexts are successfully built before any activation occurs.
In `@src/user.c`:
- Around line 1163-1168: The issue in the tags_serial management code is that
the increment operation (client->local->tags_serial++) occurs before the wrap
check, causing signed integer overflow if tags_serial already equals INT_MAX. To
fix this, reverse the order of operations by checking whether tags_serial is >=
INT_MAX before incrementing it, ensuring the reset to zero happens first to
prevent the overflow from occurring.
---
Nitpick comments:
In `@src/conf.c`:
- Around line 7725-7726: Convert the two C++ style comments (using //) to C
style block comments (using /* */) in src/conf.c. Specifically, replace the
comments about "certificate_files: done at end of function" and "key_files: done
at end of function" by wrapping each comment text with /* and */ delimiters
instead of using the // prefix. This ensures compliance with the coding
guideline that C source files must use only /* */ style comments, never //
comments.
In `@src/modules/tkl.c`:
- Around line 197-215: Update the code style for the new declarations to match
project guidelines. For the ConfigTKLHits struct and the TKLS2SField typedef,
change from K&R-style braces (opening brace on the same line) to Allman-style
braces (opening brace on a new line). Additionally, convert any C++ style
comments (using //) to C-style comments (using /* */) as required by the project
coding guidelines. This applies to the struct ConfigTKLHits definition, the
typedef struct definition, and the comment on line 6055.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 38529733-bc8d-4e38-aadb-f348fa152309
📒 Files selected for processing (70)
ConfigMakefile.inSECURITY.mdconfigureconfigure.acdoc/Config.headerdoc/RELEASE-NOTES.mddoc/conf/examples/example.confdoc/conf/help/help.fr.confdoc/conf/modules.default.confdoc/conf/tls/tls.cnfextras/build-tests/nix/buildinclude/config.hinclude/dynconf.hinclude/h.hinclude/modules.hinclude/numeric.hinclude/struct.hobbyircd.insrc/aliases.csrc/api-clicap.csrc/api-efunctions.csrc/api-history-backend.csrc/conf.csrc/ircd.csrc/json.csrc/list.csrc/log.csrc/match.csrc/misc.csrc/modules/cap.csrc/modules/central-blocklist.csrc/modules/chanmodes/floodprot.csrc/modules/chanmodes/history.csrc/modules/chathistory.csrc/modules/chgname.csrc/modules/crule.csrc/modules/history.csrc/modules/history_backend_mem.csrc/modules/join.csrc/modules/jointhrottle.csrc/modules/json-log-tag.csrc/modules/link-security.csrc/modules/message-tags.csrc/modules/multiline.csrc/modules/nick.csrc/modules/no-implicit-names.csrc/modules/plaintext-policy.csrc/modules/quit.csrc/modules/reply-tag.csrc/modules/rpc/rpc.csrc/modules/rpc/spamfilter.csrc/modules/rpc/stats.csrc/modules/rpc/user.csrc/modules/server.csrc/modules/setname.csrc/modules/spamreport.csrc/modules/starttls.csrc/modules/stats.csrc/modules/tkl.csrc/modules/tkldb.csrc/parse.csrc/socket.csrc/support.csrc/tls.csrc/unrealircdctl.csrc/url_unreal.csrc/user.csrc/windows/makecert.batsrc/windows/unrealinst.iss
💤 Files with no reviewable changes (1)
- doc/conf/tls/tls.cnf
Comment on lines
+53
to
+58
| mkdir -p $BASEPATH | ||
| mkdir -p $TMPDIR | ||
| mkdir -p $PRIVATELIBDIR | ||
| mkdir -p $CONFDIR | ||
| chmod 0700 $BASEPATH | ||
| chmod 0700 $TMPDIR | ||
| chmod 0700 $CONFDIR |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Quote and fail-check directory path operations.
Line 53-Line 58 use unquoted path variables, so paths with spaces/globs can create or chmod unintended targets.
Suggested patch
-mkdir -p $BASEPATH
-mkdir -p $TMPDIR
-mkdir -p $CONFDIR
-chmod 0700 $BASEPATH
-chmod 0700 $TMPDIR
-chmod 0700 $CONFDIR
+mkdir -p "$BASEPATH" || exit 1
+mkdir -p "$TMPDIR" || exit 1
+mkdir -p "$CONFDIR" || exit 1
+chmod 0700 "$BASEPATH" || exit 1
+chmod 0700 "$TMPDIR" || exit 1
+chmod 0700 "$CONFDIR" || exit 1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| mkdir -p $BASEPATH | |
| mkdir -p $TMPDIR | |
| mkdir -p $PRIVATELIBDIR | |
| mkdir -p $CONFDIR | |
| chmod 0700 $BASEPATH | |
| chmod 0700 $TMPDIR | |
| chmod 0700 $CONFDIR | |
| mkdir -p "$BASEPATH" || exit 1 | |
| mkdir -p "$TMPDIR" || exit 1 | |
| mkdir -p "$CONFDIR" || exit 1 | |
| chmod 0700 "$BASEPATH" || exit 1 | |
| chmod 0700 "$TMPDIR" || exit 1 | |
| chmod 0700 "$CONFDIR" || exit 1 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Config` around lines 53 - 58, Quote all the path variables in the mkdir and
chmod commands (around lines 53-58) by wrapping $BASEPATH, $TMPDIR, and $CONFDIR
in double quotes to prevent word-splitting and glob expansion. Additionally, add
fail-checks after each mkdir command by appending || exit 1 to ensure the script
exits if any directory creation fails, rather than continuing with invalid
paths.
| echo "Press ENTER to continue" | ||
| read cc | ||
| $MAKE pem | ||
| ./unrealircd mkcert |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stop on certificate generation failure before printing success.
Line 194 runs ./unrealircd mkcert without checking status, then always prints success. A failed cert bootstrap currently looks successful.
Suggested patch
- ./unrealircd mkcert
- echo "Certificate created successfully."
+ if ./unrealircd mkcert; then
+ echo "Certificate created successfully."
+ else
+ echo "Certificate generation failed."
+ exit 1
+ fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Config` at line 194, The ./unrealircd mkcert command does not have its exit
status checked before printing a success message, meaning a failed certificate
generation will still appear successful. After executing the ./unrealircd mkcert
command, check its exit status and only print the success message if the command
succeeds. If the command fails, the script should handle the error appropriately
(typically by exiting with a non-zero status or printing an error message).
Comment on lines
+561
to
+565
| # Create the private library directory now with restrictive permissions. | ||
| # This must happen here rather than in the Makefile because the bundled | ||
| # libraries are installed into it during configure. It must also work when | ||
| # configure is run directly without ./Config having created it first. | ||
| mkdir -p "$PRIVATELIBDIR" && chmod 0700 "$PRIVATELIBDIR" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make private-libdir creation failure fatal during configure.
Line 565 currently continues even if mkdir/chmod fails, which can cause later bundled-library install steps to fail in less obvious ways.
Suggested patch
- mkdir -p "$PRIVATELIBDIR" && chmod 0700 "$PRIVATELIBDIR"
+ mkdir -p "$PRIVATELIBDIR" && chmod 0700 "$PRIVATELIBDIR" || AC_MSG_ERROR([Unable to initialize private library directory: $PRIVATELIBDIR])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Create the private library directory now with restrictive permissions. | |
| # This must happen here rather than in the Makefile because the bundled | |
| # libraries are installed into it during configure. It must also work when | |
| # configure is run directly without ./Config having created it first. | |
| mkdir -p "$PRIVATELIBDIR" && chmod 0700 "$PRIVATELIBDIR" | |
| # Create the private library directory now with restrictive permissions. | |
| # This must happen here rather than in the Makefile because the bundled | |
| # libraries are installed into it during configure. It must also work when | |
| # configure is run directly without ./Config having created it first. | |
| mkdir -p "$PRIVATELIBDIR" && chmod 0700 "$PRIVATELIBDIR" || AC_MSG_ERROR([Unable to initialize private library directory: $PRIVATELIBDIR]) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@configure.ac` around lines 561 - 565, The mkdir and chmod commands in the
private library directory creation block do not cause the configure script to
exit if they fail, which can lead to confusing errors later during bundled
library installation. Add error handling to the line containing mkdir -p
"$PRIVATELIBDIR" && chmod 0700 "$PRIVATELIBDIR" so that the configure script
immediately exits with a fatal error if either the directory creation or
permission change operation fails. This ensures failures are caught early and
reported clearly rather than silently allowing the script to continue with an
improperly configured directory.
Comment on lines
+920
to
+923
| " d | handshake flood | Exception de test de flood sur les données handhsake "; | ||
| " | | (pas de ZLINE si flood avant enregistrement) "; | ||
| " m | maxperip | Exeptmion de restricion allow::maxperip restriction "; | ||
| " r | antirandom | Exception sur le module antirandom "; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix typographical errors in the new ELINE bantype descriptions.
There are multiple spelling issues in this block (eg. handhsake, Exeptmion, restricion) that reduce operator clarity in a security-sensitive help section.
Suggested wording cleanup
- " d | handshake flood | Exception de test de flood sur les données handhsake ";
+ " d | handshake flood | Exception de test de flood sur les données handshake ";
" | | (pas de ZLINE si flood avant enregistrement) ";
- " m | maxperip | Exeptmion de restricion allow::maxperip restriction ";
+ " m | maxperip | Exception à la restriction allow::maxperip ";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| " d | handshake flood | Exception de test de flood sur les données handhsake "; | |
| " | | (pas de ZLINE si flood avant enregistrement) "; | |
| " m | maxperip | Exeptmion de restricion allow::maxperip restriction "; | |
| " r | antirandom | Exception sur le module antirandom "; | |
| " d | handshake flood | Exception de test de flood sur les données handshake "; | |
| " | | (pas de ZLINE si flood avant enregistrement) "; | |
| " m | maxperip | Exception à la restriction allow::maxperip "; | |
| " r | antirandom | Exception sur le module antirandom "; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@doc/conf/help/help.fr.conf` around lines 920 - 923, Fix the spelling errors
in the ELINE bantype descriptions block where handshake is misspelled as
"handhsake", Exception is misspelled as "Exeptmion", and restriction is
misspelled as "restricion". Locate these three typos in the lines containing the
d, m, and r bantype entries respectively and correct them to their proper
spellings to ensure clarity in this security-sensitive help documentation for
French-speaking operators.
| loadmodule "standard-replies"; /* Standard Replies */ | ||
| loadmodule "no-implicit-names"; /* Opt out of receiving implicit NAMES when joining a channel */ | ||
| loadmodule "extended-isupport"; /* draft/extended-isupport */ | ||
| loadmodule "multiline"; /* draft/multiline */ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the duplicate multiline load.
multiline is already loaded on Line 241, so this second default loadmodule can trigger duplicate-module configuration failures for fresh installs.
Proposed fix
-loadmodule "multiline"; /* draft/multiline */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@doc/conf/modules.default.conf` at line 261, Remove the duplicate loadmodule
directive for "multiline" in the configuration file. The multiline module is
already being loaded earlier in the configuration, so the second loadmodule
"multiline" statement causes a duplicate module configuration error during fresh
installs. Delete the entire line containing the duplicate loadmodule "multiline"
directive with the draft/multiline comment.
Comment on lines
+3304
to
+3315
| /* Config (and central) spamfilters are local per server, so they never receive | ||
| * a random network id. Give them a deterministic id instead, identical on every | ||
| * server: the config/central id label if set, otherwise a content hash. This is | ||
| * what lets a central spamfilter be referenced by the same id on every server. | ||
| */ | ||
| if (flags & (TKL_FLAG_CONFIG | TKL_FLAG_CENTRAL_SPAMFILTER)) | ||
| { | ||
| if (!BadPtr(id)) | ||
| strlcpy(tkl->id, id, sizeof(tkl->id)); | ||
| else | ||
| strlcpy(tkl->id, spamfilter_fallback_id(tkl), sizeof(tkl->id)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t silently discard local config spamfilter IDs.
This branch says config spamfilters use the provided id, but tkl_config_run_spamfilter() still clears id for non-central spamfilters before calling here. That means a valid local spamfilter::id is accepted, then replaced with the fallback hash; either pass it through or reject id outside central configs.
Proposed direction
- } else {
- id = NULL;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/tkl.c` around lines 3304 - 3315, The issue is that valid local
config spamfilter IDs are being silently replaced with a fallback hash because
tkl_config_run_spamfilter() clears the id parameter for non-central spamfilters
before calling this function. Either ensure that the id parameter is preserved
and passed through correctly from tkl_config_run_spamfilter() for local config
spamfilters (so the condition checking !BadPtr(id) in the current code block
actually receives the intended id), or alternatively add validation logic
outside this code block to explicitly reject id values that are provided for
non-central config spamfilters to prevent silent discarding of user-provided
IDs.
| TKL *tkl; | ||
| crule_context context; | ||
|
|
||
| *stop_processing_general_spamfilters = *stop_processing_central_spamfilters = 0; /* reset */ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve existing stop-processing state for rule-only filters.
Line 6119 resets the stop flags even when _match_spamfilter() calls this after normal spamfilters. A prior stop action or stop-on-first-match can therefore be ignored for the rule-only pass.
Proposed fix
- *stop_processing_general_spamfilters = *stop_processing_central_spamfilters = 0; /* reset */
for (tkl = tklines[tkl_hash('F')]; tkl; tkl = tkl->next)Also applies to: 6408-6411
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/tkl.c` at line 6119, The assignment statement resetting
stop_processing_general_spamfilters and stop_processing_central_spamfilters
unconditionally erases any stop state that was set during normal spamfilter
processing when rule-only filters are subsequently executed. Modify the reset
logic to only reset these flags on the initial call before processing normal
spamfilters, and preserve the existing stop-processing state if it has already
been set by a prior stop action or stop-on-first-match condition. Apply the same
preservation logic at lines 6408-6411 where this pattern also occurs.
Comment on lines
+569
to
+583
| /* id and spamfilter_id were added in TKLDB_VERSION 6260 */ | ||
| if (version >= 6260) | ||
| { | ||
| R_SAFE(unrealdb_read_str(db, &str)); | ||
| strlcpy(tkl->id, str, sizeof(tkl->id)); | ||
| safe_free(str); | ||
| R_SAFE(unrealdb_read_str(db, &str)); | ||
| strlcpy(tkl->spamfilter_id, str, sizeof(tkl->spamfilter_id)); | ||
| safe_free(str); | ||
| /* Hit-stat fields for all TKL types (hits, lasthit). */ | ||
| R_SAFE(unrealdb_read_int64(db, &v)); | ||
| tkl->hits = v; | ||
| R_SAFE(unrealdb_read_int64(db, &v)); | ||
| tkl->lasthit = v; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Backfill IDs when upgrading pre-6260 tkldb entries.
For version < 6260, tkl->id remains empty, and Lines 810-811 copy that empty value into the restored entry. Those entries will then be saved as 6260 without an ID, so existing persistent server bans/name bans never receive the new operator-visible TKL identity. Generate a fresh ID for added when the DB lacks one before the first 6260 rewrite.
Also applies to: 808-813
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/tkldb.c` around lines 569 - 583, For database entries with
version less than 6260, the tkl->id field is never populated and remains empty,
which causes empty IDs to be persisted when entries are later rewritten at lines
810-811. Add an else block after the version check (after the closing brace of
the if statement that checks version >= 6260) to generate a fresh ID for tkl
entries when the version is less than 6260. This ensures that existing
persistent server bans and name bans receive the new operator-visible TKL
identity before being saved as version 6260 during the first database rewrite.
Comment on lines
+858
to
+881
| /* set::server-linking::tls-options (free+NULL if no longer configured) */ | ||
| if (iConf.server_linking_tls_options) | ||
| { | ||
| tmp = init_ctx(iConf.server_linking_tls_options, 1); | ||
| if (!tmp) | ||
| { | ||
| unreal_log(ULOG_ERROR, "config", "TLS_RELOAD_FAILED", NULL, | ||
| "TLS Reload failed at set::server-linking::tls-options. See previous errors."); | ||
| return 0; | ||
| } | ||
| if (ctx_link_server) | ||
| SSL_CTX_free(ctx_link_server); | ||
| ctx_link_server = tmp; /* activate */ | ||
|
|
||
| tmp = init_ctx(iConf.server_linking_tls_options, 0); | ||
| if (!tmp) | ||
| { | ||
| unreal_log(ULOG_ERROR, "config", "TLS_RELOAD_FAILED", NULL, | ||
| "TLS Reload failed at set::server-linking::tls-options (client). See previous errors."); | ||
| return 0; | ||
| } | ||
| if (ctx_link_client) | ||
| SSL_CTX_free(ctx_link_client); | ||
| ctx_link_client = tmp; /* activate */ |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Swap server-linking TLS contexts only after both rebuilds succeed.
Line 870 activates ctx_link_server before the client context is built. If Line 872 then fails, rehash reports failure while incoming server-linking TLS already uses the new context and outgoing links keep the old/null one. Build both contexts into temporaries first, then swap after both succeed.
Proposed reload shape
if (iConf.server_linking_tls_options)
{
- tmp = init_ctx(iConf.server_linking_tls_options, 1);
- if (!tmp)
+ SSL_CTX *tmp_link_server = init_ctx(iConf.server_linking_tls_options, 1);
+ SSL_CTX *tmp_link_client = NULL;
+
+ if (!tmp_link_server)
{
unreal_log(ULOG_ERROR, "config", "TLS_RELOAD_FAILED", NULL,
"TLS Reload failed at set::server-linking::tls-options. See previous errors.");
return 0;
}
- if (ctx_link_server)
- SSL_CTX_free(ctx_link_server);
- ctx_link_server = tmp; /* activate */
- tmp = init_ctx(iConf.server_linking_tls_options, 0);
- if (!tmp)
+ tmp_link_client = init_ctx(iConf.server_linking_tls_options, 0);
+ if (!tmp_link_client)
{
+ SSL_CTX_free(tmp_link_server);
unreal_log(ULOG_ERROR, "config", "TLS_RELOAD_FAILED", NULL,
"TLS Reload failed at set::server-linking::tls-options (client). See previous errors.");
return 0;
}
+
+ if (ctx_link_server)
+ SSL_CTX_free(ctx_link_server);
+ if (ctx_link_client)
+ SSL_CTX_free(ctx_link_client);
+ ctx_link_server = tmp_link_server;
+ ctx_link_client = tmp_link_client;
- if (ctx_link_client)
- SSL_CTX_free(ctx_link_client);
- ctx_link_client = tmp; /* activate */
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /* set::server-linking::tls-options (free+NULL if no longer configured) */ | |
| if (iConf.server_linking_tls_options) | |
| { | |
| tmp = init_ctx(iConf.server_linking_tls_options, 1); | |
| if (!tmp) | |
| { | |
| unreal_log(ULOG_ERROR, "config", "TLS_RELOAD_FAILED", NULL, | |
| "TLS Reload failed at set::server-linking::tls-options. See previous errors."); | |
| return 0; | |
| } | |
| if (ctx_link_server) | |
| SSL_CTX_free(ctx_link_server); | |
| ctx_link_server = tmp; /* activate */ | |
| tmp = init_ctx(iConf.server_linking_tls_options, 0); | |
| if (!tmp) | |
| { | |
| unreal_log(ULOG_ERROR, "config", "TLS_RELOAD_FAILED", NULL, | |
| "TLS Reload failed at set::server-linking::tls-options (client). See previous errors."); | |
| return 0; | |
| } | |
| if (ctx_link_client) | |
| SSL_CTX_free(ctx_link_client); | |
| ctx_link_client = tmp; /* activate */ | |
| /* set::server-linking::tls-options (free+NULL if no longer configured) */ | |
| if (iConf.server_linking_tls_options) | |
| { | |
| SSL_CTX *tmp_link_server = init_ctx(iConf.server_linking_tls_options, 1); | |
| SSL_CTX *tmp_link_client = NULL; | |
| if (!tmp_link_server) | |
| { | |
| unreal_log(ULOG_ERROR, "config", "TLS_RELOAD_FAILED", NULL, | |
| "TLS Reload failed at set::server-linking::tls-options. See previous errors."); | |
| return 0; | |
| } | |
| tmp_link_client = init_ctx(iConf.server_linking_tls_options, 0); | |
| if (!tmp_link_client) | |
| { | |
| SSL_CTX_free(tmp_link_server); | |
| unreal_log(ULOG_ERROR, "config", "TLS_RELOAD_FAILED", NULL, | |
| "TLS Reload failed at set::server-linking::tls-options (client). See previous errors."); | |
| return 0; | |
| } | |
| if (ctx_link_server) | |
| SSL_CTX_free(ctx_link_server); | |
| if (ctx_link_client) | |
| SSL_CTX_free(ctx_link_client); | |
| ctx_link_server = tmp_link_server; | |
| ctx_link_client = tmp_link_client; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tls.c` around lines 858 - 881, The server-linking TLS contexts are being
activated immediately after each context is built separately. If the second
init_ctx call fails to build the client context, the server context is already
activated, creating an inconsistent state where incoming server links use the
new context but outgoing links keep the old one. Instead, build both the server
context and client context using init_ctx into temporary variables first
(without activating them), verify that both init_ctx calls succeed, and only
then free the old contexts and assign the new temporaries to ctx_link_server and
ctx_link_client. This ensures both contexts are successfully built before any
activation occurs.
Comment on lines
+1163
to
1168
| /* This is only ever compared with != (a change counter), so we just need a | ||
| * wrap point high enough that it never realistically aliases within a session. | ||
| */ | ||
| client->local->tags_serial++; | ||
| if (client->local->tags_serial > 10000) | ||
| if (client->local->tags_serial >= INT_MAX) | ||
| client->local->tags_serial = 0; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check for wrap before incrementing tags_serial.
Line 1166 increments first, so if tags_serial is already INT_MAX, the signed addition overflows before Line 1167 can reset it.
Proposed fix
- client->local->tags_serial++;
- if (client->local->tags_serial >= INT_MAX)
+ if (client->local->tags_serial >= INT_MAX - 1)
client->local->tags_serial = 0;
+ else
+ client->local->tags_serial++;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /* This is only ever compared with != (a change counter), so we just need a | |
| * wrap point high enough that it never realistically aliases within a session. | |
| */ | |
| client->local->tags_serial++; | |
| if (client->local->tags_serial > 10000) | |
| if (client->local->tags_serial >= INT_MAX) | |
| client->local->tags_serial = 0; | |
| /* This is only ever compared with != (a change counter), so we just need a | |
| * wrap point high enough that it never realistically aliases within a session. | |
| */ | |
| if (client->local->tags_serial >= INT_MAX - 1) | |
| client->local->tags_serial = 0; | |
| else | |
| client->local->tags_serial++; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/user.c` around lines 1163 - 1168, The issue in the tags_serial management
code is that the increment operation (client->local->tags_serial++) occurs
before the wrap check, causing signed integer overflow if tags_serial already
equals INT_MAX. To fix this, reverse the order of operations by checking whether
tags_serial is >= INT_MAX before incrementing it, ensuring the reset to zero
happens first to prevent the overflow from occurring.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
44 upstream commits. Fork gains the
server_flood_count()/total_channel_flood_count()crules the test harness now requires, so Linux CI passes without pinning the harness.TKL hits/id taken from upstream; version kept at ObbyIRCd 6.2.6.
Summary by CodeRabbit
New Features
Changes
./unrealircd mkcertcommand instead ofmake pemFixes