You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/01/29 18:20:14 UTC
[GitHub] [incubator-nuttx] a-lunev opened a new pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
a-lunev opened a new pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373
## Summary
Added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in TCP callback handlers.
The discussion of the noticed bug started here: https://github.com/apache/incubator-nuttx/pull/5252#pullrequestreview-861912204
As it turned out, the issue was mentioned initially in 2015 here: https://github.com/apache/incubator-nuttx/blob/63071a563a1f7646dab9a6bd718d8b4b550471c4/net/socket/net_monitor.c#L166
## Impact
TCP
## Testing
CONFIG_DEBUG_ASSERTIONS and CONFIG_DEBUG_NET_ERROR need to be enabled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373#discussion_r795321572
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1145,6 +1168,20 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
if (psock->s_sndcb == NULL)
{
psock->s_sndcb = tcp_callback_alloc(conn);
+
+#ifdef CONFIG_DEBUG_ASSERTIONS
Review comment:
move the assertion to :
https://github.com/apache/incubator-nuttx/blob/master/net/devif/devif_callback.c#L354-L359
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -376,6 +376,29 @@ static uint16_t psock_send_eventhandler(FAR struct net_driver_s *dev,
conn = psock->s_conn;
DEBUGASSERT(conn != NULL);
+#ifdef CONFIG_DEBUG_NET_ERROR
+ if (conn->dev == NULL || (pvconn != conn && pvconn != NULL))
+ {
+ nerr("ERROR: conn->dev == NULL or pvconn != conn:"
+ " dev=%p pvconn=%p pvpriv=%p flags=0x%04x"
Review comment:
create the new file named tcp_dump.c and unify the tcp connect dump ?
##########
File path: include/nuttx/net/net.h
##########
@@ -270,6 +270,10 @@ struct socket
/* Callback instance for TCP send() or UDP sendto() */
FAR struct devif_callback_s *s_sndcb;
+
+#ifdef CONFIG_DEBUG_ASSERTIONS
+ int s_sndcb_alloc_cnt; /* The callback allocation counter */
Review comment:
add the assertion to
https://github.com/apache/incubator-nuttx/blob/master/net/devif/devif_callback.c#L354-L359
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373#discussion_r797631164
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -376,6 +376,29 @@ static uint16_t psock_send_eventhandler(FAR struct net_driver_s *dev,
conn = psock->s_conn;
DEBUGASSERT(conn != NULL);
+#ifdef CONFIG_DEBUG_NET_ERROR
+ if (conn->dev == NULL || (pvconn != conn && pvconn != NULL))
+ {
+ nerr("ERROR: conn->dev == NULL or pvconn != conn:"
+ " dev=%p pvconn=%p pvpriv=%p flags=0x%04x"
Review comment:
> The dump of connect could be implement to a common feature, the developers can reuse this api to debug other stack issues
Good. I have created tcp_event_handler_dump() function.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis merged pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
Posted by GitBox <gi...@apache.org>.
acassis merged pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373#discussion_r797575817
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1145,6 +1168,20 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
if (psock->s_sndcb == NULL)
{
psock->s_sndcb = tcp_callback_alloc(conn);
+
+#ifdef CONFIG_DEBUG_ASSERTIONS
Review comment:
It is about another potential issue. I am suspecting the following:
In case of closing TCP connection the TCP callback is marked as null and the callback flags are zeroed (it is normal), however in some places of the code the socket is not yet marked as closed by the time, therefore the user app can call write() further. In this case one more callback may be created in psock_tcp_send(). Then the socket is finally marked as closed, however the second callback is alive and may trigger.
Thus my plan is first to confirm the root cause (I am not yet sure the potential issue really exists), then to fix the issue (if this root cause is confirmed).
Even if the root cause is not confirmed for now, in any case this debug assert is reasonable to be in the code forever to detect similar issue in the future because of further code modifications.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373#discussion_r797494484
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -376,6 +376,29 @@ static uint16_t psock_send_eventhandler(FAR struct net_driver_s *dev,
conn = psock->s_conn;
DEBUGASSERT(conn != NULL);
+#ifdef CONFIG_DEBUG_NET_ERROR
+ if (conn->dev == NULL || (pvconn != conn && pvconn != NULL))
+ {
+ nerr("ERROR: conn->dev == NULL or pvconn != conn:"
+ " dev=%p pvconn=%p pvpriv=%p flags=0x%04x"
Review comment:
The dump of connect could be implement to a common feature, the developers can reuse this api to debug other stack issues
if this PR is a common feature, welcome.
If this PR is just for testing or debugging, please mark the tag as draft and send the patch link to reporter locally, I hope the issue can be resolved before mainline.
about #5252, I will upload relevant patch asap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373#issuecomment-1052156695
@a-lunev sorry my delay, I missed your fix!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373#discussion_r795741229
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -376,6 +376,29 @@ static uint16_t psock_send_eventhandler(FAR struct net_driver_s *dev,
conn = psock->s_conn;
DEBUGASSERT(conn != NULL);
+#ifdef CONFIG_DEBUG_NET_ERROR
+ if (conn->dev == NULL || (pvconn != conn && pvconn != NULL))
+ {
+ nerr("ERROR: conn->dev == NULL or pvconn != conn:"
+ " dev=%p pvconn=%p pvpriv=%p flags=0x%04x"
Review comment:
This "if" condition is temporary and is going to be removed as soon as the issue investigation is finished. So I do not think it is reasonable to unify / organize some new file for it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373#discussion_r797575817
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1145,6 +1168,20 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
if (psock->s_sndcb == NULL)
{
psock->s_sndcb = tcp_callback_alloc(conn);
+
+#ifdef CONFIG_DEBUG_ASSERTIONS
Review comment:
It is about another potential issue. I am suspecting the following:
In case of closing TCP connection the TCP callback is marked as null and the callback flags are zeroed (it is normal), however in some places of the code the socket is not yet marked as closed by the time, therefore the user app can call write() further. In this case one more callback may be created in psock_tcp_send(). Then the socket is finally marked as closed, however the second callback is alive and may trigger.
Thus my plan is first to confirm the root cause (I am not yet sure the potential issue really exists), then to fix the issue (if this root cause is confirmed).
Even if the root cause is not confirmed for now, in any case this debug assert is reasonable to be in the code forever to prevent similar issue in the future because of further code modifications.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373#issuecomment-1035372685
> LGTM ( @a-lunev please fix the conflicting files)
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis edited a comment on pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
Posted by GitBox <gi...@apache.org>.
acassis edited a comment on pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373#issuecomment-1035255930
LGTM ( @a-lunev please fix the conflicting files)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on a change in pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
Posted by GitBox <gi...@apache.org>.
a-lunev commented on a change in pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373#discussion_r795738040
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1145,6 +1168,20 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
if (psock->s_sndcb == NULL)
{
psock->s_sndcb = tcp_callback_alloc(conn);
+
+#ifdef CONFIG_DEBUG_ASSERTIONS
Review comment:
This assertion is not to debug devif_callback.c. Instead the assertion is to debug only tcp_send_buffered.c. It is to make sure that not more than one callback is allocated on each psock_tcp_send() invocation within given socket instance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] acassis commented on pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373#issuecomment-1035255930
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373#discussion_r797485704
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -1145,6 +1168,20 @@ ssize_t psock_tcp_send(FAR struct socket *psock, FAR const void *buf,
if (psock->s_sndcb == NULL)
{
psock->s_sndcb = tcp_callback_alloc(conn);
+
+#ifdef CONFIG_DEBUG_ASSERTIONS
Review comment:
vfs will guarantee that invalid sockets will not pass here, the check is useless
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #5373: tcp: added debug asserts and logging to investigate the rare (conn->dev == NULL) bug in callback handlers
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #5373:
URL: https://github.com/apache/incubator-nuttx/pull/5373#discussion_r797494484
##########
File path: net/tcp/tcp_send_buffered.c
##########
@@ -376,6 +376,29 @@ static uint16_t psock_send_eventhandler(FAR struct net_driver_s *dev,
conn = psock->s_conn;
DEBUGASSERT(conn != NULL);
+#ifdef CONFIG_DEBUG_NET_ERROR
+ if (conn->dev == NULL || (pvconn != conn && pvconn != NULL))
+ {
+ nerr("ERROR: conn->dev == NULL or pvconn != conn:"
+ " dev=%p pvconn=%p pvpriv=%p flags=0x%04x"
Review comment:
The dump of connect could be implement to a common feature, the developers can reuse this api to debug other stack issues
if this PR is a common feature, welcome.
If this PR is just for testing of debugging, please mark the tag as draft and send the patch link to reporter locally, I hope the issue can be resolved before mainline.
about #5252, I will upload relevant patch asap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org