Patchwork [4,of,5,V2] lfs: gracefully handle aborts on the server when corrupt blobs are detected

login
register
mail settings
Submitter Matt Harbison
Date April 14, 2018, 6:47 p.m.
Message ID <6645ae6f5e2a248f23ec.1523731656@Envy>
Download mbox | patch
Permalink /patch/31057/
State Accepted
Headers show

Comments

Matt Harbison - April 14, 2018, 6:47 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1519585633 18000
#      Sun Feb 25 14:07:13 2018 -0500
# Node ID 6645ae6f5e2a248f23ec05df1ff6f48f3377cc47
# Parent  414fdd730760f960aaa9b9e8cfb7853b543c8fca
lfs: gracefully handle aborts on the server when corrupt blobs are detected

The aborts weren't killing the server, but this seems cleaner.  I'm not sure if
it matters to handle the remaining IOError in the test like this, for
consistency.

The error code still feels wrong (especially if the client is trying to download
a corrupt blob) but I don't see anything better in the RFCs, and this is already
used elsewhere because the Batch API spec specifically mentioned this as a
"Validation Error".

Patch

diff --git a/hgext/lfs/blobstore.py b/hgext/lfs/blobstore.py
--- a/hgext/lfs/blobstore.py
+++ b/hgext/lfs/blobstore.py
@@ -152,7 +152,8 @@  class local(object):
 
             realoid = sha256.hexdigest()
             if realoid != oid:
-                raise error.Abort(_('corrupt remote lfs object: %s') % oid)
+                raise LfsCorruptionError(_('corrupt remote lfs object: %s')
+                                         % oid)
 
         self._linktousercache(oid)
 
@@ -526,8 +527,8 @@  def _deduplicate(pointers):
 def _verify(oid, content):
     realoid = hashlib.sha256(content).hexdigest()
     if realoid != oid:
-        raise error.Abort(_('detected corrupt lfs object: %s') % oid,
-                          hint=_('run hg verify'))
+        raise LfsCorruptionError(_('detected corrupt lfs object: %s') % oid,
+                                 hint=_('run hg verify'))
 
 def remote(repo, remote=None):
     """remotestore factory. return a store in _storemap depending on config
@@ -573,3 +574,8 @@  def remote(repo, remote=None):
 
 class LfsRemoteError(error.RevlogError):
     pass
+
+class LfsCorruptionError(error.Abort):
+    """Raised when a corrupt blob is detected, aborting an operation
+
+    It exists to allow specialized handling on the server side."""
diff --git a/hgext/lfs/wireprotolfsserver.py b/hgext/lfs/wireprotolfsserver.py
--- a/hgext/lfs/wireprotolfsserver.py
+++ b/hgext/lfs/wireprotolfsserver.py
@@ -20,6 +20,8 @@  from mercurial import (
     pycompat,
 )
 
+from . import blobstore
+
 HTTP_OK = hgwebcommon.HTTP_OK
 HTTP_CREATED = hgwebcommon.HTTP_CREATED
 HTTP_BAD_REQUEST = hgwebcommon.HTTP_BAD_REQUEST
@@ -276,13 +278,15 @@  def _processbasictransfer(repo, req, res
         #       Content-Length, but what happens if a client sends less than it
         #       says it will?
 
-        # TODO: download() will abort if the checksum fails.  It should raise
-        #       something checksum specific that can be caught here, and turned
-        #       into an http code.
-        localstore.download(oid, req.bodyfh)
+        statusmessage = hgwebcommon.statusmessage
+        try:
+            localstore.download(oid, req.bodyfh)
+            res.status = statusmessage(HTTP_OK if existed else HTTP_CREATED)
+        except blobstore.LfsCorruptionError:
+            _logexception(req)
 
-        statusmessage = hgwebcommon.statusmessage
-        res.status = statusmessage(HTTP_OK if existed else HTTP_CREATED)
+            # XXX: Is this the right code?
+            res.status = statusmessage(422, b'corrupt blob')
 
         # There's no payload here, but this is the header that lfs-test-server
         # sends back.  This eliminates some gratuitous test output conditionals.
@@ -296,9 +300,18 @@  def _processbasictransfer(repo, req, res
         res.status = hgwebcommon.statusmessage(HTTP_OK)
         res.headers[b'Content-Type'] = b'application/octet-stream'
 
-        # TODO: figure out how to send back the file in chunks, instead of
-        #       reading the whole thing.
-        res.setbodybytes(localstore.read(oid))
+        try:
+            # TODO: figure out how to send back the file in chunks, instead of
+            #       reading the whole thing.  (Also figure out how to send back
+            #       an error status if an IOError occurs after a partial write
+            #       in that case.  Here, everything is read before starting.)
+            res.setbodybytes(localstore.read(oid))
+        except blobstore.LfsCorruptionError:
+            _logexception(req)
+
+            # XXX: Is this the right code?
+            res.status = hgwebcommon.statusmessage(422, b'corrupt blob')
+            res.setbodybytes(b'')
 
         return True
     else:
diff --git a/tests/test-lfs-serve-access.t b/tests/test-lfs-serve-access.t
--- a/tests/test-lfs-serve-access.t
+++ b/tests/test-lfs-serve-access.t
@@ -236,7 +236,7 @@  Test a bad checksum sent by the client i
   $ hg -R client push http://localhost:$HGPORT1
   pushing to http://localhost:$HGPORT1/
   searching for changes
-  abort: HTTP error: HTTP Error 500: Internal Server Error (oid=b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c, action=upload)!
+  abort: HTTP error: HTTP Error 422: corrupt blob (oid=b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c, action=upload)!
   [255]
 
   $ echo 'test lfs file' > server/lfs3.bin
@@ -255,7 +255,7 @@  Test a checksum failure during the proce
 
   $ hg --config lfs.url=http://localhost:$HGPORT1/.git/info/lfs \
   >    -R client update -r tip
-  abort: HTTP error: HTTP Error 500: Internal Server Error (oid=276f73cfd75f9fb519810df5f5d96d6594ca2521abd86cbcd92122f7d51a1f3d, action=download)!
+  abort: HTTP error: HTTP Error 422: corrupt blob (oid=276f73cfd75f9fb519810df5f5d96d6594ca2521abd86cbcd92122f7d51a1f3d, action=download)!
   [255]
 
   $ $PYTHON $RUNTESTDIR/killdaemons.py $DAEMON_PIDS
@@ -279,14 +279,14 @@  Test a checksum failure during the proce
   $LOCALIP - - [$LOGDATE$] "GET /?cmd=branchmap HTTP/1.1" 200 - x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull (glob)
   $LOCALIP - - [$LOGDATE$] "GET /?cmd=listkeys HTTP/1.1" 200 - x-hgarg-1:namespace=bookmarks x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull (glob)
   $LOCALIP - - [$LOGDATE$] "POST /.git/info/lfs/objects/batch HTTP/1.1" 200 - (glob)
-  $LOCALIP - - [$LOGDATE$] "PUT /.hg/lfs/objects/b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c HTTP/1.1" 500 - (glob)
+  $LOCALIP - - [$LOGDATE$] "PUT /.hg/lfs/objects/b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c HTTP/1.1" 422 - (glob)
   $LOCALIP - - [$LOGDATE$] "GET /?cmd=capabilities HTTP/1.1" 200 - (glob)
   $LOCALIP - - [$LOGDATE$] "GET /?cmd=batch HTTP/1.1" 200 - x-hgarg-1:cmds=heads+%3Bknown+nodes%3D392c05922088bacf8e68a6939b480017afbf245d x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull (glob)
   $LOCALIP - - [$LOGDATE$] "GET /?cmd=getbundle HTTP/1.1" 200 - x-hgarg-1:bookmarks=1&bundlecaps=HG20%2Cbundle2%3DHG20%250Abookmarks%250Achangegroup%253D01%252C02%252C03%250Adigests%253Dmd5%252Csha1%252Csha512%250Aerror%253Dabort%252Cunsupportedcontent%252Cpushraced%252Cpushkey%250Ahgtagsfnodes%250Alistkeys%250Aphases%253Dheads%250Apushkey%250Aremote-changegroup%253Dhttp%252Chttps%250Arev-branch-cache%250Astream%253Dv2&cg=1&common=525251863cad618e55d483555f3d00a2ca99597e&heads=506bf3d83f78c54b89e81c6411adee19fdf02156+525251863cad618e55d483555f3d00a2ca99597e&listkeys=bookmarks&phases=1 x-hgproto-1:0.1 0.2 comp=$USUAL_COMPRESSIONS$ partial-pull (glob)
   $LOCALIP - - [$LOGDATE$] "POST /.git/info/lfs/objects/batch HTTP/1.1" 200 - (glob)
   $LOCALIP - - [$LOGDATE$] "GET /.hg/lfs/objects/276f73cfd75f9fb519810df5f5d96d6594ca2521abd86cbcd92122f7d51a1f3d HTTP/1.1" 500 - (glob)
   $LOCALIP - - [$LOGDATE$] "POST /.git/info/lfs/objects/batch HTTP/1.1" 200 - (glob)
-  $LOCALIP - - [$LOGDATE$] "GET /.hg/lfs/objects/276f73cfd75f9fb519810df5f5d96d6594ca2521abd86cbcd92122f7d51a1f3d HTTP/1.1" 500 - (glob)
+  $LOCALIP - - [$LOGDATE$] "GET /.hg/lfs/objects/276f73cfd75f9fb519810df5f5d96d6594ca2521abd86cbcd92122f7d51a1f3d HTTP/1.1" 422 - (glob)
 
   $ grep -v '  File "' $TESTTMP/errors.log
   $LOCALIP - - [$ERRDATE$] HG error:  Exception happened while processing request '/.git/info/lfs/objects/batch': (glob)
@@ -301,20 +301,13 @@  Test a checksum failure during the proce
   $LOCALIP - - [$ERRDATE$] HG error:      raise IOError(errno.EIO, '%s: I/O error' % oid) (glob)
   $LOCALIP - - [$ERRDATE$] HG error:  IOError: [Errno 5] b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c: I/O error (glob)
   $LOCALIP - - [$ERRDATE$] HG error:   (glob)
-  $LOCALIP - - [$ERRDATE$] Exception happened during processing request '/.hg/lfs/objects/b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c': (glob)
-  Traceback (most recent call last):
-      self.do_write()
-      self.do_hgweb()
-      for chunk in self.server.application(env, self._start_response):
-      for r in self._runwsgi(req, res, repo):
-      rctx, req, res, self.check_perm)
-      return func(*(args + a), **kw)
-      lambda perm:
-      localstore.download(oid, req.bodyfh)
-      super(badstore, self).download(oid, src)
-      raise error.Abort(_('corrupt remote lfs object: %s') % oid)
-  Abort: corrupt remote lfs object: b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c
-  
+  $LOCALIP - - [$ERRDATE$] HG error:  Exception happened while processing request '/.hg/lfs/objects/b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c': (glob)
+  $LOCALIP - - [$ERRDATE$] HG error:  Traceback (most recent call last): (glob)
+  $LOCALIP - - [$ERRDATE$] HG error:      localstore.download(oid, req.bodyfh) (glob)
+  $LOCALIP - - [$ERRDATE$] HG error:      super(badstore, self).download(oid, src) (glob)
+  $LOCALIP - - [$ERRDATE$] HG error:      % oid) (glob)
+  $LOCALIP - - [$ERRDATE$] HG error:  LfsCorruptionError: corrupt remote lfs object: b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c (glob)
+  $LOCALIP - - [$ERRDATE$] HG error:   (glob)
   $LOCALIP - - [$ERRDATE$] Exception happened during processing request '/.hg/lfs/objects/276f73cfd75f9fb519810df5f5d96d6594ca2521abd86cbcd92122f7d51a1f3d': (glob)
   Traceback (most recent call last):
       self.do_write()
@@ -329,18 +322,11 @@  Test a checksum failure during the proce
       raise IOError(errno.EIO, '%s: I/O error' % oid)
   IOError: [Errno 5] 276f73cfd75f9fb519810df5f5d96d6594ca2521abd86cbcd92122f7d51a1f3d: I/O error
   
-  $LOCALIP - - [$ERRDATE$] Exception happened during processing request '/.hg/lfs/objects/276f73cfd75f9fb519810df5f5d96d6594ca2521abd86cbcd92122f7d51a1f3d': (glob)
-  Traceback (most recent call last):
-      self.do_write()
-      self.do_hgweb()
-      for chunk in self.server.application(env, self._start_response):
-      for r in self._runwsgi(req, res, repo):
-      rctx, req, res, self.check_perm)
-      return func(*(args + a), **kw)
-      lambda perm:
-      res.setbodybytes(localstore.read(oid))
-      blob = self._read(self.vfs, oid, verify)
-      blobstore._verify(oid, 'dummy content')
-      hint=_('run hg verify'))
-  Abort: detected corrupt lfs object: 276f73cfd75f9fb519810df5f5d96d6594ca2521abd86cbcd92122f7d51a1f3d
-  
+  $LOCALIP - - [$ERRDATE$] HG error:  Exception happened while processing request '/.hg/lfs/objects/276f73cfd75f9fb519810df5f5d96d6594ca2521abd86cbcd92122f7d51a1f3d': (glob)
+  $LOCALIP - - [$ERRDATE$] HG error:  Traceback (most recent call last): (glob)
+  $LOCALIP - - [$ERRDATE$] HG error:      res.setbodybytes(localstore.read(oid)) (glob)
+  $LOCALIP - - [$ERRDATE$] HG error:      blob = self._read(self.vfs, oid, verify) (glob)
+  $LOCALIP - - [$ERRDATE$] HG error:      blobstore._verify(oid, 'dummy content') (glob)
+  $LOCALIP - - [$ERRDATE$] HG error:      hint=_('run hg verify')) (glob)
+  $LOCALIP - - [$ERRDATE$] HG error:  LfsCorruptionError: detected corrupt lfs object: 276f73cfd75f9fb519810df5f5d96d6594ca2521abd86cbcd92122f7d51a1f3d (glob)
+  $LOCALIP - - [$ERRDATE$] HG error:   (glob)