Problem in implementing siginfo_t for Linux

[qinsoon]

The definition of siginfo_t in Linux (https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/siginfo.h) uses a inlined union type as its last field. Rust did not have support for union types, I guess that was why those fields were left as a padding array in current libc implementation.

I made some attempts to use union (introduced in Rust 1.19) to represent siginfo_t. However the resulting struct is 8 bytes larger than the original one (the one currently in libc, which is the same size as the C struct type). See the code here: https://play.rust-lang.org/?gist=99630206fcc8fd44b452d22552b37793&version=stable

My guess is the 8 more bytes come from two places:

1. the union type is 8 bytes aligned, but the first 3 fields of siginfo_t are 12 bytes (3x 4-bytes integer), so the padding is needed after the first 3 fields, which makes the offset of the union type at 16 bytes (instead of 12 bytes)
2. because the union type is 8 bytes aligned, so the size of the union type cannot be 4*29 (which is not a multiple of the 8 bytes alignment), thus padding is needed at the end to make it a muliple of 8.

I am not sure why the C definition does not have issues like this (probably because it uses inlined union in the struct definition) and how to properly implement siginfo_t in Rust. Please correct me if I am wrong. For my project, I have to extract data from the padding array (which looks awful).

[ndusart]

Yes, I think the issue is that in C, the alignment is done for all the struct after union elements are inserted, then in C we do not align the union itself.
I do not think we can do it in Rust.

We can force the union to be aligned in 4bytes by only using 4bytes fields or less, then for example *mut c_void become [u32; 2] for 64 bits architectures.

It is passing the test now, but it's not a clean solution. We have then to define the size of these arrays conditionally in function of the target while the types which can be longer than 4 bytes are already defined conditionnaly in libc.(when const fn will stabilize, we could use [u8; std::mem::size_of::<*mut c_void>()] though)

The other bad point is that users of libc would be forced to transmute these fields to access them as their correct type.

Maybe the solution would be to let this as is, and let higher-level crate as nix provides a safe interface for the padding bytes ?

[qinsoon]

Thanks for the reply. I was a bit surprised as I thought with the introduction of union types, it is always possible to declare identical types in Rust as in C. I don't see there is a neat way to deal with this case. Providing accessor to the padding bytes always adds overhead (though it doesn't matter in this case - signal handling).

Also nix is totally ignoring this part as well. As for extracting info from siginfo_t and ucontext_t, it seems more convenient to just write the code in C and call form Rust (which is as efficient, but is a bit awkward for a Rust project).

[alexcrichton]

Yeah right now the handling of siginfo_t upstream in rust-lang/rust is pretty nasty. Until we get unions this unfortunately isn't really gonna get much better.

[vojtechkral]

@qinsoon Marking the si_fields_t union as #[repr(C, packed)] instead of just #[repr(C)] makes it the same size as the original one. Is this correct / does this help?

[gnzlbg]

Anyone interested on working on this anytime soon? It appears that @vojtechkral suggest a way to solve this. I can mentor.

[kornelski]

Should this be in the 1.0 roadmap? #547

[alex]

I'd be interested in working on this, at least far enough to get si_addr :-) Would a patch that just added enough to read that be acceptable, or do you want this to solve this completely?

[gnzlbg]

@alex what do you mean by not solving this completely? the proposed change should just work, it will only need a bit of cfg-magic to be active on newer Rust versions.

[alex]

Not adding every single field :-)

[gnzlbg]

As long as the struct layout is correct, if the union field is private, we can do whatever we want (as long as we only use the union on toolchains that support it).

If the question is whether we can make the field a public union, and then add newer union fields incrementally over time, I haven't thought about that, but I think it's a good question. I've asked it here: rust-lang/api-guidelines#196

[alex]

Ah yes, that does suggest a good question: do we want to just expose the underlying unions, or add methods on siginfo_t which access the underlying field?

[gnzlbg]

If C exposes the union we should do that. If C has functions to expose the fields, we can add those. I don't think C has methods, so doing that would be something more appropriate for the nix crate.

[alex]

In C the field containing a union has a name with an underscore `_sifields`, and then there are macros like `#define si_addr _sifields._sigfault._addr`. So you can do `some_siginfo_t.si_addr` (and you can't really have a field named `si_addr` on any other struct, but oh well).

…

On Fri, Apr 12, 2019 at 8:35 AM gnzlbg ***@***.***> wrote: If C exposes the union we should do that. If C has functions to expose the fields, we can add those. I don't think C has methods, so doing that would be something more appropriate for the nix crate. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#716 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAADBD10ePsPypLCiLRuXizwOZMTgjdXks5vgH2RgaJpZM4OxrDb> .

-- All that is necessary for evil to succeed is for good people to do nothing.