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

Refactoring sockaddr Handling #42

Open
Yaxuan-w opened this issue Nov 19, 2024 · 5 comments
Open

Refactoring sockaddr Handling #42

Yaxuan-w opened this issue Nov 19, 2024 · 5 comments
Assignees
Labels
enhancement New feature or request RawPOSIX RawPOSIX related issue Refactor File structure refactor specific

Comments

@Yaxuan-w
Copy link
Member

Description:

This issue tracks the refactoring of sockaddr handling and addresses the problems encountered with accept_syscall.

Background:

Currently, sockaddr is initialized using the default Unix GenSockaddr borrowed from RustPOSIX, which has the largest memory space. This may cause issues with getpeername, as IPv4/IPv6 shouldn't have a path field.

It's been observed that byte-level operations should be preferred over struct-type checks.

In Linux, there are various sockaddr structures, such as sockaddr_in for IPv4 and sockaddr_in6 for IPv6. However, tracing through the PostgreSQL source code shows that PostgreSQL uses sockaddr_storage, which is cast to sockaddr when necessary. The key difference between sockaddr and sockaddr_storage is size, with sockaddr_storage being large enough to accommodate all sockaddr types.

PostgreSQL's accept syscall passes a sockaddr->family=0 with sock_len=128, making it impossible to determine the family based on size alone.

Problem:

When using accept, recvfrom, getsockname, or getpeername, issues arise because the wrong sockaddr family is being inferred. The system receives a NULL sockaddr, and the copy_out function from RustPOSIX doesn't perform any operations.

Current Patch

I made a temporary modification to accept_syscall to make RawPOSIX work for now:

  • Initialize a default GenSockaddr struct based on the sockaddr family received at the dispatcher stage.
  • Handle the UNIX path conversion within the syscall itself.
  • Additionally, in the copy_out function, the number of bytes to copy will be determined by comparing initaddrlen with the actual length of the structure (taking the minimum value), ensuring that no more than the reserved space is copied.

Proposed Further Solution:

To resolve this, I suggest:

  • Refining the GenSockAddr data structure. Directly allocate a buffer of size 128 bytes for syscalls like accept, recvfrom, getsockname, and getpeername.
  • Implement a new function: Pass pointers to avoid NULL values being sent to syscalls.
@Yaxuan-w Yaxuan-w added enhancement New feature or request RawPOSIX RawPOSIX related issue labels Nov 19, 2024
@Yaxuan-w Yaxuan-w added the Refactor File structure refactor specific label Jan 27, 2025
@robinyuan1002 robinyuan1002 self-assigned this Feb 5, 2025
@Yaxuan-w
Copy link
Member Author

Yaxuan-w commented Feb 5, 2025

I’d like to remind that after modifying the structure, we also need to update the relevant type conversion logic accordingly

@Yaxuan-w
Copy link
Member Author

Yaxuan-w commented Feb 9, 2025

Talking with @robinyuan1002 a bit about actual implementation here.

Now we proposed two solutions:

1. Directly translate using libc::sockaddr_un

This approach directly parses all incoming arguments into libc::sockaddr_un, checking the sa_family field before performing path translation for Unix domain sockets.

Pros: Simple to implement

Cons: If we implement network grates in the future (ie: in memory domain sockets etc.), this would require more modifications

2. Define a more complex data structure

Proposed by @robinyuan1002, this solution involves storing the sockaddr structure (retrieved from glibc/wasmtime) in rawposix after parsing and performing path translation using a custom data structure.

Pros: Will be easier to handle situations for future grate implementation

Cons: Increased code complexity

@JustinCappos
Copy link
Member

Cons: If we implement network grates in the future (ie: in memory domain sockets etc.), this would require more modifications

Would this be true because you need to do path manipulation on these? What part requires modification?

@Yaxuan-w
Copy link
Member Author

Cons: If we implement network grates in the future (ie: in memory domain sockets etc.), this would require more modifications

Would this be true because you need to do path manipulation on these? What part requires modification?

Oh I just realized that the effort of implementing those options is roughly same now. Previously since RawPOSIX and 3i are in same lib, my thought was to use extra mapping table for cageid-sockaddr in order to allow grate serves as external lib and avoid cyclic import. But I have separated cage-vmmap structure / RawPOSIX / 3i to 3 different libs now, the implementation complexity should be roughly same..?

@robinyuan1002 has a drafted data structure of option 2 by using third-party lib(net). I'm not sure do we want to implement as much as by our own even our codebase would be a bit larger to minimize third party lib usage?

@JustinCappos
Copy link
Member

Oh I just realized that the effort of implementing those options is roughly same now. Previously since RawPOSIX and 3i are in same lib, my thought was to use extra mapping table for cageid-sockaddr in order to allow grate serves as external lib and avoid cyclic import. But I have separated cage-vmmap structure / RawPOSIX / 3i to 3 different libs now, the implementation complexity should be roughly same..?

You can safely ignore the fact that grates exist when thinking about RawPOSIX. Even 3i ignores these details except for the fact that it implements the calls to register system call handlers.

@robinyuan1002 has a drafted data structure of option 2 by using third-party lib(net). I'm not sure do we want to implement as much as by our own even our codebase would be a bit larger to minimize third party lib usage?

Yes, unfortunately, I think we don't want to go this route. I think the first path makes much more sense. If you're doing anything special to handle this (except removing code and calling through via fdtables), I think you're doing it wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request RawPOSIX RawPOSIX related issue Refactor File structure refactor specific
Projects
None yet
Development

No branches or pull requests

3 participants