[PATCH 2/3] Fix issue where cli socket buffer may get full
The cli module must reset the socket io buffer rpos when all characters in the socket buffer has been processed. The method cli_get_command is always invoked twice for every CLI command, thus rxpos may also be reset at the second invocation if no newline was found and no new data is input to the socket buffer. Signed-off-by: Kenth Eriksson <kenth.eriksson@infinera.com> --- sysdep/unix/main.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sysdep/unix/main.c b/sysdep/unix/main.c index 921115b1..cf5f4a3c 100644 --- a/sysdep/unix/main.c +++ b/sysdep/unix/main.c @@ -403,7 +403,10 @@ cli_get_command(cli *c) { t++; c->rx_pos = c->rx_buf; - c->rx_aux = t; + if (t < tend) + c->rx_aux = t; + else + c->rx_aux = s->rpos = s->rbuf; *d = 0; return (d < dend) ? 1 : -1; } -- 2.21.0
On Tue, 2019-06-11 at 12:09 +0200, Kenth Eriksson wrote:
The cli module must reset the socket io buffer rpos when all characters in the socket buffer has been processed.
The method cli_get_command is always invoked twice for every CLI command, thus rxpos may also be reset at the second invocation if no newline was found and no new data is input to the socket buffer.
Signed-off-by: Kenth Eriksson <kenth.eriksson@infinera.com> --- sysdep/unix/main.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/sysdep/unix/main.c b/sysdep/unix/main.c index 921115b1..cf5f4a3c 100644 --- a/sysdep/unix/main.c +++ b/sysdep/unix/main.c @@ -403,7 +403,10 @@ cli_get_command(cli *c) { t++; c->rx_pos = c->rx_buf; - c->rx_aux = t; + if (t < tend) + c->rx_aux = t; + else + c->rx_aux = s->rpos = s->rbuf;
Nu går det att läsa :) Indent ser lite knas ut el. så är det Evolution som buggar
*d = 0; return (d < dend) ? 1 : -1; } -- 2.21.0
On Tue, Jun 11, 2019 at 12:09:02PM +0200, Kenth Eriksson wrote:
The cli module must reset the socket io buffer rpos when all characters in the socket buffer has been processed.
The method cli_get_command is always invoked twice for every CLI command, thus rxpos may also be reset at the second invocation if no newline was found and no new data is input to the socket buffer.
Hi Now i see where the bug is hidden: There are two RX buffers in CLI, one part of socket (s->rbuf) and one directly in CLI (c->rx_buf). In cli_get_command(), the command is copied from the first to the second (where it starts from the beginning of the buffer), but position in the second buffer is reset only during unsuccessful cli_get_command() of the next command. Therefore, if the command ends on the same position as the end of the first buffer, then sk_read calls read with 0 length. This your patch resets position of the first buffer when all data in it were processed. That is correct and helps with the bug (as it is less likely to hit the end of the first buffer). But the socket is still a bit problematic - it works only in request-reply mode (which is OK w.r.t. birdc client). If a client sends more bytes (e.g. next commands) before all the reply is sent back, it is possible to hang the CLI without processing all commands, or trigger the condition when read is called with 0 length. E.g. sender sends enough bytes to fill the socket buffer, CLI event is called, cli_get_command() reads first command, but there are more data in the socket buffer, so it does not reset the position, therefore the next rx_hook for CLI is called with 0 length. But as we do not support pipelining commands properly anyways, we could just fix the issue by this patch.
Signed-off-by: Kenth Eriksson <kenth.eriksson@infinera.com> --- sysdep/unix/main.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/sysdep/unix/main.c b/sysdep/unix/main.c index 921115b1..cf5f4a3c 100644 --- a/sysdep/unix/main.c +++ b/sysdep/unix/main.c @@ -403,7 +403,10 @@ cli_get_command(cli *c) { t++; c->rx_pos = c->rx_buf; - c->rx_aux = t; + if (t < tend) + c->rx_aux = t; + else + c->rx_aux = s->rpos = s->rbuf; *d = 0; return (d < dend) ? 1 : -1; } -- 2.21.0
-- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
On Tue, 2019-06-11 at 17:03 +0200, Ondrej Zajicek wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Tue, Jun 11, 2019 at 12:09:02PM +0200, Kenth Eriksson wrote:
The cli module must reset the socket io buffer rpos when all characters in the socket buffer has been processed.
The method cli_get_command is always invoked twice for every CLI command, thus rxpos may also be reset at the second invocation if no newline was found and no new data is input to the socket buffer.
Hi
Now i see where the bug is hidden:
There are two RX buffers in CLI, one part of socket (s->rbuf) and one directly in CLI (c->rx_buf). In cli_get_command(), the command is copied from the first to the second (where it starts from the beginning of the buffer), but position in the second buffer is reset only during unsuccessful cli_get_command() of the next command. Therefore, if the command ends on the same position as the end of the first buffer, then sk_read calls read with 0 length.
This your patch resets position of the first buffer when all data in it were processed. That is correct and helps with the bug (as it is less likely to hit the end of the first buffer). But the socket is still a bit problematic - it works only in request-reply mode (which is OK w.r.t. birdc client). If a client sends more bytes (e.g. next commands) before all the reply is sent back, it is possible to hang the CLI without processing all commands, or trigger the condition when read is called with 0 length.
E.g. sender sends enough bytes to fill the socket buffer, CLI event is called, cli_get_command() reads first command, but there are more data in the socket buffer, so it does not reset the position, therefore the next rx_hook for CLI is called with 0 length.
Correct, you still need patch 1/3 to not close socket if you read zero bytes due to socket buffer is full. Also patch 1/3 is not only relevant for CLI. E.g. what happens if you get a big burst of protocol packets (OSPF, BGP etc.)? That would cause the socket to get closed due to buffer is full?
But as we do not support pipelining commands properly anyways, we could just fix the issue by this patch.
Signed-off-by: Kenth Eriksson <kenth.eriksson@infinera.com> --- sysdep/unix/main.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/sysdep/unix/main.c b/sysdep/unix/main.c index 921115b1..cf5f4a3c 100644 --- a/sysdep/unix/main.c +++ b/sysdep/unix/main.c @@ -403,7 +403,10 @@ cli_get_command(cli *c) { t++; c->rx_pos = c->rx_buf; - c->rx_aux = t; + if (t < tend) + c->rx_aux = t; + else + c->rx_aux = s->rpos = s->rbuf; *d = 0; return (d < dend) ? 1 : -1; } -- 2.21.0
-- Elen sila lumenn' omentielvo
Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
On Tue, Jun 11, 2019 at 03:54:21PM +0000, Kenth Eriksson wrote:
On Tue, 2019-06-11 at 17:03 +0200, Ondrej Zajicek wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Tue, Jun 11, 2019 at 12:09:02PM +0200, Kenth Eriksson wrote:
The cli module must reset the socket io buffer rpos when all characters in the socket buffer has been processed.
The method cli_get_command is always invoked twice for every CLI command, thus rxpos may also be reset at the second invocation if no newline was found and no new data is input to the socket buffer.
Hi
Now i see where the bug is hidden:
There are two RX buffers in CLI, one part of socket (s->rbuf) and one directly in CLI (c->rx_buf). In cli_get_command(), the command is copied from the first to the second (where it starts from the beginning of the buffer), but position in the second buffer is reset only during unsuccessful cli_get_command() of the next command. Therefore, if the command ends on the same position as the end of the first buffer, then sk_read calls read with 0 length.
This your patch resets position of the first buffer when all data in it were processed. That is correct and helps with the bug (as it is less likely to hit the end of the first buffer). But the socket is still a bit problematic - it works only in request-reply mode (which is OK w.r.t. birdc client). If a client sends more bytes (e.g. next commands) before all the reply is sent back, it is possible to hang the CLI without processing all commands, or trigger the condition when read is called with 0 length.
E.g. sender sends enough bytes to fill the socket buffer, CLI event is called, cli_get_command() reads first command, but there are more data in the socket buffer, so it does not reset the position, therefore the next rx_hook for CLI is called with 0 length.
Correct, you still need patch 1/3 to not close socket if you read zero bytes due to socket buffer is full.
Also patch 1/3 is not only relevant for CLI. E.g. what happens if you get a big burst of protocol packets (OSPF, BGP etc.)? That would cause the socket to get closed due to buffer is full?
No, because all other protocols process data from rx-buffer immediately in rx_hook. It is trivial for datagram-based protocols like OSPF or RIP, where one rx_hook call is one datagram. It is a bit more complicated for TCP-based BGP, where BGP message framing is not congruent with read(), but in that case we process the rx-buffer in rx_hook until we have one full message (and we have big enough buffer for one full message) and the we immediately process it from rx_hook. Therefore we have invariant that after rx_hook, there is non-full buffer. This invariant is not satisfied by CLI rx_hook. This is crudely fixed by 3/3, but i think that if we want to throw away some data, it should be done on message boundary. Also, better way to handle 1/3 is to add condition on line io.c line 2211, so that if we have full buffer, we do not try to check POLLIN, so that we do not even call sk_read() in such case instead of try it to handle from inside sk_read(). -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
On Tue, 2019-06-11 at 18:14 +0200, Ondrej Zajicek wrote:
On Tue, Jun 11, 2019 at 03:54:21PM +0000, Kenth Eriksson wrote:
On Tue, 2019-06-11 at 17:03 +0200, Ondrej Zajicek wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Tue, Jun 11, 2019 at 12:09:02PM +0200, Kenth Eriksson wrote:
The cli module must reset the socket io buffer rpos when all characters in the socket buffer has been processed.
The method cli_get_command is always invoked twice for every CLI command, thus rxpos may also be reset at the second invocation if no newline was found and no new data is input to the socket buffer.
Hi
Now i see where the bug is hidden:
There are two RX buffers in CLI, one part of socket (s->rbuf) and one directly in CLI (c->rx_buf). In cli_get_command(), the command is copied from the first to the second (where it starts from the beginning of the buffer), but position in the second buffer is reset only during unsuccessful cli_get_command() of the next command. Therefore, if the command ends on the same position as the end of the first buffer, then sk_read calls read with 0 length.
This your patch resets position of the first buffer when all data in it were processed. That is correct and helps with the bug (as it is less likely to hit the end of the first buffer). But the socket is still a bit problematic - it works only in request-reply mode (which is OK w.r.t. birdc client). If a client sends more bytes (e.g. next commands) before all the reply is sent back, it is possible to hang the CLI without processing all commands, or trigger the condition when read is called with 0 length.
E.g. sender sends enough bytes to fill the socket buffer, CLI event is called, cli_get_command() reads first command, but there are more data in the socket buffer, so it does not reset the position, therefore the next rx_hook for CLI is called with 0 length.
Correct, you still need patch 1/3 to not close socket if you read zero bytes due to socket buffer is full.
Also patch 1/3 is not only relevant for CLI. E.g. what happens if you get a big burst of protocol packets (OSPF, BGP etc.)? That would cause the socket to get closed due to buffer is full?
No, because all other protocols process data from rx-buffer immediately in rx_hook. It is trivial for datagram-based protocols like OSPF or RIP, where one rx_hook call is one datagram. It is a bit more complicated for TCP-based BGP, where BGP message framing is not congruent with read(), but in that case we process the rx-buffer in rx_hook until we have one full message (and we have big enough buffer for one full message) and the we immediately process it from rx_hook.
I see now, the protocols are synchronous to the io read event.
Therefore we have invariant that after rx_hook, there is non-full buffer. This invariant is not satisfied by CLI rx_hook. This is crudely fixed by 3/3, but i think that if we want to throw away some data, it should be done on message boundary.
Also, better way to handle 1/3 is to add condition on line io.c line 2211, so that if we have full buffer, we do not try to check POLLIN, so that we do not even call sk_read() in such case instead of try it to handle from inside sk_read().
Ok, I can update patch 1/3 so that is not included in the pollset if the io buffer is full. Will repost, so that you can review. But I still don't see reading 0 shall close the socket as reading 0 is not an error. Shall we combine the two patches to get a more robust solution? I.e. don't include in pollset if buffer full and don't close if reading 0?
On Wed, Jun 12, 2019 at 07:29:51AM +0000, Kenth Eriksson wrote:
Also, better way to handle 1/3 is to add condition on line io.c line 2211, so that if we have full buffer, we do not try to check POLLIN, so that we do not even call sk_read() in such case instead of try it to handle from inside sk_read().
Ok, I can update patch 1/3 so that is not included in the pollset if the io buffer is full. Will repost, so that you can review. But I still don't see reading 0 shall close the socket as reading 0 is not an error. Shall we combine the two patches to get a more robust solution?
My position is that sk_read() should depend on result of internal read() and not on external information like POLLIN status. But how to distinguish reading 0 as a result of socket close from other cases? Also, the issue of empty buffer could be simply handled by return 0 from sk_read() just before the read(). If there is no space in buffer, then there is no point to call read(). I will look at the patches and merge something reasonable. -- Elen sila lumenn' omentielvo Ondrej 'Santiago' Zajicek (email: santiago@crfreenet.org) OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net) "To err is human -- to blame it on a computer is even more so."
Don't set the POLLIN flag on a socket if the socket buffer is full. I.e. a socket with full read buffer is not included in the io loop for reading. Signed-off-by: Kenth Eriksson <kenth.eriksson@infinera.com> --- sysdep/unix/io.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/sysdep/unix/io.c b/sysdep/unix/io.c index d1d86e3b..2b163417 100644 --- a/sysdep/unix/io.c +++ b/sysdep/unix/io.c @@ -1845,6 +1845,12 @@ sk_read_ssh(sock *s) } #endif +static inline size_t +sk_rbcount(sock *s) +{ + return s->rbuf + s->rbsize - s->rpos; +} + /* sk_read() and sk_write() are called from BFD's event loop */ int @@ -1861,7 +1867,7 @@ sk_read(sock *s, int revents) case SK_TCP: case SK_UNIX: { - int c = read(s->fd, s->rpos, s->rbuf + s->rbsize - s->rpos); + int c = read(s->fd, s->rpos, sk_rbcount(s)); if (c < 0) { @@ -2207,7 +2213,7 @@ io_loop(void) { pfd[nfds] = (struct pollfd) { .fd = -1 }; /* everything other set to 0 by this */ s = SKIP_BACK(sock, n, n); - if (s->rx_hook) + if (s->rx_hook && sk_rbcount(s)) { pfd[nfds].fd = s->fd; pfd[nfds].events |= POLLIN; -- 2.21.0
On Tue, 2019-06-11 at 18:14 +0200, Ondrej Zajicek wrote:
On Tue, Jun 11, 2019 at 03:54:21PM +0000, Kenth Eriksson wrote:
On Tue, 2019-06-11 at 17:03 +0200, Ondrej Zajicek wrote:
CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
On Tue, Jun 11, 2019 at 12:09:02PM +0200, Kenth Eriksson wrote:
The cli module must reset the socket io buffer rpos when all characters in the socket buffer has been processed.
The method cli_get_command is always invoked twice for every CLI command, thus rxpos may also be reset at the second invocation if no newline was found and no new data is input to the socket buffer.
Hi
Now i see where the bug is hidden:
There are two RX buffers in CLI, one part of socket (s->rbuf) and one directly in CLI (c->rx_buf). In cli_get_command(), the command is copied from the first to the second (where it starts from the beginning of the buffer), but position in the second buffer is reset only during unsuccessful cli_get_command() of the next command. Therefore, if the command ends on the same position as the end of the first buffer, then sk_read calls read with 0 length.
This your patch resets position of the first buffer when all data in it were processed. That is correct and helps with the bug (as it is less likely to hit the end of the first buffer). But the socket is still a bit problematic - it works only in request-reply mode (which is OK w.r.t. birdc client). If a client sends more bytes (e.g. next commands) before all the reply is sent back, it is possible to hang the CLI without processing all commands, or trigger the condition when read is called with 0 length.
E.g. sender sends enough bytes to fill the socket buffer, CLI event is called, cli_get_command() reads first command, but there are more data in the socket buffer, so it does not reset the position, therefore the next rx_hook for CLI is called with 0 length.
Correct, you still need patch 1/3 to not close socket if you read zero bytes due to socket buffer is full.
Also patch 1/3 is not only relevant for CLI. E.g. what happens if you get a big burst of protocol packets (OSPF, BGP etc.)? That would cause the socket to get closed due to buffer is full?
No, because all other protocols process data from rx-buffer immediately in rx_hook. It is trivial for datagram-based protocols like OSPF or RIP, where one rx_hook call is one datagram. It is a bit more complicated for TCP-based BGP, where BGP message framing is not congruent with read(), but in that case we process the rx-buffer in rx_hook until we have one full message (and we have big enough buffer for one full message) and the we immediately process it from rx_hook.
Therefore we have invariant that after rx_hook, there is non-full buffer. This invariant is not satisfied by CLI rx_hook. This is crudely fixed by 3/3, but i think that if we want to throw away some data, it should be done on message boundary.
Also, better way to handle 1/3 is to add condition on line io.c line 2211, so that if we have full buffer, we do not try to check POLLIN, so that we do not even call sk_read() in such case instead of try it to handle from inside sk_read().
I believe we *might* have seen a side effect of trying to do what you suggest here. The sk_read also makes accept on the socket if it is not in a connected state, e.g. SK_UNIX_PASSIVE and SK_TCP_PASSIVE. Might be that I implemented your suggestion incorrectly. So could you please provide a patch for your suggestion? Or maybe comment on the one I sent on the list?
participants (4)
-
Joakim Tjernlund -
Kenth Eriksson -
Kenth Eriksson
-
Ondrej Zajicek