Patchwork [2,of,2] httppeer: use util.readexactly() to abort on incomplete responses

login
register
mail settings
Submitter Anton Shestakov
Date Sept. 10, 2018, 5:49 a.m.
Message ID <91dd8310bca9f336290e.1536558564@neuro>
Download mbox | patch
Permalink /patch/34445/
State Accepted
Headers show

Comments

Anton Shestakov - Sept. 10, 2018, 5:49 a.m.
# HG changeset patch
# User Anton Shestakov <av6@dwimlabs.net>
# Date 1536415131 -28800
#      Sat Sep 08 21:58:51 2018 +0800
# Node ID 91dd8310bca9f336290e883b0d03d1521f259165
# Parent  f3293d747c49a77677fcb00554eecb09fe5502cb
httppeer: use util.readexactly() to abort on incomplete responses

Plain resp.read(n) may not return exactly n bytes when we need, and to detect
such cases before trying to interpret whatever has been read, we can use
util.readexactly(), which raises an Abort when stream ends unexpectedly. In the
first case here, readexactly() prevents a traceback with struct.error, in the
second it avoids looking for invalid compression engines.

In this test case, _wraphttpresponse doesn't catch the problem (presumably
because it doesn't know transfer encoding), and the code continues reading the
response until it gets to compression engine data. Maybe there should be checks
before the execution gets there, but I'm not sure where (httplib?)

Patch

diff --git a/mercurial/httppeer.py b/mercurial/httppeer.py
--- a/mercurial/httppeer.py
+++ b/mercurial/httppeer.py
@@ -401,8 +401,8 @@  def parsev1commandresponse(ui, baseurl, 
     elif version_info == (0, 2):
         # application/mercurial-0.2 always identifies the compression
         # engine in the payload header.
-        elen = struct.unpack('B', resp.read(1))[0]
-        ename = resp.read(elen)
+        elen = struct.unpack('B', util.readexactly(resp, 1))[0]
+        ename = util.readexactly(resp, elen)
         engine = util.compengines.forwiretype(ename)
 
         resp = engine.decompressorreader(resp)
diff --git a/tests/test-http-bad-server.t b/tests/test-http-bad-server.t
--- a/tests/test-http-bad-server.t
+++ b/tests/test-http-bad-server.t
@@ -464,6 +464,26 @@  TODO this output is terrible
 
   $ rm -f error.log
 
+Server stops before it sends transfer encoding
+
+  $ hg serve --config badserver.closeaftersendbytes=959 -p $HGPORT -d --pid-file=hg.pid -E error.log
+  $ cat hg.pid > $DAEMON_PIDS
+
+  $ hg clone http://localhost:$HGPORT/ clone
+  requesting all changes
+  abort: stream ended unexpectedly (got 0 bytes, expected 1)
+  [255]
+
+  $ killdaemons.py $DAEMON_PIDS
+
+  $ tail -4 error.log
+  write(41 from 41) -> (25) Content-Type: application/mercurial-0.2\r\n
+  write(25 from 28) -> (0) Transfer-Encoding: chunke
+  write limit reached; closing socket
+  write(36) -> HTTP/1.1 500 Internal Server Error\r\n
+
+  $ rm -f error.log
+
 Server sends empty HTTP body for getbundle
 
   $ hg serve --config badserver.closeaftersendbytes=964 -p $HGPORT -d --pid-file=hg.pid -E error.log