Skip to content

getpeername, getsockname, gethostname and getifaddrs update#307

Merged
rennergade merged 17 commits intodevelopfrom
qianxi_getsockname
Jul 24, 2024
Merged

getpeername, getsockname, gethostname and getifaddrs update#307
rennergade merged 17 commits intodevelopfrom
qianxi_getsockname

Conversation

@qianxichen233
Copy link
Copy Markdown
Contributor

Description

This PR adds some comments and tests for getpeername_syscall, getsockname_syscall, gethostname_syscall and getifaddrs_syscall. A issue was specified in the test: when calling connect without binding to any address, an new address will be automatically assigned to the socket. But we failed to update the corresponding information in the sockhandle, causing the ip and port returned from getsockname_syscall are both 0.

Fixes # (issue)

  1. Add fd range check at the beginning of getpeername_syscall and getsockname_syscall

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Test A - ut_lind_net_getpeername_bad_input
  • Test B - ut_lind_net_getpeername_inet
  • Test C - ut_lind_net_getpeername_unix
  • Test D - ut_lind_net_getpeername_udp
  • Test E - ut_lind_net_getsockname_bad_input
  • Test F - ut_lind_net_getsockname_empty
  • Test G - ut_lind_net_getsockname_inet
  • Test H - ut_lind_net_getsockname_unix
  • Test I - ut_lind_net_gethostname
  • Test J - ut_lind_net_getifaddrs

Checklist:

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been added to a pull request and/or merged in other modules (native-client, lind-glibc, lind-project)

Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs
@qianxichen233 qianxichen233 requested a review from Anway-Agte July 17, 2024 16:34
Copy link
Copy Markdown
Contributor

@namanlalitnyu namanlalitnyu left a comment

Choose a reason for hiding this comment

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

Just one minor comment.

Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Copy link
Copy Markdown
Member

@rupeshkoushik07 rupeshkoushik07 left a comment

Choose a reason for hiding this comment

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

LGTM!

@rennergade rennergade requested a review from JustinCappos July 18, 2024 14:15
Copy link
Copy Markdown
Member

@JustinCappos JustinCappos left a comment

Choose a reason for hiding this comment

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

A few suggestions throughout...

Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/safeposix/syscalls/net_calls.rs Outdated
Comment thread src/tests/networking_tests.rs Outdated
Comment thread src/tests/networking_tests.rs Outdated
Comment thread src/tests/networking_tests.rs Outdated
Comment thread src/tests/networking_tests.rs
Copy link
Copy Markdown
Contributor

@ve1nard ve1nard left a comment

Choose a reason for hiding this comment

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

lgtm!

@JustinCappos
Copy link
Copy Markdown
Member

@qianxichen233 If this is ready for my review again, please ping me so I know. (And follow up on slack in a day or so, if I don't respond.)

Copy link
Copy Markdown
Member

@JustinCappos JustinCappos 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 now. Thanks for the fixes!

@qianxichen233 qianxichen233 requested a review from rennergade July 24, 2024 00:49
@rennergade rennergade merged commit d09c658 into develop Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants