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

telnet: Quote IAC on sending #42

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

ukleinek
Copy link
Contributor

@ukleinek ukleinek commented Jun 4, 2023

To send a single IAC in the payload, you have to send two IACs, the first quoting the second. Otherwise the sequence will be interpreted as a command by the other side.

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

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()).

@ukleinek ukleinek force-pushed the quote-IAC-on-send branch 2 times, most recently from f979125 to d721d2d Compare June 5, 2023 09:06
@ukleinek
Copy link
Contributor Author

ukleinek commented Jun 5, 2023

I accidently pushed some debug code, to see the intended changes, look at https://github.com/pengutronix/microcom/compare/3080a9d..d721d2d9bd49e7fcd399e7fd3b3bb548e077ce3d

@a3f
Copy link
Member

a3f commented Jun 5, 2023

Why not just write(ios->fd, "\xff\xff", 2); /* 2x IAC */? And in future, this could be changes to write_full or similar that retries on EINTR.

@ukleinek
Copy link
Contributor Author

ukleinek commented Jun 5, 2023

Why not just write(ios->fd, "\xff\xff", 2); /* 2x IAC */? And in future, this could be changes to write_full or similar that retries on EINTR.

I wanted to benefit from the definition of IAC and not open-code it as \xff.

@a3f
Copy link
Member

a3f commented Jun 5, 2023

I wanted to benefit from the definition of IAC and not open-code it as \xff.

Not completely serious, but there's also (uint8_t []) { IAC, IAC }..

To send a single IAC in the payload, you have to send two IACs, the
first quoting the second. Otherwise the sequence will be interpreted as
a command by the other side.

Signed-off-by: Uwe Kleine-König <[email protected]>
@ukleinek ukleinek merged commit 5a3bc9d into pengutronix:master Jun 8, 2023
@ukleinek ukleinek deleted the quote-IAC-on-send branch June 8, 2023 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants