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/07/25 04:15:49 UTC
[GitHub] [incubator-nuttx] zhhyu7 opened a new pull request, #6702: tcp: check option length before d_len update
zhhyu7 opened a new pull request, #6702:
URL: https://github.com/apache/incubator-nuttx/pull/6702
Signed-off-by: zhanghongyu <zh...@xiaomi.com>
Change-Id: I67eb94228c32f70eb5622426e680ed562eb9d42d
## Summary
## Impact
## Testing
--
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] pkarashchenko merged pull request #6702: tcp: check option length before d_len update
Posted by GitBox <gi...@apache.org>.
pkarashchenko merged PR #6702:
URL: https://github.com/apache/incubator-nuttx/pull/6702
--
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] pkarashchenko commented on a diff in pull request #6702: tcp: check option length before d_len update
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6702:
URL: https://github.com/apache/incubator-nuttx/pull/6702#discussion_r929712960
##########
net/tcp/tcp_input.c:
##########
@@ -616,6 +616,17 @@ static void tcp_input(FAR struct net_driver_s *dev, uint8_t domain,
goto drop;
}
+ /* d_appdata should remove the tcp specific option field. */
+
+ if ((tcp->tcpoffset & 0xf0) > 0x50)
+ {
+ len = ((tcp->tcpoffset >> 4) - 5) << 2;
Review Comment:
Now changes looks better. I was not saying that your initial code was wrong, but at least hard to track what is going on.
--
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] zhhyu7 commented on a diff in pull request #6702: tcp: check option length before d_len update
Posted by GitBox <gi...@apache.org>.
zhhyu7 commented on code in PR #6702:
URL: https://github.com/apache/incubator-nuttx/pull/6702#discussion_r928861268
##########
net/tcp/tcp_input.c:
##########
@@ -616,6 +616,17 @@ static void tcp_input(FAR struct net_driver_s *dev, uint8_t domain,
goto drop;
}
+ /* d_appdata should remove the tcp specific option field. */
+
+ if ((tcp->tcpoffset & 0xf0) > 0x50)
+ {
+ len = ((tcp->tcpoffset >> 4) - 5) << 2;
Review Comment:
> I do not understand this. The `len` value is unconditionally overwritten few lines below. Maybe `len = (tcp->tcpoffset >> 4) << 2;` at line 634 should be relocated to the `else` of `if ((tcp->tcpoffset & 0xf0) > 0x50)`?
@pkarashchenko
They're calculated separately, so maybe they're a little redundant. Offset do not contain options length when tcp_ipvx_select, so need to skip options field when TCP_NEWDATA flag set. Should I skip options field in tcp_ipvx_select function with IPvxTCP_HDRLEN? It doesn't feel right either.
--
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] pkarashchenko commented on a diff in pull request #6702: tcp: check option length before d_len update
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6702:
URL: https://github.com/apache/incubator-nuttx/pull/6702#discussion_r928678085
##########
net/tcp/tcp_input.c:
##########
@@ -616,6 +616,17 @@ static void tcp_input(FAR struct net_driver_s *dev, uint8_t domain,
goto drop;
}
+ /* d_appdata should remove the tcp specific option field. */
+
+ if ((tcp->tcpoffset & 0xf0) > 0x50)
+ {
+ len = ((tcp->tcpoffset >> 4) - 5) << 2;
Review Comment:
I do not understand this. The `len` value is unconditionally overwritten few lines below.
Maybe `len = (tcp->tcpoffset >> 4) << 2;` at line 634 should be relocated to the `else` of `if ((tcp->tcpoffset & 0xf0) > 0x50)`?
--
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] xiaoxiang781216 commented on pull request #6702: tcp: check option length before d_len update
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6702:
URL: https://github.com/apache/incubator-nuttx/pull/6702#issuecomment-1193606342
@zhhyu7 remove the change-id from commit message.
--
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] zhhyu7 commented on a diff in pull request #6702: tcp: check option length before d_len update
Posted by GitBox <gi...@apache.org>.
zhhyu7 commented on code in PR #6702:
URL: https://github.com/apache/incubator-nuttx/pull/6702#discussion_r928861268
##########
net/tcp/tcp_input.c:
##########
@@ -616,6 +616,17 @@ static void tcp_input(FAR struct net_driver_s *dev, uint8_t domain,
goto drop;
}
+ /* d_appdata should remove the tcp specific option field. */
+
+ if ((tcp->tcpoffset & 0xf0) > 0x50)
+ {
+ len = ((tcp->tcpoffset >> 4) - 5) << 2;
Review Comment:
> I do not understand this. The `len` value is unconditionally overwritten few lines below. Maybe `len = (tcp->tcpoffset >> 4) << 2;` at line 634 should be relocated to the `else` of `if ((tcp->tcpoffset & 0xf0) > 0x50)`?
They're calculated separately, so maybe they're a little redundant. Offset do not contain options length when tcp_ipvx_select, so need to skip options field when TCP_NEWDATA flag set. Should I skip options field in tcp_ipvx_select function with IPvxTCP_HDRLEN? It doesn't feel right either.
--
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