Rubensei / windivert-rust

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

Eliminate Option from single recv #5

Closed AriFordsham closed 1 year ago

AriFordsham commented 1 year ago

This seems to be unused.

Rubensei commented 1 year ago

Hi, thanks for the contribution!

WinDivertRecv buffer is optional because you are not forced to capture the data even in the layers that do support capturing:

pPacket: An optional buffer for the captured packet.

For layers that do not capture packets/data, the pPacket parameter should be NULL and packetLen should be zero.

AriFordsham commented 1 year ago

Understood!

AriFordsham commented 1 year ago

Passing None to WinDivertRecv gives me an immediate InsufficientBuffer error.

AriFordsham commented 1 year ago

This error can be ignored if the application only intends to receive part of the packet, e.g., the IP headers only.

The problem is that the windivert-rs API returns Option<>, which does not support ignoring errors.

This is a problem also for ERROR_HOST_UNREACHABLE, which signals an infinite packet loop and is intended to be ignored and still deliver the packet.

Rubensei commented 1 year ago

The problem is that the windivert-rs API returns Option<>, which does not support ignoring errors.

But it does return Result<_, WinDivertError> (?)

This error can be ignored if the application only intends to receive part of the packet, e.g., the IP headers only.

This one is trickier. I'm not sure how to handle partial results where both the data and the error need to be returned tbh 🤔

This is a problem also for ERROR_HOST_UNREACHABLE, which signals an infinite packet loop and is intended to be ignored and still deliver the packet.

I don't understand this last part. This should be returned as an error, although it's true that I didn't notice that one and it will appear as a generic OS error 1232 instead of a specific one.

And about delivery, I believe it can't be done, if that error is returned the driver itself is rejecting the packet. Forcing that error is even a recommended way of dropping batched packets without needing to copy/move.

AriFordsham commented 1 year ago

But it does return Result<_, WinDivertError>

Pardon, that's what I meant.

Regarding ERROR_HOST_UNREACHABLE, I am referring to https://github.com/basil00/Divert/issues/41, particularly https://github.com/basil00/Divert/issues/41#issuecomment-338377976, where ERROR_HOST_UNREACHABLE is used t o signal the infinite loop condition, and still delivers the packet.

The hacky solution would be to add a field to the enum variants that still have a packet, returning the packet. The other option is returning (Option<WinDivertError>, Option<WinDivertPacket), not elegant at all 😬

AriFordsham commented 1 year ago

You might want this: https://docs.rs/these/latest/these/

Rubensei commented 1 year ago

[...] since the looping packet will eventually be dropped.

I fail to see the part where the packet is delivered. Specially since the comment includes the sentence above.

You might want this: https://docs.rs/these/latest/these/

It's an option but I don't like the API at all tbh. Maybe adding something like partial_recv(_ex) where that error is ignored. This way it wouldn't even be a breaking change