Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial commit of updated IPv6 #307

Merged
merged 2 commits into from
Aug 18, 2023
Merged

Initial commit of updated IPv6 #307

merged 2 commits into from
Aug 18, 2023

Conversation

RoyalStewart90
Copy link
Contributor

It was discovered during Keysight's testing of our local fork of GNUStep Master that IPv6 functionality wasn't working properly. While this Pull Request doesn't fix all areas of IPv6, it at least gets sockets properly sized for IPv6 addresses. Please feel free to reach out to me with any questions or comments. If I missed anything I will happily provide it.

@RoyalStewart90 RoyalStewart90 requested a review from rfm as a code owner July 25, 2023 16:12
@gcasa
Copy link
Member

gcasa commented Jul 26, 2023

@RoyalStewart90 Hey, Royal. Please commit any changes you have to get these changes building correctly under non-Windows platforms.

@gcasa
Copy link
Member

gcasa commented Aug 16, 2023

@rfm Can you review this? It looks like a fairly simple change.

@gcasa gcasa requested a review from fredkiefer August 17, 2023 07:54
Copy link
Contributor

@rfm rfm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks

@rfm rfm merged commit 5912f4e into master Aug 18, 2023
9 checks passed
@rfm
Copy link
Contributor

rfm commented Aug 21, 2023

I ended up just adding the code in win32/NSStream.m to create IPv6 streams if creating v4 streams fails.
I couldn't make sense of the use of 'struct socket_storage' ... it just seemed to add complexity because the ipv6 code already uses sockaddr_any (which contains struct sockaddr_in6 and should therefore be correctly sized).
If I missed something, please let me know.

@RoyalStewart90
Copy link
Contributor Author

RoyalStewart90 commented Sep 15, 2023

Hi @rfm, we need to use 'struct socket_storage' because it guarantees the correct size is always allocated for v6 and v4. It was during dealloc that Windows and Linux would throw an exception saying that there is a discrepancy between what the system thought the size was and the actual size. Microsoft recommends using 'sockaddr_storage' https://learn.microsoft.com/en-us/previous-versions/windows/desktop/legacy/ms740504(v=vs.85) . Here is more information about the struct from Ubuntu: https://manpages.ubuntu.com/manpages/lunar/man3/sockaddr.3type.html

@gcasa
Copy link
Member

gcasa commented Sep 19, 2023

@rfm I have tested @RoyalStewart90 's changes and they work fine for me on Linux and on windows.

@RoyalStewart90
Copy link
Contributor Author

Hi @rfm, I don't mean to be a bother, please let me know if there's any questions or work that's needed. I noticed this is the "reverted" PR. Happy to make a new PR if that's needed.

@rfm
Copy link
Contributor

rfm commented Sep 24, 2023

The patch was reverted because it produced a lot of compiler warnings without obviously fixing anything, and on systems without sockaddr_storage would cause compilation failure (the use of sockaddr_storage in general seems bogus, because the sockaddr_any struct already guarantees sufficient memory and correct memory alignment). I did commit a change to use sockaddr_storage as a failsafe on any system where a) it is provided and b) sockaddr_in6 is incorrectly declared.
Presumably somewhere in the change to use sockaddr_storage is hidden the fix for the actual bug, but I haven't spotted it yet. Do you have testcases we can use to demonstrate the issue you are having (so we can track down what the cause is?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants