View Issue Details

IDProjectCategoryView StatusLast Update
0000892LDMud 3.6Networkingpublic2022-01-09 20:36
Reporterfufu Assigned Tofufu  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Fixed in Version3.6.5 
Summary0000892: Newline characters may be lost in switch to INPUT_CHARMODE handling
DescriptionAssume we want to read some data, a line of text followed by a single character:

  ... input_to("fun1"); ...
  fun1(string str) { input_to("fun2", INPUT_CHARMODE); }
  fun2(string str) { ... }

If we send "a\n\nb" to the server in one packet, then "fun2" will never be executed. What happens under the hood is a bit convoluted:

  1. get_message() receives 4 bytes of data, passes this off to telnet_neg() (comm.c line 2836), which consumes 2 bytes of data and prepares a line containing "a" in the command buffer
  2. still in get_message(), the command buffer is copied to the output buffer and then telnet_neg() is invoked again to "reinitialize the telnet machine" (comm.c line 3000). Note that we're still in line mode, so the newline is consumed and an empty line is prepared in the command buffer.
  3. the first input_to() completes, the second is issued.
  4. in get_message(), now we're in char mode, and the code will see that the telnet state machine is ready, and then fail to extract a character (comm.c line 2892). Nothing happens; we're stuck in this state forever.
Steps To ReproduceI'll attach a testcase.
TagsNo tags attached.

Activities

fufu

2021-04-17 00:50

manager   ~0002622

t-0000892.c (1,052 bytes)   
#include "/inc/base.inc"
#include "/inc/client.inc"

#include "/sys/input_to.h"

bytes b(string s)
{
    return to_bytes(s, "ISO-8859-1");
}

void run_server()
{
    binary_message(b"a\n\nb");
}

void run_client()
{
    input_to("client_input1");
    call_out("end_client", __ALARM_TIME__);
}

int step;

void client_input1(string str)
{
    step = 1;
    if (str != "a") {
        msg("FAILURE: Received %O instead of %O.\n", str, "a");
        shutdown(1);
    }
    input_to("client_input2", INPUT_CHARMODE);
}

void client_input2(string str)
{
    step = 2;
    if (str != "\n") {
        msg("FAILURE: Received %O instead of %O.\n", b(str), b("\n"));
        shutdown(1);
    } else {
        msg("Success!\n");
        shutdown(0);
    }
}

void end_client()
{
    msg("FAILURE: Received %d/2 messages.\n", step);
    shutdown(1);
}

void run_test()
{
    msg("\nRunning test for #0000892:\n"
          "--------------------------\n");

    connect_self("run_server", "run_client");
}

string *epilog(int eflag)
{
    run_test();
    return 0;
}
t-0000892.c (1,052 bytes)   

fufu

2021-04-17 01:00

manager   ~0002623

There are quite a few things that are odd about this:

- telnet_neg() gets ahead of the data the driver has actually consumed
- telnet_neg() is responsible for stripping newlines for line mode input
- NUL is somehow treated as a newline character
- as far as I can see, charmode reads can never recover from that empty command buffer state; arguably they should try to get more input if that state ever occurs

Changing either of the first two points should solve the issue at hand. But they both feel wrong to me.

zesstra

2021-04-17 07:55

administrator   ~0002624

Last edited: 2021-04-17 07:59

The telnet convention concerning end-of-line is kind of difficult. There are some clients, that do not send the usual CR LF sequence, but CR NUL as EOL sequence. I could imagine, this being related to treating NUL as EOL.
This document also discusses EOL stuff and mentions that in character mode CR will be passed to the application when in "raw mode" - maybe your NUL was CR NUL and only the NUL remained for interpretation?
https://www.freesoft.org/CIE/RFC/1123/31.htm
(Originally, the idea behind CR NUL seems to be to send only a CR, but I guess, at some point the implementations interpreted that as EOL, because some clients never sent CR LF.)

BTW: could you try to switch to BINARY mode - this may enable yet another combination of problems with EOL. (e.g. I noticed that Tinyfugue in BINARY LINEMODE with a current driver will change the line, but not perform the carriage return... But no idea if that is our fault or TF).

Jof, how I wish we could leave stuff like this to a library!

Gnomi

2021-04-18 18:26

manager   ~0002625

I tried to get rid of the second telnet_neg() call in get_message() a few years ago. (I found it odd and just removed it.) And then something broke. I don't know the details anymore, but the second call was somehow important. Maybe we could try to move it to the top of the function, before the select call.

I think it's correct that telnet_neg() does the newline stripping as newlines are part of the telnet protocol. There is information about that in the state (TS_READY: newlines were stripped, TS_CHAR_READY: they weren't). I would argue, that it is a mistake, that TS_READY occurs in charmode. This also points to postponing the second telnet_neg() call.

fufu

2021-04-18 18:33

manager   ~0002626

See also https://github.com/ldmud/ldmud/pull/61

fufu

2021-04-18 18:36

manager   ~0002627

@Gnomi, indeed that second call to telnet_neg() was odd, but that was because telnet_neg() served two purposes, 1) reset the telnet machinery after processing a line/character of input and 2) advance the telnet negotiations until the next line of input is available. The patch I produced splits it into two functions for that reason.

Issue History

Date Modified Username Field Change
2021-04-17 00:49 fufu New Issue
2021-04-17 00:50 fufu Note Added: 0002622
2021-04-17 00:50 fufu File Added: t-0000892.c
2021-04-17 01:00 fufu Note Added: 0002623
2021-04-17 01:00 fufu Assigned To => fufu
2021-04-17 01:00 fufu Status new => assigned
2021-04-17 07:55 zesstra Note Added: 0002624
2021-04-17 07:59 zesstra Note Edited: 0002624
2021-04-18 18:26 Gnomi Note Added: 0002625
2021-04-18 18:33 fufu Note Added: 0002626
2021-04-18 18:36 fufu Note Added: 0002627
2022-01-09 20:36 Gnomi Project LDMud => LDMud 3.6
2022-01-09 20:36 Gnomi Status assigned => resolved
2022-01-09 20:36 Gnomi Resolution open => fixed
2022-01-09 20:36 Gnomi Fixed in Version => 3.6.5