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

Various command parsers do not handle a colon before the last parameter #2271

Closed
linuxdaemon opened this issue Nov 17, 2018 · 8 comments
Closed
Labels

Comments

@linuxdaemon
Copy link
Contributor

@linuxdaemon linuxdaemon commented Nov 17, 2018

This issue was first noticed when testing Hexchat with Inspircd v3, channel topic times keep showing as 0 in unix time. CHGHOST also does not seem to remove the colon before the last parameter resulting in a user's host being set to @:<newhost>.
Inspircd 3.0 changed its message formats a bit, resulting in a colon being prepended to the last parameter in every message from the server. According to the IRC RFCs, clients should generally just ignore the colon before the last parameter in all cases.

Versions

HexChat: 2.12.4 and 2.14.1
InspIRCd built from commit https://github.com/inspircd/inspircd/tree/37977710a79472741c043b2dc8c17f520429c9f4

@TingPing TingPing added the bug label Nov 17, 2018
@TingPing
Copy link
Member

@TingPing TingPing commented Nov 17, 2018

Known issue for a while, it makes a lot of bad assumptions. All the current parsing is just bad and touching it would be painful.

I wrote a new one here: https://github.com/TingPing/irc-client/blob/master/lib/irc-message.c

And while it would be an equally painful task to use it I think it would probably have the best results.

Loading

@SadieCat
Copy link
Contributor

@SadieCat SadieCat commented Nov 17, 2018

This is caused by incorrect protocol message parsing. According to the notes of section 2.3.1 of RFC 1459 a <trailing> parameter should be treated as equivalent to a <middle> parameter but HexChat does not do this.

You can test this behaviour on testnet.inspircd.org.

Loading

@prawnsalad
Copy link

@prawnsalad prawnsalad commented Jan 4, 2019

+1
I'm starting to see more people report this error when connecting to insp3 testnets now it's nearing RC :(

Loading

@TingPing
Copy link
Member

@TingPing TingPing commented Jan 4, 2019

I've been quite busy but if you point out specific commands where you are hitting its going to be quicker to fix those. I know the entire parser is incorrect but that is a bigger change that won't happen in 10 minutes of free time.

You can also submit a PR that would look like: 7d78c6b

Loading

linuxdaemon added a commit to linuxdaemon/hexchat that referenced this issue Jan 5, 2019
Make sure trailing parameters are treated the same as other parameters
to avoid issues like hexchat#2271

Fixes hexchat#2271
linuxdaemon added a commit to linuxdaemon/hexchat that referenced this issue Jan 5, 2019
Make sure trailing parameters are treated the same as other parameters
to avoid issues like hexchat#2271

Fixes hexchat#2271
@linuxdaemon
Copy link
Contributor Author

@linuxdaemon linuxdaemon commented Jan 5, 2019

The specific commands I've observed breaking are:

  • 333 (topictime)
  • CHGHOST
  • MODE specifically a : before a parameter
  • ACCOUNT
  • The ban/exception lists, timestamp is not handled correctly

This is all I could find so far, #2294 would be the best option but as you said there, it does break API compat

Loading

@linuxdaemon
Copy link
Contributor Author

@linuxdaemon linuxdaemon commented Jan 28, 2019

It appears 341 breaks as well [11:21:17] --- You're inviting ChanServ to :##test (irc.example.com)

Loading

@TingPing
Copy link
Member

@TingPing TingPing commented Jan 28, 2019

Can you just do one line fixes for those events and open a PR?

Loading

TingPing added a commit that referenced this issue Jan 31, 2019
Partial fix for #2271 

This isn't an exhaustive list, but it's everything I could find. The bug still exists in the parser though, this is just a workaround for the moment
@H7-25
Copy link
Contributor

@H7-25 H7-25 commented May 10, 2019

When a release including this fix ?

Loading

TingPing added a commit that referenced this issue Dec 21, 2019
Partial fix for #2271 

This isn't an exhaustive list, but it's everything I could find. The bug still exists in the parser though, this is just a workaround for the moment
@TingPing TingPing closed this Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linked pull requests

Successfully merging a pull request may close this issue.

5 participants