Repository navigation
IPcheck: unit + integration tests; fix exempt-address crash, rehash, and 16-bit/unsigned arithmetic bugs - #102
Merged
Conversation
…ic bugs
Found by the new IPcheck unit and integration tests.
- Exempt addresses (IPCheck { except ... }) were accepted without being
recorded, but the client was still marked IPChecked, so
IPcheck_connect_succeeded() reported a stale entry and
IPcheck_disconnect() decremented a count it never incremented. With an
entry present for the address (clients connected before the exemption,
or remote users), the third exempt disconnect tripped
"Assertion failure at IPcheck.c:665: entry->connected > 0" and aborted
the server; with asserts off the count wrapped and Client-block maxlinks
stopped working. Remote exempt clients were flagged the same way.
IPcheck_local_connect() now returns IPCHECK_REFUSED / IPCHECK_COUNTED /
IPCHECK_EXEMPT and callers mark the client IPChecked only when it was
counted; ip_registry_check_remote() sets the flag only after the invalid
and exempt early-outs; the post-hooks (connect_succeeded, connect_fail)
are guarded by IsIPChecked() like IPcheck_disconnect() already was.
- Removing the IPCheck block from the config and rehashing left the old
exemptions in force: IPcheck_clear_config() was only called while
parsing an IPCheck block. Call it from read_configuration_file().
- CONNECTED_SINCE() subtracted two 16-bit timestamps in int, so it went
negative for up to 65536 s after CurrentTime crossed a multiple of 65536
(every 18.2 hours): the clone period never reset, entries did not expire
and free-target regeneration underflowed. Reduce modulo 2^16.
- With no free targets left, `CurrentTime - (TARGET_DELAY * free_targets
- 1)` was computed in unsigned int, giving CurrentTime - 4294967295 on
64-bit time_t instead of CurrentTime + 1: a client returning from an
address that had exhausted its targets got a full set instead of none.
- The remote-connect overflow guard refused the client but left the
connected counter wrapped to zero.
ircd/test/ipcheck_t.c (make check) drives ircd/IPcheck.c through its public API with a fake clock and feature values: clone limit and period (including refused attempts restarting the period), address independence, boot grace (IPCHECK_CLONE_DELAY), connect_fail() undo, disconnect accounting and the reset on last disconnect, exemptions (result code, no flag, no interference with an existing entry, cleared config), remote vs. burst introductions, IPv6 /64 keying and the /48 limit, IPv4 canonical form, free-target inheritance / pinning / regeneration / long-connection bonus, the expiry pass, the 16-bit clock wrap and the connected-counter overflow guard. tests/ipcheck/ (docker): - test_ipcheck_limits.py on the limits server, driving the limits with SET (PRIV_SET granted by rehash): registry notice contents, throttle ERROR before any input, recovery after the period, refused attempts restarting the period, slot release on disconnect, boot grace, IPCheck except block via rehash (and its removal), Client-block maxlinks enforced from the registry. - test_ipcheck_remote.py on the hub: a P10-introduced user with the test host's address counts toward (and is released from) the address, one with another address does not. P10Server.introduce_user() gains an ip parameter for this.
Contributor
Author
|
Regression run over the suites that share the touched paths ( The failure is Also confirmed after the fixes: |
ip_registry_check_remote() sets the IPChecked flag before the connected-counter overflow guard. When the guard refuses the client the caller kills it, and exit_client() then calls IPcheck_disconnect(), which releases a slot the client never took. Clear the flag on that path. The iauth IP override path, which ignores the return value, is covered by the same change.
The limits server ran with the default IPcheck settings (4 attempts per 40 s, enforced once the server is 600 s old). All test clients share the docker gateway address, so once a session kept the server up past IPCHECK_CLONE_DELAY the IPcheck tests' own fixture connections were throttled: 7 of 8 tests in test_ipcheck_limits.py errored in setup after 610 s of uptime, and the address was left throttled for later tests. Set IPCHECK_CLONE_LIMIT to 1000 as ircd-hub.conf does; the IPcheck tests lower it with SET, and RESET/REHASH now fall back to the safe value.
last_connect kept only the low 16 bits of CurrentTime, so any idle time of 18.2 hours or more aliased to a short one: an address idle for 65537 s looked 1 s old, its attempt counter was not reset and its free targets were not regenerated. Both registry structs already reserve a full int for the field, so keep the low 32 bits instead, at no memory cost, and do the subtraction modulo 2^32. Rework test_clock_wrap accordingly: add an idle-for-2^16+1-seconds case (which fails with 16-bit stamps), move the boundary cases to 2^32, and drop the comment that described the old failure mode backwards.
read_configuration_file() cleared the IPCheck exemptions before init_lexer(), so a rehash that could not open the config file dropped every exemption until the next successful rehash. Clear them only once the file is open. The parser also still cleared them at the start of each IPCheck block, so a second block (e.g. from an include) silently discarded the first block's exemptions. With the clear now done once per read, drop the per-block one so blocks add up. test_except_blocks_accumulate covers this and fails with the per-block clear restored.
test_refused_attempt_restarts_period retried CLONE_PERIOD - 1 seconds after a refusal, which is still inside the period counted from the last accepted connect, so it passed whether or not refusals restart the period. Retry twice, half a period apart: the last retry is more than a period after the last accepted connect but only half a period after the previous refusal, so it is refused only if refusals stamp the entry. (Verified to fail when the reject path keeps the old last_connect.)
The exempt path returned IPCHECK_EXEMPT but the other paths still returned the literals 1 and 0, which only work because they happen to equal IPCHECK_COUNTED and IPCHECK_REFUSED. No functional change.
The check compared the patched config against the snapshot, which always differs because _grant_set() has already edited it, so a changed Client line would have gone unnoticed until a confusing failure later on.
In DEBUGMODE builds the 65535-connect overflow loop printed a Debug() line per connect, about 3.5 MB per run into ipcheck_t.log. Add an opt-in test_debug_quiet flag to the shared test stub (default off, so other tests are unchanged) and set it around the loop.
IPcheck_except() allocates with MyMalloc() but IPcheck_clear_config() released with plain free(), which breaks MDEBUG (--with-leak-detect) builds where MyMalloc() hands out a pointer behind a debug header. Clearing now runs on every config read, so match the allocator.
Follow the tests/prNN_name/ layout used for PR test suites.
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
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.
Summary
Adds comprehensive tests for
ircd/IPcheck.c(there were none) and fixes the five bugs they surfaced. Two commits: fixes (74307e5) and tests (2d57779).Bugs fixed (
74307e5)IPCheck { except ... }addresses are accepted without being recorded, but the client was still markedIPChecked, soIPcheck_connect_succeeded()reported a stale entry andIPcheck_disconnect()decremented a count it never incremented. With an entry present for the address (clients connected before the exemption, or remote users) the third exempt disconnect hitAssertion failure at IPcheck.c:665: "entry->connected > 0"and aborted the server (reproduced in the docker harness:ircu-limits exited 134); with asserts off the counter wraps and Client-blockmaxlinksstops working. Remote exempt clients were flagged the same way.IPcheck_local_connect()now returnsIPCHECK_REFUSED/IPCHECK_COUNTED/IPCHECK_EXEMPT; callers (s_bsd.c,websocket.c,s_auth.c) mark the clientIPCheckedonly when counted;ip_registry_check_remote()sets the flag only after the invalid/exempt early-outs;connect_succeeded/connect_failcall sites are guarded byIsIPChecked()asIPcheck_disconnect()already was.IPCheckblock (IPcheck_clear_config()was only called while parsing one). Now called fromread_configuration_file().CONNECTED_SINCE()went negative for up to 65536 s afterCurrentTimecrossed a multiple of 65536 (every 18.2 h): clone period never reset, entries never expired, free-target regeneration underflowed. Reduced modulo 2^16.CurrentTime - (TARGET_DELAY * free_targets - 1)was evaluated inunsigned int, yieldingCurrentTime - 4294967295on 64-bittime_tinstead ofCurrentTime + 1.connectedwrapped to 0.Behaviour changes worth noting: exempt clients no longer receive the (stale)
on N ca M(L) ...notice at all; clients reconnecting from an address that exhausted its targets now get none, as the comments inip_registry_disconnect()intend.Tests (
2d57779)ircd/test/ipcheck_t.c(make check): fake clock + feature values; clone limit/period (incl. refused attempts restarting the period), address independence, boot grace,connect_failundo, disconnect accounting and reset-on-last-disconnect, exemptions (result code, no flag, existing entry untouched, config clear), remote vs. burst, IPv6 /64 keying and /48 limit, IPv4 canonical form, free-target inheritance/pinning/regeneration/bonus, expiry pass, clock wrap, counter overflow.tests/ipcheck/test_ipcheck_limits.py(limits server,SET-driven): registry notice contents, throttleERRORbefore any input, recovery after the period, refused-attempt period restart, slot release, boot grace,IPCheckexcept block via rehash and its removal, Client-blockmaxlinksenforced from the registry.tests/ipcheck/test_ipcheck_remote.py(hub): a P10-introduced user with the test host's address counts toward and is released from that address; another address does not.P10Server.introduce_user()gainsip=.Verification
make -C ircd/test check: 7/7tests/ipcheck/: 10/10 (docker)class_limits,pr_websocket,cidr_glines,pr69_exempt_throttle: running at PR time; result will be posted as a comment.