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.

Sign up for GitHub

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

Jump to bottom

telnet: Quote IAC on sending #42

Merged
merged 1 commit into from
Jun 8, 2023
Merged

telnet: Quote IAC on sending #42

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion telnet.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,33 @@

static ssize_t telnet_write(struct ios_ops *ios, const void *buf, size_t count)
{
return write(ios->fd, buf, count);
size_t handled = 0;
ssize_t ret;
void *iac;

/*
* To send an IAC character in the data stream, two IACs must be sent.
* So find the first IAC in the data to be send (if any), send the data
* before that IAC unquoted, then send the double IAC. Repeat until
* all IACs are handled.
*/
while ((iac = memchr(buf + handled, IAC, count - handled)) != NULL) {
if (iac - (buf + handled)) {
ret = write(ios->fd, buf + handled, iac - (buf + handled));
if (ret < 0)
return ret;
handled += ret;
} else {
dprintf(ios->fd, "%c%c", IAC, IAC);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will dprintf handle EINTR and incomplete writes correctly?

It took me some time to understand what this code should do. Perhaps it would be better to first quote the IAC in a local buffer (which can't fail) and then use a generic retrying send helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete writes are a principle problem before and after my patch also in the other backends. See also issue #21.

The problem with quoting the IAC first is that the buffer is provided by mux.c, so the telnet code would need to allocate memory (which complicates the code further).

I added a few comments to hopefully simplify understanding the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why must only the first IAC be quoted?

Copy link
Contributor Author

@ukleinek ukleinek Jun 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why must only the first IAC be quoted?

Hmm, should I add "Iterate for all IACs in the input" to the comment? In the code all IACs are handled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, the comment confused me. Being explicit about iteration sounds good.

I still think this would be simpler by just allocating count*2, handling the quoting and then one write. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think this would be simpler by just allocating count*2, handling the quoting and then one write. :)

I found another difficulty with this approach: I want to keep the return code semantic as is. This is necessary to later properly handle partial writes. If telnet_write is called with (say) 40 bytes to write, there are 3 IACs in the input, so in the end write(ios->fd, localbuf, 43); is called. If that write returns 20, it's not trivial to determine what to return. In the end this has the downside to allocate extra memory and isn't easier than the approach I chose now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any way this function would return with a partial write in a non-error-case.

In your example above, the function would not return, keep retrying. The final return value seems to be either negative or count. As count is already known to the caller, there is not much benefit in returning it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is to not block in .write() to keep microcom responsive to user input (and maybe other events) and so I want to do completion of writes in the core instead of in the callback directly.
Once this works, .write() can also become simpler and look e.g. like this:

if (count == 0)
    return 0;
if (buf[0] == IAC) {
    return write_full(ios->fd, "%c%c", IAC, IAC); // TODO: better handle partial write
} else {
    const char *iac = memchr(buf, IAC, count);
    if (!iac)
        iac = buf + count;
    return write(ios->fd, buf, iac - buf);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. As long as you may need to write more than one byte in the lower level write, you may need to block. Perhaps split the quoting from the write function? The quoting function would append to a buffer and the core could handle draining the buffer asynchronously (i.e. via poll()/select()), perhaps using libuv/libevent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the eventual plan is something like that. However I hesitate to add a dependency like libuv or libevent, so I tend to do it directly with poll() or select() (or maybe epoll()).

handled += 1;
}
}

/* Send the remaining data that needs no quoting. */
ret = write(ios->fd, buf + handled, count - handled);
if (ret < 0)
return ret;
return ret + handled;
}

static ssize_t telnet_read(struct ios_ops *ios, void *buf, size_t count)
Expand Down