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 2020/02/25 00:05:29 UTC
[GitHub] [incubator-nuttx] masayuki2009 opened a new pull request #370: net:
socket: Introduce net_clear_sinzero()
masayuki2009 opened a new pull request #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370
### Summary
- In https://github.com/apache/incubator-nuttx-apps/pull/85, we finally found that sin_zero field in sockaddr_in should be set to zero in the kernel space. This PR introduces net_clear_sinzero() for it.
### Impact
- Some socket APIs such as accept(), getpeername(), getsockname() and recvfrom() are affected, if they are used with IPv4 address.
### Testing
- I tested this PR with spresense:wifi with usrsocktest application which I modified to check all address field. Also, I confirmed spresense:rndis which does not use usrsock also works.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #370: net: socket:
Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590670787
The OS must **NEVER** modify an input sockaddr type (of an variety). The socketaddr type is usually passed as a FAR const struct sockaddr * and the OS is not permitted to modify read-only user values. The input sockaddr structures must **always** be modify by the application logic. The input sockaddr structures may, very possibly, reside in read-only memory.
Returned sockaddr structures are not const and, of course, can be modified by the OS.
The const attribute is a "contract" that does not permit modification of the input value by the OS.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #370: net: socket:
Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590869227
> does it make sense that we call memset on the whole addr before call si_getpeername/si_getsockename/si_accept/si_recvfrom?
I dislike that solution too. It is a ugly hack. We should do it right. sin_zerio is not different then any other field in sockadd_in. The clean, easily unstandable way is:
addr->sin_port = AF_INET;
addr->sin_addr.s_addr = addr;
memset(addr->sin_zero, 0, sizeof(addr->sin_zero));
Let's not take ugly shortcuts.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #370: net:
socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590892853
Yes, this is what I want to say:
1.put to IPv4 case if we just zero sin_zero field or
2.put to common case but we set the whole structure
The mix(set sin_zero in common place) isn't a good implementation.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #370: net: socket:
Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590961398
> But as long as I checked Linux kernel, it clears in the network stack.
Ignorinig the read-only 'const' would be a violation of C principles. We should not do that.
I think it would be legal to test if sin_zero is all zero and, if not, return an error. That is correct, but would probably annoy a lot of people.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull
request #370: net: socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#discussion_r383633427
##########
File path: include/nuttx/net/net.h
##########
@@ -478,6 +478,26 @@ FAR struct iob_s *net_ioballoc(bool throttled, enum iob_user_e consumerid);
int net_checksd(int fd, int oflags);
+/****************************************************************************
+ * Name: net_clear_sinzero()
+ *
+ * Description:
+ * In IPv4, sin_zero field exists in sockaddr_in for padding and should
+ * be set to zero.
+ *
+ * Input Parameters:
+ * from - Address of source (may be NULL)
+ * fromlen - The length of the address structure
+ *
+ * Returned Value:
+ * None
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_NET_IPv4
Review comment:
@xiaoxiang781216 In my understanding, padding field only exists in sockaddr_in (i.e. IPv4 family). So that's why I added ifdef CONFIG_NET_IPv4.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo removed a comment on issue #370: net:
socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo removed a comment on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590671700
It is difficult to review changes from differences. It looks to me like you are setting sin_zero only on values of sockaddr_in returned from the OS. In that case, ignore my preceding rant. The change is OK. The creator/initialize of the sockaddr structure, whether the application or the OS must initialize sin_zero. Never the receiver of sin_zero.
I retract my previous statements in opposition to the change based on the understanding.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #370: net:
socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590865470
@patacongo does it make sense that we call memset on the whole addr before call si_getpeername/si_getsockename/si_accept/si_recvfrom?
It's simple and flexible for any address family which may define the reserved field too.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #370: net: socket:
Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590662062
All examples I have found say it is the application resonsibility:https://stackoverflow.com/questions/26255786/is-this-a-correct-way-of-initializing-struct-sockaddr-in-sin-zero8-as-0
https://silviocesare.wordpress.com/2007/10/22/setting-sin_zero-to-0-in-struct-sockaddr_in/
And others.
This must not go into the OS. It is wrong.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #370: net: socket:
Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590889683
But the memset of the whole structure would be wasteful for IPv6 addresses.
> accept/getsockname/getpeername/recvfrom are general functions, shouldn't assume addr must be IPv4 and then contain sin_zero.
The distinction between IPv4 and IPv6 is made at a lower level than accept/getsockname/getpeername/recvfrom. At that lower level, the IPv4 specific code and always assume that sin_zero is present. If sin_port and sin_addr are set then you know that sin_zero can be set too.. always.
For example:
accept->psock_accept->inet_accept->tcp_psock_accept
then the address is initialized like:
106 #ifdef CONFIG_NET_IPv4
114 {
115 FAR struct sockaddr_in *inaddr = (FAR struct sockaddr_in *)addr;
116
117 inaddr->sin_family = AF_INET;
118 inaddr->sin_port = conn->rport;
119 net_ipv4addr_copy(inaddr->sin_addr.s_addr, conn->u.ipv4.raddr);
120
121 *addrlen = sizeof(struct sockaddr_in);
122 }
123 #endif /* CONFIG_NET_IPv4 */
The memset of sin_zero belongs between lines 119 and 120. Easy and perfect!
sin_zero should be initialized at the very, very lowest level at the same time that sin_port and sin_addr are initialize. That is the simplest and can never fail. I have not looked at every case, but I believe that all reduce to this simplest, most straight-forward, monst maintainable, most understanable solution.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] masayuki2009 commented on a change in pull
request #370: net: socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on a change in pull request #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#discussion_r383635279
##########
File path: net/socket/getpeername.c
##########
@@ -121,7 +123,22 @@ int psock_getpeername(FAR struct socket *psock, FAR struct sockaddr *addr,
return -EOPNOTSUPP;
}
- return psock->s_sockif->si_getpeername(psock, addr, addrlen);
Review comment:
@xiaoxiang781216 Do you mean that if (add != NULL && addrlen > 0), then memset(addr, 0, addrlen) ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #370: net: socket:
Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590863874
Let me be clearer. Whenever logic within the OS inializes a sockaddr_in structure, the code sequence should always be modified to set sin_zero() like:
addr->sin_port = AF_INET;
addr->sin_addr.s_addr = addr;
memset(addr->sin_zero, 0, sizeof(addr->sin_zero));
There should be no centralized logic to try to set sin_zero in some other place.
If the sockaddr_in structure is not created by the OS, it must never modify 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] masayuki2009 commented on issue #370: net:
socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
masayuki2009 commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590678442
> Isn't it the application's responsibility to clear sin_zero,not the OS. Googling for examples suggest it is the application responsibility. If that is there case, there should be no OS change.
@patacongo I thought it's the application's responsibility to clear sin_zero. But as long as I checked Linux kernel, it clears in the network stack. (perhaps to accept network applications having an wrong manner?)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #370: net: socket:
Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590671700
It is difficult to review changes from differences. It looks to me like you are setting sin_zero only on values of sockaddr_in returned from the OS. In that case, ignore my preceding rant. The change is OK. The creator/initialize of the sockaddr structure, whether the application or the OS must initialize sin_zero. Never the receiver of sin_zero.
I retract my previous statements in opposition to the change based on the understanding.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 edited a comment on issue #370:
net: socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 edited a comment on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590892853
Yes, this is what I want to say:
1.put to IPv4 case if we just zero sin_zero field or
2.put to common place but we set the whole structure
The mix(set sin_zero in common place) isn't a good implementation.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo removed a comment on issue #370: net:
socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo removed a comment on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590660615
Isn't it the application's responsibility to clear sin_zero,not the OS. Googling for examples suggest it is the application responsibility. If that is there case, there should be no OS change.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #370: net: socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#discussion_r383617852
##########
File path: net/socket/getpeername.c
##########
@@ -121,7 +123,22 @@ int psock_getpeername(FAR struct socket *psock, FAR struct sockaddr *addr,
return -EOPNOTSUPP;
}
- return psock->s_sockif->si_getpeername(psock, addr, addrlen);
Review comment:
Can we zero out the whole addr before call si_getpeername/si_getsockename/si_accept/si_recvfrom?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #370: net:
socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590869227
> does it make sense that we call memset on the whole addr before call si_getpeername/si_getsockename/si_accept/si_recvfrom?
I dislike that solution too. It is a ugly hack. We should do it right. sin_zerio is not different then any other field in sockadd_in. The clean, easily unstandable way is:
addr->sin_port = AF_INET;
addr->sin_addr.s_addr = addr;
memset(addr->sin_zero, 0, sizeof(addr->sin_zero));
Let's not take ugly shortcuts.
But having said that, it is not as ugly as the original proposal. It is just not as straightforward.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo removed a comment on issue #370: net:
socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo removed a comment on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590670787
The OS must **NEVER** modify an input sockaddr type (of an variety). The socketaddr type is usually passed as a FAR const struct sockaddr * and the OS is not permitted to modify read-only user values. The input sockaddr structures must **always** be modify by the application logic. The input sockaddr structures may, very possibly, reside in read-only memory.
Returned sockaddr structures are not const and, of course, can be modified by the OS.
The const attribute is a "contract" that does not permit modification of the input value by the OS.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #370: net: socket:
Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-591044415
AFAIK nothing in the NuttX network stack cares about the content of sin_zero. There would be nothing gained for NuttX by clearing that field for input sockaddr_in values.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo closed pull request #370: net: socket:
Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo closed pull request #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #370: net: socket:
Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590907342
Okay so I did the work and checked out where all IPv4 sockadd_in structures are initialed. There are many, many more than just accept/getsockname/getpeername/recvfrom .
I added memset(sin_zero, 0, sizeof(sin_zero)); to each of them and create pr377.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #370: net: socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#discussion_r383636673
##########
File path: include/nuttx/net/net.h
##########
@@ -478,6 +478,26 @@ FAR struct iob_s *net_ioballoc(bool throttled, enum iob_user_e consumerid);
int net_checksd(int fd, int oflags);
+/****************************************************************************
+ * Name: net_clear_sinzero()
+ *
+ * Description:
+ * In IPv4, sin_zero field exists in sockaddr_in for padding and should
+ * be set to zero.
+ *
+ * Input Parameters:
+ * from - Address of source (may be NULL)
+ * fromlen - The length of the address structure
+ *
+ * Returned Value:
+ * None
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_NET_IPv4
Review comment:
BSD socket is a general interface, many protocol build on top of it: IPv4, IPv6, Unix domain, Bluetooth etc. Each protocol define his own address repsentation, and then may add the padding field like IPv4. We have to find a general solution.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #370: net: socket:
Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590859692
> I thought it's the application's responsibility to clear sin_zero
For input addresses, the form of the address parameter will be "FAR const struct sockaddr *addr" and the application must set the sin_zero field. **_These are read-only for the OS and must never be modified by the OS._** For example when a struct sockaddr_in is passed to sendto(). These may not be written to under any circumstances (and may even lie in ROM).
If Linux is modifying read-only values, it is screwed up. We do not want duplicate such errors in the this OS.
For output addresses, the form form of the address parameter will be "FAR struct sockaddr *addr). In that case, **_the OS must also set the sin_zero field_** before returning,
I really dislike this solution. I think that the sin_zero field should always be set where ever sin_port and sin_addr is set. You chould not have to check anyhting in those cases just memset() the field. There should not be any centralizing logic to retroactively sets sin_zero.
I am closing this PR because I think it is not correct. It is difficult to understand the logic completely form just looking at the differences, and it may be correct if it is applied ONLY to output addresses. However, even in that case. I do not believe that it is the correct way to implement the zeroing. That should be done when sin_port and sin_addr are set.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #370: net: socket:
Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590870707
People on the internet are saying that leaving the sin_zero field is causing some kinds of issues. I don't know how that could be unless the sin_zero actually contains values in some cases. Is there there some use case where sin_zero would not be zero (for example, some protocol-specific usage).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #370: net: socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#discussion_r383645317
##########
File path: net/socket/getpeername.c
##########
@@ -121,7 +123,22 @@ int psock_getpeername(FAR struct socket *psock, FAR struct sockaddr *addr,
return -EOPNOTSUPP;
}
- return psock->s_sockif->si_getpeername(psock, addr, addrlen);
Review comment:
Yes, something like this, but we need check each implementation don't overwrite the unused field with the random bit 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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #370: net: socket:
Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-591034005
One thing we could do it if matters enough to you is to make a copy of socketadd_in input then clear the sin_zero field in the copy. I don't recommend this for embedded systems but this is most likely what Linux is doing. Linux never accesses any user pointers without going though severl security layers so, for Linux, is it simpler to copy input argument (if they are not large) rather than go through the security measures on every access.
If there are no security macros in the code you are looking at, you can be assured that linux is modifying a copy of the user input, not the actual user input itself. That should never be done for a const input.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a change in pull
request #370: net: socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#discussion_r383618044
##########
File path: include/nuttx/net/net.h
##########
@@ -478,6 +478,26 @@ FAR struct iob_s *net_ioballoc(bool throttled, enum iob_user_e consumerid);
int net_checksd(int fd, int oflags);
+/****************************************************************************
+ * Name: net_clear_sinzero()
+ *
+ * Description:
+ * In IPv4, sin_zero field exists in sockaddr_in for padding and should
+ * be set to zero.
+ *
+ * Input Parameters:
+ * from - Address of source (may be NULL)
+ * fromlen - The length of the address structure
+ *
+ * Returned Value:
+ * None
+ *
+ ****************************************************************************/
+
+#ifdef CONFIG_NET_IPv4
Review comment:
It doesn't make sense to just clear IPv4 address, we should clear all unused field to zero for all address family, otherwise the similiar error will report again and 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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on issue #370: net:
socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590873476
The problem is that:
accept/getsockname/getpeername/recvfrom are general functions, shouldn't assume addr must be IPv4 and then contain sin_zero. If we want to just zero sin_zero, it's better to move the code into inet/.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #370: net: socket:
Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590660615
Isn't it the application's responsibility to clear sin_zero,not the OS. Googling for examples suggest it is the application responsibility. If that is there case, there should be no OS change.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo removed a comment on issue #370: net:
socket: Introduce net_clear_sinzero()
Posted by GitBox <gi...@apache.org>.
patacongo removed a comment on issue #370: net: socket: Introduce net_clear_sinzero()
URL: https://github.com/apache/incubator-nuttx/pull/370#issuecomment-590662062
All examples I have found say it is the application resonsibility:https://stackoverflow.com/questions/26255786/is-this-a-correct-way-of-initializing-struct-sockaddr-in-sin-zero8-as-0
https://silviocesare.wordpress.com/2007/10/22/setting-sin_zero-to-0-in-struct-sockaddr_in/
And others.
This must not go into the OS. It is wrong.
----------------------------------------------------------------
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
With regards,
Apache Git Services