Windows TAP driver issues

[Todd C. Miller]

On Tue, 16 Jan 2018 23:49:30 +0100, Guus Sliepen wrote:

> Hm, there's indeed some differences between the two versions, even apart
> from the fact that one uses a thread and the other the event system. It
> seems that in write_packet(), tinc 1.0 does a ResetEvent() if a
> WriteFile() succeeded immediately, but 1.1 doesn't. Also, in tinc 1.1,
> for reads, ResetEvent() is called right before GetOverlappedResult(),
> and that looks very suspicious... with tinc 1.0 it is called right
> before ReadFile().
>
> Overlapped IO feels alien to me.

I think the 1.1 device.c should call ResetEvent() before calling
WriteFile().  This is simpler than resetting the event after a
successful WriteFile() or error.

Another difference between 1.0 and 1.1 is what happens if the write
hasn't completed when it needs to perform a further write.  In 1.0,
write_packet() returns true when the packet is dropped, whereas in
1.1 it returns false.

I suppose it doesn't make much difference since the return value
is never checked but returning false seems wrong since it was not
a write error.

The diff below resets the write event before WriteFile(), changes
the return value on packet drop to match 1.0, and adds a missing
"device_write_packet.len = 0" on an error path.

Does this make sense to you?

 - todd

diff --git a/src/mingw/device.c b/src/mingw/device.c
index d90c69b..315c08d 100644
--- a/src/mingw/device.c
+++ b/src/mingw/device.c
@@ -311,9 +311,13 @@ static bool write_packet(vpn_packet_t *packet) {
 		   which according to MSDN is a no-no. */
 
 		if(!GetOverlappedResult(device_handle, &device_write_overlapped, &outlen, FALSE)) {
-			int log_level = (GetLastError() == ERROR_IO_INCOMPLETE) ? DEBUG_TRAFFIC : DEBUG_ALWAYS;
-			logger(log_level, LOG_ERR, "Error while checking previous write to %s %s: %s", device_info, device, winerror(GetLastError()));
-			return false;
+			if(GetLastError() != ERROR_IO_INCOMPLETE) {
+				logger(DEBUG_ALWAYS, LOG_ERR, "Error completing previously queued write to %s %s: %s", device_info, device, winerror(GetLastError));
+			} else {
+				logger(DEBUG_TRAFFIC, LOG_ERR, "Previous overlapped write to %s %s still in progress", device_info, device);
+				// drop this packet
+				return true;
+			}
 		}
 	}
 
@@ -321,10 +325,14 @@ static bool write_packet(vpn_packet_t *packet) {
 
 	memcpy(&device_write_packet, packet, sizeof(*packet));
 
+	ResetEvent(device_write_overlapped.hEvent);
+
 	if(WriteFile(device_handle, DATA(&device_write_packet), device_write_packet.len, &outlen, &device_write_overlapped)) {
+		// Write was completed immediately.
 		device_write_packet.len = 0;
 	} else if(GetLastError() != ERROR_IO_PENDING) {
 		logger(DEBUG_ALWAYS, LOG_ERR, "Error while writing to %s %s: %s", device_info, device, winerror(GetLastError()));
+		device_write_packet.len = 0;
 		return false;
 	}