Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UHNAMES Race Condition #2289

Closed
half-duplex opened this issue May 29, 2022 · 5 comments · Fixed by #2321
Closed

UHNAMES Race Condition #2289

half-duplex opened this issue May 29, 2022 · 5 comments · Fixed by #2321
Labels
Bug Things to squish; generally used for issues Core/IRC Protocol Handling
Milestone

Comments

@half-duplex
Copy link
Member

half-duplex commented May 29, 2022

Description

When Sopel joins a server that supports UHNAMES but not the userhost-in-names CAP, it sends PROTOCTL UHNAMES to accomplish the same goal.

In my testing, this results in:

>> CAP LS 302
>> NICK Sopel
>> USER ...
<< 001 ...
>> JOIN #test
<< 005 [...] UHNAMES
>> PROTOCTL UHNAMES
<< JOIN #test
<< 353 Sopel = #test :@Op Sopel

Followed by an exception (expected 2 to unpack, got 1) on coretasks.py:524

Sending PROTOCTL UHNAMES before the JOIN seems to work.

The same problem may exist with the CAP, if not in practice, than in theory.

Previously: #2102

Reproduction steps

  1. Find a server with UHNAMES but no userhost-in-names
  2. Configure Sopel to connect to it and join a channel
  3. Start sopel
  4. Note exception

Expected behavior

Joins without exception

Environment

  • Sopel .version: 36b7c2f
  • Sopel installed via: pip
  • Python version: 3.9.2
  • Operating system: Debian 10
  • IRCd /version: InspIRCd 2

Notes

@dgw broke it

@half-duplex half-duplex added Bug Things to squish; generally used for issues Core/IRC Protocol Handling labels May 29, 2022
@half-duplex half-duplex added this to the 8.0.0 milestone May 29, 2022
@Exirel
Copy link
Contributor

Exirel commented May 29, 2022

I can see a quick fix, that would probably be a safe fix as well: try/catch.

@dgw
Copy link
Member

dgw commented May 29, 2022

The best solution would simply prevent sending any JOINs until after 005 has been processed.

I would love to just move a bunch of startup tasks from 001 to 251, but unfortunately the latter is no longer required. We could move joining channels to a new handler that triggers on 376 (RPL_ENDOFMOTD) or 422 (ERR_NOMOTD), since the server "MUST" send either the MOTD or 422 after 001-005. We already constrain ISUPPORT handling to happen on the main thread, so that should block until after PROTOCTL UHNAMES has been sent, after which it will be fine to send JOINs. (Can also try/except the line that currently errors, for maximum safety.)

The advantage of triggering JOINs and such on 001 is that it's existed in the spec for ages. As unlikely as it would be to encounter an IRCd that doesn't send 005, or doesn't send the MOTD automatically after registration, it's far more unlikely to find a server that doesn't send 001—that would completely break the protocol, as I understand it.

As an aside, I have no idea why this rule exists on the 005 handler. It shouldn't be necessary. Though I guess it's a guard against super outdated IRCds that still use 005 as RPL_BOUNCE, per RFC 2812.

dgw added a commit that referenced this issue Jun 25, 2022
This doesn't fix the noncompliant server, but it will keep Sopel from
dying early and skipping the rest of the items in a given malformed
RPL_NAMREPLY line.

Workaround (but not a "fix" per se) for #2289 and #2311.
@Exirel
Copy link
Contributor

Exirel commented Jul 14, 2022

We could move joining channels to a new handler that triggers on 376 (RPL_ENDOFMOTD) or 422 (ERR_NOMOTD), since the server "MUST" send either the MOTD or 422 after 001-005.

That shouldn't be too much of a trouble, right? Right... I'll try.

@Exirel
Copy link
Contributor

Exirel commented Jul 14, 2022

So, hm... you're not going to believe that: it's stupidly simple. All I have to do is move a portion of the code into its own plugin callable in coretasks. Everything else is just about managing the automated tests.

@Exirel
Copy link
Contributor

Exirel commented Jul 14, 2022

@half-duplex I invite you to take a look at #2321 as it should fix this issue, however I don't have a server to test the PR against.

@dgw dgw closed this as completed in #2321 Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Things to squish; generally used for issues Core/IRC Protocol Handling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants