avoid hGetMetered 0 closing the handle

This is an edge case, which happened to be triggered by the P2P protocol
seeing DATA 0. When reading 0 bytes, getting an empty string does
not mean the handle has reached EOF.

I verified there was in fact a bug, where get of an empty file followed
by another file would get the empty file and then fail
with "handle is closed". This fixes it.

This commit was sponsored by Boyd Stephen Smith Jr. on Patreon.

diff --git a/CHANGELOG b/CHANGELOG
index f2e4b1950958977cb82a1109120f4af3dd11da2f..18890f78f039ed481e3c7972aa20e9510033c3ab 100644 (file)
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -2,6 +2,8 @@ git-annex (8.20201128) UNRELEASED; urgency=medium
 
   * Fix hang on shutdown of external special remote using ASYNC protocol
     extension. (Reversion introduced in version 8.20201007.)
+  * Fix bug that made the next download after an empty file from a ssh
+    or tor remote fail.
 
  -- Joey Hess <id@joeyh.name>  Mon, 30 Nov 2020 12:55:49 -0400
 

diff --git a/Utility/Metered.hs b/Utility/Metered.hs
index 888e7e14162f229102e904f1cc4a4d97efb78b5d..3e2333cc716d3522b6cae0e782675197b1c1043b 100644 (file)
--- a/Utility/Metered.hs
+++ b/Utility/Metered.hs
@@ -169,8 +169,9 @@ hGetMetered h wantsize meterupdate = lazyRead zeroBytesProcessed
                c <- S.hGet h (nextchunksize (fromBytesProcessed sofar))
                if S.null c
                        then do
-                               hClose h
-                               return $ L.empty
+                               when (wantsize /= Just 0) $
+                                       hClose h
+                               return L.empty
                        else do
                                let !sofar' = addBytesProcessed sofar (S.length c)
                                meterupdate sofar'

diff --git a/doc/bugs/p2p_protocol_misbehavior_when_location_log_out_of_date.mdwn b/doc/bugs/p2p_protocol_misbehavior_when_location_log_out_of_date.mdwn
index 44f0639822bb8be768c6076a38db1a9ad7db872a..9e967a20007b3dfeb95ea52f989c0bffc2ade11b 100644 (file)
--- a/doc/bugs/p2p_protocol_misbehavior_when_location_log_out_of_date.mdwn
+++ b/doc/bugs/p2p_protocol_misbehavior_when_location_log_out_of_date.mdwn
@@ -13,3 +13,31 @@ The problem is on the first move, the protocol does not handle a file
 that's not present well, so it's not clear why it failed. And since that
 closes the connection, the next move fails when it should not need to.
 --[[Joey]]
+
+> Protocol dump:
+> 
+>      P2P > VERSION 1
+>      P2P < VERSION 1
+>      P2P > GET 0 foo SHA256E-s30--f8f8766a836fb6120abf4d5328ce8761404e437529e997aaa0363bdd4fecd7bb
+>      P2P < GET 0 foo SHA256E-s30--f8f8766a836fb6120abf4d5328ce8761404e437529e997aaa0363bdd4fecd7bb
+>      P2P > DATA 0
+>      P2P > VALID
+>      P2P < DATA 0
+>      P2P > SUCCESS
+>      P2P < SUCCESS
+> 
+> So it's sending an empty object and claiming it's valid when in
+> fact it does not have the object. That was done because the sender
+> does not have any other way in the protocol to indicate that.
+> The receiver is what sends SUCCESS/FAILURE, not the sender.a
+> 
+> Seems like, it could send DATA 0 followed by INVALID,
+> to avoid needing to add to the protocol. That should avoid
+> the spurious "verification of content failed".
+> 
+> But what causes the connection to get closed? It seems that
+> while the server sends VALID, the client never debugs that it received
+> it. Indeeed, the receiveMessage call that should receive it
+> fails because the handle is closed at that point. Seems that
+> this is caused by trying to receive 0 bytes as indicated by DATA
+> ending up closing the handle.