Add explicit padding to network structures; add repr_c_implicit_padding lint to help
#16208
Conversation
The `repr_c_implicit_padding` lint checks structures marked a u8 followed by a u64, which would require 7 bytes of padding to get the u64 aligned correctly. The reason for this is that in the BSD socket code, there is some shifty casting between structs which results in data being stored in padding bytes. Since the LLVM Load instruction does not copy padding, this can result in data loss. (This is also a problem in C, but since structs are rarely copied by value, instead using pointers and memcpy(), it is not as serious a problem.) We therefore need explicit padding for the socket structures, and this lint will make sure we get it. This is necessary because the recent pull rust-lang#16081 which is a fix for rust-lang#15763, changes the return value semantics to involve a Load. Without padding fixes, this breaks the Tcp code.
I have only added padding for the Linux structures -- when compiling on other systems (which I don't have available) the compiler will flag which fields require additional padding to be added.
|
I feel like the fact that BSD socket structs are casted around and care about the contents of the padding is something specific to socket code and is completely unnecessary for other libc structs. In fact, it's probably a bad idea to add it to other libc structs, because libc structs in general are translations of C structs, and if the C struct doesn't include explicit padding, neither should the libc struct. Including explicit padding runs the risk of getting it wrong for various platforms. |
|
For what it's worth, at least on Linux the socket structures are the only ones that even have implicit padding to be made explicit. But I agree with what you're saying, we should be more surgical in using this lint. |
|
I'm not sure how you can be more surgical when using it. You can't very well add it to individual structs, these struct definitions aren't going to change. Adding it to the module is too broad. Unless there's a single sub-module that contains all networking-related definitions (and I don't think there is), I suspect the lint just doesn't make sense. |
|
This bug is fixed (at least on Linux), so I have no future use for this lint and won't complain if it's rejected. But the reason I included it is because I wouldn't have been able to confidently find the implicit padding in all of the network structures without it. So the lint is useful for: (a) others to be confident that I've tracked down all the implicit padding with this fix, (b) others to be confident that no implicit padding shows up due to future changes (though as you say, it is pretty unlikely that 30-40 year old network structures are going to change), (c) others to check for implicit padding on other architectures where this might be a problem, (d) other FFI writers encountering the same concern. |
|
Are you sure the padding you've inserted is correct for all target triples? |
|
Should this be considered a bug in all the sockaddr-handling code instead of a bug in the struct definitions? I'm wary of modifying the definitions from what they are in the headers themselves as it may be tricky to get right here and there. |
|
@alexcrichton probably yes, but when I tried to modify the socket code I got even more wary! It's very hard to see where structures are being returned as the wrong type. I attacked this problem from the struct definition side only because it was obvious how to write a lint to mechanically check my thoroughness. Edit: Maybe thoroughness is not so critical -- it is pretty visible if connections are failing or packet checksums are wrong, so there will be bug reports. (In fact the original bug was caught by an existing unit test!) |
|
I would expect that anything returning a |
|
Should this PR be closed now? |
|
Yes, thanks for the reminder @kballard |
The recent pull request
#15763
changes the structure return value semantics to use a LLVM Load instruction. This instruction does not copy padding fields when copying structs, which interacts badly with BSD socket code -- which makes a habit of casting pointers to different structures amongst each other, inadvertently storing important data in struct padding. The result is that Tcp connections cannot be made after the referenced PR.
This is not a problem for C code, which generally uses
memcpyand raw pointers to copy structures around. For Rust, which typically copies structures by value, it is a problem. The solution is to make sure that any part of a structure containing data is an explicit field. We accomplish this by adding a lint,repr_c_implicit_padding, and enabling it forlibc, which emits a warning whenever a structure contains padding. This lets us detect implicit padding and replace it with an explicit field, which will be copied on load/store.This PR will allow PR 15763 to pass the testsuite.
There should be no user-visible changes by this PR, except the new lint that is available. It is off by default.