search

rust-lang / socket2

Advanced configuration options for sockets.
https://docs.rs/socket2
Apache License 2.0
683 stars 227 forks source link

Prevent MsgHdr::with_control with empty buffer causes error in macOS #507

Closed youknowone closed 6 months ago

youknowone commented 6 months ago

Otherwise panic when cfg(debug_assertions) will be another good prevention.

youknowone commented 6 months ago

To tell about the valid reasons, I am not familiar with low level socket API that much. At least it was a programmer error for my case. Is panic better?

Thomasdezeeuw commented 6 months ago

To tell about the valid reasons, I am not familiar with low level socket API that much. At least it was a programmer error for my case. Is panic better?

No, I would say not doing anything would be better. We're not going to introduce panics in our API, but hiding errors/problematic code is also not something we want.

youknowone commented 6 months ago

It was a small glitch but painful to debug. I finally could find the buffer is wrong, but it was hidden inside socket2 crate. As a library user, I could see something goes wrong on sendmsg but not on with_control. If with_control is not expected to be called with empty buffer by nature, it could be warned at the moment, instead of deferring it. Maybe the other side of the reason is Debug for MsgHdr was useless to look in it and didn't display its contents. The combination of blackboxed MsgHdr and deferred error took me long time to look around control_buffer.

I changed the patch to debug_assert!() make it neither panic(in real use) nor hiding and deferring error. Please take a review if you agree it is a UX caveat.

Thomasdezeeuw commented 6 months ago

It was a small glitch but painful to debug. I finally could find the buffer is wrong, but it was hidden inside socket2 crate.

Socket2 doesn't hide anything, you pass it an invalid buffer, it will pass that buffer along to the OS.

As a library user, I could see something goes wrong on sendmsg but not on with_control. If with_control is not expected to be called with empty buffer by nature, it could be warned at the moment, instead of deferring it. Maybe the other side of the reason is Debug for MsgHdr was useless to look in it and didn't display its contents. The combination of blackboxed MsgHdr and deferred error took me long time to look around control_buffer.

The layout of MsgHdr depends on the underlying OS, it's not a black box.

I changed the patch to debug_assert!() make it neither panic(in real use) nor hiding and deferring error. Please take a review if you agree it is a UX caveat.

Socket2 is just wrapper around the OS interfaces, so I don't think this fits in socket2.

youknowone commented 6 months ago

I understand your concern. Is there any plan to implement better Debug for MagHdr in that case? Debug can be platform-specific implementation and it will not affect any actual behavior. Minor platforms still can use current implementation

Thomasdezeeuw commented 6 months ago

I understand your concern. Is there any plan to implement better Debug for MagHdr in that case? Debug can be platform-specific implementation and it will not affect any actual behavior. Minor platforms still can use current implementation

A pr for that would be accepted.

Thomasdezeeuw commented 6 months ago

Closing this since I still don't think it's something we want.