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 2021/05/21 05:33:43 UTC
[GitHub] [incubator-nuttx] anchao opened a new pull request #3755: net/icmp: icmp bug fix and enhancement
anchao opened a new pull request #3755:
URL: https://github.com/apache/incubator-nuttx/pull/3755
## Summary
net/icmp: fix race condition in icmp recvmsg
net/icmp: add nonblocking support
net/icmp: consume the data length to avoid duplicate packet
net/icmp: fix invalid condition comparison
## Impact
ping/ping6
## Testing
ping/ping6 works as expected
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] yamt commented on a change in pull request #3755: net/icmp: icmp bug fix and enhancement
Posted by GitBox <gi...@apache.org>.
yamt commented on a change in pull request #3755:
URL: https://github.com/apache/incubator-nuttx/pull/3755#discussion_r637719224
##########
File path: net/icmp/icmp_recvmsg.c
##########
@@ -162,7 +162,7 @@ static uint16_t recvfrom_eventhandler(FAR struct net_driver_s *dev,
/* Return the size of the returned data */
- DEBUGASSERT(recvsize > INT16_MAX);
+ DEBUGASSERT(recvsize < INT16_MAX);
Review comment:
maybe "<=" ?
##########
File path: net/icmpv6/icmpv6_recvmsg.c
##########
@@ -169,7 +169,7 @@ static uint16_t recvfrom_eventhandler(FAR struct net_driver_s *dev,
/* Return the size of the returned data */
- DEBUGASSERT(recvsize > INT16_MAX);
+ DEBUGASSERT(recvsize < INT16_MAX);
Review comment:
<= ?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #3755: net/icmp: icmp bug fix and enhancement
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #3755:
URL: https://github.com/apache/incubator-nuttx/pull/3755#discussion_r638528398
##########
File path: net/icmp/icmp_recvmsg.c
##########
@@ -162,7 +162,7 @@ static uint16_t recvfrom_eventhandler(FAR struct net_driver_s *dev,
/* Return the size of the returned data */
- DEBUGASSERT(recvsize > INT16_MAX);
+ DEBUGASSERT(recvsize < INT16_MAX);
Review comment:
Done
##########
File path: net/icmpv6/icmpv6_recvmsg.c
##########
@@ -169,7 +169,7 @@ static uint16_t recvfrom_eventhandler(FAR struct net_driver_s *dev,
/* Return the size of the returned data */
- DEBUGASSERT(recvsize > INT16_MAX);
+ DEBUGASSERT(recvsize < INT16_MAX);
Review comment:
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3755: net/icmp: icmp bug fix and enhancement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3755:
URL: https://github.com/apache/incubator-nuttx/pull/3755#discussion_r642248466
##########
File path: net/icmp/icmp_recvmsg.c
##########
@@ -389,111 +389,114 @@ ssize_t icmp_recvmsg(FAR struct socket *psock, FAR struct msghdr *msg,
goto errout;
Review comment:
net_unlock shouldn't be called here.
##########
File path: net/icmp/icmp_recvmsg.c
##########
@@ -389,111 +389,114 @@ ssize_t icmp_recvmsg(FAR struct socket *psock, FAR struct msghdr *msg,
goto errout;
}
+ /* Get the device that was used to send the ICMP request. */
+
+ dev = conn->dev;
+ DEBUGASSERT(dev != NULL);
+ if (dev == NULL)
+ {
+ ret = -EPROTO;
+ goto errout;
Review comment:
net_unlock shouldn't be called here.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #3755: net/icmp: icmp bug fix and enhancement
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #3755:
URL: https://github.com/apache/incubator-nuttx/pull/3755#discussion_r646338066
##########
File path: net/icmp/icmp_recvmsg.c
##########
@@ -181,7 +181,8 @@ static uint16_t recvfrom_eventhandler(FAR struct net_driver_s *dev,
/* Indicate that the data has been consumed */
- flags &= ~ICMP_NEWDATA;
+ flags &= ~ICMP_NEWDATA;
+ dev->d_len = 0;
Review comment:
Hi, @antmerlino
This change follows the logic of other protocol stacks, d_Len should be set to 0 if the buffer is consumed
https://github.com/apache/incubator-nuttx/blob/3859031a6bfdc987b6704f5460762e91e8d84b8e/net/tcp/tcp_recvfrom.c#L215
https://github.com/apache/incubator-nuttx/blob/3859031a6bfdc987b6704f5460762e91e8d84b8e/net/udp/udp_recvfrom.c#L179
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull request #3755: net/icmp: icmp bug fix and enhancement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #3755:
URL: https://github.com/apache/incubator-nuttx/pull/3755#discussion_r642274906
##########
File path: net/icmp/icmp_recvmsg.c
##########
@@ -384,108 +385,116 @@ ssize_t icmp_recvmsg(FAR struct socket *psock, FAR struct msghdr *msg,
conn = psock->s_conn;
if (conn->nreqs < 1)
{
- ret = -EPROTO;
- goto errout;
+ return -EPROTO;
}
+ /* Get the device that was used to send the ICMP request. */
+
+ dev = conn->dev;
Review comment:
should we keep it in the original place?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #3755: net/icmp: icmp bug fix and enhancement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged pull request #3755:
URL: https://github.com/apache/incubator-nuttx/pull/3755
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] antmerlino commented on a change in pull request #3755: net/icmp: icmp bug fix and enhancement
Posted by GitBox <gi...@apache.org>.
antmerlino commented on a change in pull request #3755:
URL: https://github.com/apache/incubator-nuttx/pull/3755#discussion_r646891542
##########
File path: net/icmp/icmp_recvmsg.c
##########
@@ -181,7 +181,8 @@ static uint16_t recvfrom_eventhandler(FAR struct net_driver_s *dev,
/* Indicate that the data has been consumed */
- flags &= ~ICMP_NEWDATA;
+ flags &= ~ICMP_NEWDATA;
+ dev->d_len = 0;
Review comment:
@anchao Thanks for clarifying. I didn't see them because they were in a function called from recv_handler.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] anchao commented on a change in pull request #3755: net/icmp: icmp bug fix and enhancement
Posted by GitBox <gi...@apache.org>.
anchao commented on a change in pull request #3755:
URL: https://github.com/apache/incubator-nuttx/pull/3755#discussion_r642254749
##########
File path: net/icmp/icmp_recvmsg.c
##########
@@ -389,111 +389,114 @@ ssize_t icmp_recvmsg(FAR struct socket *psock, FAR struct msghdr *msg,
goto errout;
Review comment:
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] antmerlino commented on pull request #3755: net/icmp: icmp bug fix and enhancement
Posted by GitBox <gi...@apache.org>.
antmerlino commented on pull request #3755:
URL: https://github.com/apache/incubator-nuttx/pull/3755#issuecomment-851612186
@xiaoxiang781216 give me a day or so to look at this again
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3755: net/icmp: icmp bug fix and enhancement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3755:
URL: https://github.com/apache/incubator-nuttx/pull/3755#issuecomment-851295294
@yamt @antmerlino could you take a look again?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #3755: net/icmp: icmp bug fix and enhancement
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #3755:
URL: https://github.com/apache/incubator-nuttx/pull/3755#issuecomment-851760820
> @xiaoxiang781216 give me a day or so to look at this again
Sure.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] antmerlino commented on a change in pull request #3755: net/icmp: icmp bug fix and enhancement
Posted by GitBox <gi...@apache.org>.
antmerlino commented on a change in pull request #3755:
URL: https://github.com/apache/incubator-nuttx/pull/3755#discussion_r638831198
##########
File path: net/icmp/icmp_recvmsg.c
##########
@@ -181,7 +181,8 @@ static uint16_t recvfrom_eventhandler(FAR struct net_driver_s *dev,
/* Indicate that the data has been consumed */
- flags &= ~ICMP_NEWDATA;
+ flags &= ~ICMP_NEWDATA;
+ dev->d_len = 0;
Review comment:
@anchao Can you explain how not setting d_len=0 caused packet duplication? Is this only required for ICMP or should the change be propageted to other network protocols?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org