kristianoye / kmud

LPC/MudOS-inspired Node-based MUD
10 stars 0 forks source link

Mud client issues (assumption on cr/lf) #4

Closed LimpingNinja closed 5 years ago

LimpingNinja commented 5 years ago

When connecting with certain mud clients (GMud32 for example) the socket code appears to make some assumptions about line-endings. When logging on if I type "limpingninja" it returns: "limpingninj" and in single character selection menus ([y/n] it never gets input) as it appears to be pulling len-2 and probably only getting a single CR or similar. I plan on testing a little today just to see where the failure is, but haven't dug too deeply yet.

LimpingNinja commented 5 years ago

It appears to be as I expected:

RanvierTelnet.js On entry, databuf (toString()) = "Test\n"

The loop on line 252-262 checks for \n, pushing each char to the buffer databuf. This leads to databuf being: "Test".

            // If multiple commands were sent \r\n separated in the same packet process
            // them separately. Some client auto-connect features do this
            let bucket = [];
            for (let i = 0; i < inputlen; i++) {
                if (databuf[i] === 3) { // ctrl-c 
                    continue; //eat it
                }
                if (databuf[i] !== 10) { // \n
                    bucket.push(databuf[i]);
                } else {
                    this.input(Buffer.from(bucket));
                    bucket = [];
                }
            }

            if (bucket.length) {
                this.input(Buffer.from(bucket));
            }

When this is then parsed for negotiations, in input the string is chopped by one character: Line 471 of RanvierTelnet.js:

this.emit('data', cleanbuf.slice(0, cleanlen - 1));

These leaves the emitted result as "Tes"

I'm not sure this is only common to GMud, I believe I had this problem when implementing my own socket library previously. While the newest telnet specification says you should only be parsing CRLF in line mode, it does mention that LF \n alone is questionable (saying prefer not) but that older specifications didn't really call it out. For interoperability on Mud Clients it might be something to consider.

RFC5198 Appendix B

The historical NVT documents do not call out either "bare LF" (LF without CR) or HT for special treatment. Both have generally been understood to be problematic. In the case of LF, there is a difference in interpretation as to whether its semantics imply "go to same position on the next line" or "go to the first position on the next line" and interoperability considerations suggest not depending on which interpretation the receiver applies. At the same time, misinterpretation of LF is less harmful than misinterpretation of "bare" CR: in the CR case, text may be erased or made completely unreadable; in the LF one, the worst consequence is a very funny-looking display. Obviously, HT is problematic because there is no standard way to transmit intended tab position or width information in running text. Again, the harm is unlikely to be great if HT is simply interpreted as one or more spaces, but, in general, it cannot be relied upon to format information.

For accessibility, though, and as I previously said, I do fall into the camp of accepting \n by itself or, if the MUD does not have any specific character-based terminal actions even the stripping of \r completely and processing of \n by itself.

LimpingNinja commented 5 years ago

This is a naive patch:

diff --git a/src/network/clients/telnet/RanvierTelnet.js b/src/network/clients/telnet/RanvierTelnet.js
index da04722..165082b 100644
--- a/src/network/clients/telnet/RanvierTelnet.js
+++ b/src/network/clients/telnet/RanvierTelnet.js
@@ -254,6 +254,9 @@ class TelnetSocket extends EventEmitter {
                 if (databuf[i] === 3) { // ctrl-c
                     continue; //eat it
                 }
+                if (databuf[i] === 13) { // \r
+                    continue; //eat it
+                }
                 if (databuf[i] !== 10) { // \n
                     bucket.push(databuf[i]);
                 } else {
@@ -468,7 +471,7 @@ class TelnetSocket extends EventEmitter {
          * @event TelnetSocket#data
          * @param {Buffer} data
          */
-        this.emit('data', cleanbuf.slice(0, cleanlen - 1));
+        this.emit('data', cleanbuf);
     }
 }

It could also be that you might want to save any additional \r so just modifying the this.emit('data', cleanbuf.slice(0, cleanlen - 1)); to add a wrapper check for a [13] as the last character in the buf and stripping it could work too without adding snarf logic to === 13.

The other option is to test for \r as last character on line 474. This of course leaves a dilemma in how to handle the rest of the assumptions on \r\n with telnet IAC since GMud actually - I will test those out later

LimpingNinja commented 5 years ago

Here is a proper pull request from the original project that has never been absorbed into the original or forks: RanvierMUD/ranvier-telnet pull#5 Fix: Handle any combination or order of CR and LF as command delimiters

kristianoye commented 5 years ago

I tried integrating the change but I ran into some double-buffering in the MUD shell from my Linux box (which is firing the data event twice... presumably once for CR and once for LF?). That is with me only playing with the patch for 30 seconds though so I will have to reserve comments for now). Also, CTRL-C still borks the connection on a Linux telnet session. Non-HTTP clients are lower priority for me though (although super handy for debugging).

LimpingNinja commented 5 years ago

I did some tests, it seems like you tried my patch vs the upstream pull request; I'll fork you and drop the upstream pull request in and then do some tests to verify.

I'll test all of the above and if it works drop you a pull request. I get it's lower priority - but since even now the ubiquity of text clients in mudding is high, it'll be good to have in.

Offtopic: Trying to create a base-lib do you have any clean driver applies mapping so I can baseline?

LimpingNinja commented 5 years ago

PR #5 should satisfy and I've done all tests with the exception of TinTin.

kristianoye commented 5 years ago

Thanks for that. The merge looks good. I haven't done any widespread client testing (I briefly tested with zMUD but am used to Linux telnet). I will work on trying to better document the supported driver applies but I've been trying to re-think my own lib's master object and configuration files (which are kind of a hot mess right now). I am kind of new to the whole npm ecosystem or I probably would have split the mudlib and the game driver into separate packages.