search

Rubensei / windivert-rust

Rust bindings and wrapper around WinDivert user library
GNU Lesser General Public License v3.0
43 stars 9 forks source link

Logic Errors in `ChecksumFlags` #2

Closed mhils closed 1 year ago

mhils commented 1 year ago

First off, thank you for the fantastic project! We're currently looking at building mitmproxy's Windows support with this, and it has been pretty awesome so far. 🍰

One thing I noticed in passing today is that the ChecksumFlags implementation looks wrong:

https://github.com/Rubensei/windivert-rust/blob/df70e969774cdbe1648ab07cfd9176ea8c588fc9/windivert-sys/src/bindings/newtypes.rs#L396-L442

set_no_icmp should probably use |=, unset_no_icmp should probably use &=, etc? Maybe it would be even nicer to have a .set_icmp(bool) API instead of set/unset methods. :)

Rubensei commented 1 year ago

Hi, thanks for reporting!

Yeah, that's completely wrong, don't know how it slipped 😅 I'll give it a look later today, or feel free to open a pr.

... Maybe it would be even nicer to have a .set_icmp(bool) API instead of set/unset methods. :)

I used set/unset because I think they provide an expressive builder like API for flag creation, but it's true that outside the creation of flags with values known at compile time they are quite more tedious to use.

For the time being I'd rather have set/unset coexist with a set_xxx_value(&mut self, value: bool) to avoid adding breaking changes to the sys crate.

Rubensei commented 1 year ago

Hotfix released as 0.9.1.

Thanks again for reporting!

mhils commented 1 year ago

Awesome, thank you for the super quick fix! 🍰 😃 🚀