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

Raw socket support #11410

Closed
wants to merge 64 commits into from
Closed

Conversation

mrmonday
Copy link
Contributor

@mrmonday mrmonday commented Jan 8, 2014

This pull request adds raw socket support to rust using libuv.

Some notes:

  • The native backend is now implemented It only supports the libuv backend - the native backend is stubbed out.
  • Tests are not included
  • Documentation follows that of the surrounding code (I can add some if you prefer)
  • Windows does not support raw sockets

Using raw sockets requires root access on the machine, which I assume the test suite does not have (and will not have) access to. The way around this would be to create a dummy library which exposes all the C functions used without actually implementing the functionality - I don't know how best to go about this or how to integrate it with the test-suite, guidance would be helpful.

I can add support for the native backend if required - there's no reason it can't be there, it just didn't exist while I was writing support. I have tested this on all the platforms I have available to me under various scenarios (transport layer/ipv4/ipv6 and network layer).

fcntl() is a vararg function allowing many options to be passed. This commit
adds support for a single argument, as well as adding some options which may be
passed to it.
Adds a new IoFactory method for creating a raw socket.
Native/blocking implementation is stubbed out.
Adds a RawSocketWatcher which creates a non-blocking raw socket using berkeley
sockets. It registers the socket with libuv to handle reading and writing. When
the socket is ready the sendto/recvfrom calls are used to handle sending. Note
that Windows does not support raw sockets.
This is a small weapper around the functionality provided by IoFactory and very
similar to std::io::net::udp.
@emberian
Copy link
Member

emberian commented Jan 8, 2014

Can you add native support? green isn't destined to be the default, and keeping feature parity is important.

@mrmonday
Copy link
Contributor Author

mrmonday commented Jan 8, 2014

I will work on that now.

@alexcrichton
Copy link
Member

This is pretty awesome, nice work! @cmr is right in that sadly we should not be merging this unless there's both native and green support. It looks like it's trivial to add native support though as you've already done the bulk of the work in green!

@alexcrichton
Copy link
Member

Hm, now that I've actually read the PR description, there's a few things that I'm a little concerned about.

Tests are not included

Is there really no way for us to add tests? This is a fairly large chunk of code and it's like to regress without tests.

Documentation follows that of the surrounding code

Documenting the green/native impls isn't so important, but documenting libstd code is paramount.

Windows does not support raw sockets

I'm unhappy that we have cfg(unix) on mod unix because it really is supported by windows. I would rather not have cfg in the mod and just raise an IoError that it's impossible to work on windows.

@mrmonday
Copy link
Contributor Author

mrmonday commented Jan 8, 2014

It looks like it's trivial to add native support though as you've already done the bulk of the work in green!

This is what I'm hoping. I'm working on this now.

Is there really no way for us to add tests? This is a fairly large chunk of code and it's like to regress without tests.

Tests can be added by linking to a dummy library which emulates the functionality of the C functions which are being used. That way no root access is required. I don't know what would be the best way to achieve this with the build system you are using. I agree there should be test cases.

Documenting the green/native impls isn't so important, but documenting libstd code is paramount.

I'll add documentation for the bits of libstd that I have changed.

I'm unhappy that we have cfg(unix) on mod unix because it really is supported by windows. I would rather not have cfg in the mod and just raise an IoError that it's impossible to work on windows.

That was there when I got here! I can't test on Windows right now, I'd be happy to remove the cfg(unix) around raw socket support though and add in an appropriate error.

@alexcrichton
Copy link
Member

If you'd like, anyone with r+ access can push to try for you which will run most of the tests on windows and you can see if it builds, feel free to ask in IRC (or pm me directly)

@mrmonday
Copy link
Contributor Author

mrmonday commented Jan 9, 2014

I have now added native backend support.

How would I go about creating/linking with another library in the test suite? Where in the source tree should I put it? I should be able to put together some tests if I can do this.

let proto = if domain == libc::AF_INET { libc::IPPROTO_IP } else { libc::IPPROTO_IPV6 };
let res = unsafe { libc::setsockopt(socket, proto, libc::IP_HDRINCL, (&one as *libc::c_int) as *libc::c_void, intrinsics::size_of::<libc::c_int>() as u32) };
if res == -1 {
return Err(super::last_error());
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't close the socket on error, I'd recommend creating the RawSocket as soon as possible to let the destructor kick in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is leaving it to the destructor the best way to do this? I seem to recall that there's no guarantee that they will be called?

Copy link
Contributor

Choose a reason for hiding this comment

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

A destructor is guaranteed to be called when a value goes out of scope. Perhaps you're thinking of another language with a focus on garbage collection. A close method doesn't make much sense in Rust.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's probably it, thanks. I'll fix it.

@mrmonday
Copy link
Contributor Author

I've made a ton of changes:

  • Factor out common code between libnative and librustuv into libnetsupport
  • Complete raw socket support for Linux
  • Fix the issues already brought up

I'd like to get this in fairly soon (despite some missing platform support), since it's quite difficult to keep merging the latest changes.

@sfackler sfackler reopened this Mar 15, 2014
@mrmonday
Copy link
Contributor Author

New since the pull was re-opened:

  • Code now builds on Linux/FreeBSD/OS X/Windows
  • Test cases pass on Linux and Windows
  • Test cases need some work on BSD/OS X. I believe they're failing due to limitations in the BSD implementation of raw sockets but need to investigate.

Are you happy with the API? If so I'll get some documentation written up for it.

@alexcrichton
Copy link
Member

This has reached the point that I fear that this no longer belongs in the standard library. The new module is quite extensive, and the new netsupport crate is a little unfortunate. I think that there are a two paths forward that I can think of:

  1. Submit an RFC for inclusion in the standard library
  2. Figure out how to have this as an external library rather than built-in

I would tend to lean towards 2 before 1 because I would think that it would be generally useful, but it also may be difficult.

@mrmonday
Copy link
Contributor Author

What would you do instead of netsupport? There is already a lot of duplicated code between libnative and librustuv, I would argue that the crate (or something else) should be there regardless of this pull.

In order to do this in a separate library there either needs:

  • To be some way to extend the native and rustuv crates outwith the crate itself
  • Some amount of the code I've written to be pulled which another crate can build upon

I agree that there is a lot of content in this pull, and there's also a lot of scope to extend it, for example:

  • Layer two support for Windows/FreeBSD/OS X (significant amounts of code each)
  • XXXPacket/XXXHeader implementations for other protocols

Perhaps a subset of these things could be added to the standard library? The trouble is where to draw the line, since quite a few of the things here would not be required for basic support but are required for testing.

@bill-myers
Copy link
Contributor

Maybe it could be reduced to the minimal code that needs to be in the core libraries?

For example, there's a lot of code that gets put in libstd to encode and decode Ethernet, IP, UDP, TCP packets as well as constants like IpNextHeaderProtocols which seem unnecessary in libstd and could be moved to another crate.

Also, there's a lot of code that attempts to lookup network interface in libnetsupport, and that also seems unnecessary since the raw socket code could just translate sockaddr_ll fields more directly without needing to do get_network_interfaces and so on (which could be done in another crate if necessary).

@alexcrichton
Copy link
Member

Closing due to inactivity.

I like the idea of adding a little support to the green/native implementations if necessary, but leaving most of this outside of the standard library for a more proper and well defined library. Depending on the extent of this library, I would also recommend an RFC for adding a new library.

@mrmonday
Copy link
Contributor Author

Since someone on reddit linked to here in search of raw sockets with Rust:

libpnet is what this pull request became, and the project you're looking for if you want to use raw sockets in Rust, without using the C APIs directly.

To everyone else, sorry for the noise...

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.

None yet

7 participants