TS-2553 Fix a crash in the Metalink plugin

CHANGES

@@ -1,6 +1,8 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache Traffic Server 5.0.0
 
+  *) [TS-2578] Close the client connection when you close TransformTerminus
+
   *) [TS-1893] Add more options to server session control.
 
   *) [TS-2239] Initial ALPN TLS extension support.

plugins/experimental/metalink/Makefile.am

@@ -19,3 +19,9 @@ include $(top_srcdir)/build/plugins.mk
 pkglib_LTLIBRARIES = metalink.la
 metalink_la_SOURCES = metalink.cc
 metalink_la_LDFLAGS = $(TS_PLUGIN_LDFLAGS)
+
+check:
+	for e in test/*; do $$e; done | sed ' #\
+	  s/^ok [0-9]\+/\x1b[1;32m\0\x1b[0m/ #\
+	  s/^not ok [0-9]\|Bail out!\+/\x1b[1;37;41m\0\x1b[0m/ #\
+	  s/#.*/\x1b[33m\0\x1b[0m/'

plugins/experimental/metalink/metalink.cc

@@ -234,7 +234,17 @@ vconn_write_ready(TSCont contp, void * /* edata ATS_UNUSED */)
   /* Initialize data here because can't call TSVConnWrite() before
    * TS_HTTP_RESPONSE_TRANSFORM_HOOK */
   if (!data->output_bufp) {
+
+    /* Avoid failed assert "sdk_sanity_check_iocore_structure(connp) ==
+     * TS_SUCCESS" in TSVConnWrite() if the response is 304 Not Modified */
     TSVConn output_connp = TSTransformOutputVConnGet(contp);
+    if (!output_connp) {
+      TSContDestroy(contp);
+
+      TSfree(data);
+
+      return 0;
+    }
 
     data->output_bufp = TSIOBufferCreate();
     TSIOBufferReader readerp = TSIOBufferReaderAlloc(data->output_bufp);
@@ -259,38 +269,34 @@ vconn_write_ready(TSCont contp, void * /* edata ATS_UNUSED */)
    * nbytes is INT64_MAX.
    *
    * In that case to get it to send a TS_EVENT_VCONN_WRITE_COMPLETE event,
-   * update the downstream nbytes and reenable it. */
+   * update the downstream nbytes and reenable it.  Zero the downstream nbytes
+   * is a shortcut. */
   int ntodo = TSVIONTodoGet(input_viop);
   if (!ntodo) {
-
-    int ndone = TSVIONDoneGet(input_viop);
-    TSVIONBytesSet(data->output_viop, ndone);
+    TSVIONBytesSet(data->output_viop, 0);
 
     TSVIOReenable(data->output_viop);
 
     return 0;
   }
 
   /* Avoid failed assert "sdk_sanity_check_iocore_structure(readerp) ==
-   * TS_SUCCESS" in TSIOBufferReaderAvail() if the status code is 302?  or the
-   * message body is empty? */
+   * TS_SUCCESS" in TSIOBufferReaderAvail() if the client or server disconnects
+   * or the content length is zero.
+   *
+   * Don't update the downstream nbytes and reenable it because if not at the
+   * end yet and can't read any more content then can't compute the digest.
+   *
+   * (There hasn't been a TS_EVENT_VCONN_WRITE_COMPLETE event from downstream
+   * yet so if the response has a "Content-Length: ..." header, it is greater
+   * than the content so far.  ntodo is still greater than zero so if the
+   * response is "Transfer-Encoding: chunked", not at the end yet.) */
   TSIOBufferReader readerp = TSVIOReaderGet(input_viop);
   if (!readerp) {
+    TSContDestroy(contp);
 
-    /* Avoid segfault in TSVIOReenable() if the client disconnected */
-    if (TSVConnClosedGet(contp)) {
-      TSContDestroy(contp);
-
-      TSIOBufferDestroy(data->output_bufp);
-      TSfree(data);
-
-    } else {
-
-      int ndone = TSVIONDoneGet(input_viop);
-      TSVIONBytesSet(data->output_viop, ndone);
-
-      TSVIOReenable(data->output_viop);
-    }
+    TSIOBufferDestroy(data->output_bufp);
+    TSfree(data);
 
     return 0;
   }
@@ -566,6 +572,12 @@ digest_handler(TSCont contp, TSEvent event, void *edata)
 static int
 location_handler(TSCont contp, TSEvent event, void * /* edata ATS_UNUSED */)
 {
+  const char *value;
+  int length;
+
+  /* ATS_BASE64_DECODE_DSTLEN() */
+  char digest[33];
+
   SendData *data = (SendData *) TSContDataGet(contp);
   TSContDestroy(contp);
 
@@ -576,28 +588,20 @@ location_handler(TSCont contp, TSEvent event, void * /* edata ATS_UNUSED */)
 
   /* No: Check if the "Digest: SHA-256=..." digest already exists in the cache */
   case TS_EVENT_CACHE_OPEN_READ_FAILED:
-    {
-      const char *value;
-      int length;
-
-      /* ATS_BASE64_DECODE_DSTLEN() */
-      char digest[33];
 
-      value = TSMimeHdrFieldValueStringGet(data->resp_bufp, data->hdr_loc, data->digest_loc, data->idx, &length);
-      if (TSBase64Decode(value + 8, length - 8, (unsigned char *) digest, sizeof(digest), NULL) != TS_SUCCESS
-          || TSCacheKeyDigestSet(data->key, digest, 32 /* SHA-256 */ ) != TS_SUCCESS) {
-        break;
-      }
+    value = TSMimeHdrFieldValueStringGet(data->resp_bufp, data->hdr_loc, data->digest_loc, data->idx, &length);
+    if (TSBase64Decode(value + 8, length - 8, (unsigned char *) digest, sizeof(digest), NULL) != TS_SUCCESS
+        || TSCacheKeyDigestSet(data->key, digest, 32 /* SHA-256 */ ) != TS_SUCCESS) {
+      break;
+    }
 
-      contp = TSContCreate(digest_handler, NULL);
-      TSContDataSet(contp, data);
+    contp = TSContCreate(digest_handler, NULL);
+    TSContDataSet(contp, data);
 
-      TSCacheRead(contp, data->key);
-      TSHandleMLocRelease(data->resp_bufp, data->hdr_loc, data->digest_loc);
+    TSCacheRead(contp, data->key);
+    TSHandleMLocRelease(data->resp_bufp, data->hdr_loc, data->digest_loc);
 
-      return 0;
-    }
-    break;
+    return 0;
 
   default:
     TSAssert(!"Unexpected event");

plugins/experimental/metalink/test/chunkedEncoding

@@ -0,0 +1,81 @@
+#!/usr/bin/env python
+
+print '''1..1 chunkedEncoding
+# The proxy forwards the final chunk at the end of a chunked response'''
+
+from twisted.internet import error, protocol, reactor, tcp
+from twisted.web import http
+
+def callback():
+  print 'not ok 1 - No final chunk yet'
+
+  reactor.stop()
+
+reactor.callLater(2, callback)
+
+class factory(http.HTTPFactory):
+  class protocol(http.HTTPChannel):
+    class requestFactory(http.Request):
+      def requestReceived(ctx, method, target, version):
+
+        ctx.client = None
+        ctx.clientproto = version
+
+        ctx.write('chunkedEncoding')
+
+        # If the proxy reads the final chunk before it sends the response
+        # headers, it may send a Content-Length header vs. a chunked response
+        reactor.callLater(1, ctx.finish)
+
+server = tcp.Port(0, factory())
+server.startListening()
+
+print '# Listening on {0}:{1}'.format(*server.socket.getsockname())
+
+class factory(protocol.ClientFactory):
+  def clientConnectionFailed(ctx, connector, reason):
+
+    print 'Bail out!'
+    reason.printTraceback()
+
+    reactor.stop()
+
+  class protocol(http.HTTPClient):
+    def connectionLost(ctx, reason):
+      try:
+        reactor.stop()
+
+      except error.ReactorNotRunning:
+        pass
+
+      else:
+        print 'not ok 1 - Did the proxy crash?  (The client connection closed.)'
+
+    connectionMade = lambda ctx: ctx.transport.write('GET {0}:{1} HTTP/1.1\r\n\r\n'.format(*server.socket.getsockname()))
+
+    def handleHeader(ctx, k, v):
+      if k.lower() == 'content-length':
+        print 'not ok 1 - Got a Content-Length header vs. a chunked response'
+
+        # No hope of a final chunk now
+        reactor.stop()
+
+    # Avoid calling undefined handleResponse() at the end of the content (if
+    # the proxy sent a Content-Length header vs. a chunked response).
+    # (Override connectionLost() when the proxy crashes or stop the reactor.)
+    #
+    # The data that was already received will be processed (the end of the
+    # headers), then shutdown events will fire (connections will be closed),
+    # and then finally the reactor will grind to a halt.
+    def handleResponseEnd(ctx):
+      pass
+
+    def handleResponsePart(ctx, data):
+      if data.endswith('0\r\n\r\n'):
+        print 'ok 1 - Got the final chunk'
+
+        reactor.stop()
+
+tcp.Connector('localhost', 8080, factory(), 30, None, reactor).connect()
+
+reactor.run()

plugins/experimental/metalink/test/chunkedEncodingDisconnect

@@ -0,0 +1,81 @@
+#!/usr/bin/env python
+
+print '''1..1 chunkedEncodingDisconnect
+# The proxy closes the client connection and doesn't send a final chunk if the
+# server disconnects without sending one'''
+
+from twisted.internet import error, protocol, reactor, tcp
+from twisted.web import http
+
+def callback():
+  print 'not ok 1 - The client was left hanging'
+
+  reactor.stop()
+
+reactor.callLater(2, callback)
+
+class factory(http.HTTPFactory):
+  class protocol(http.HTTPChannel):
+    class requestFactory(http.Request):
+      def requestReceived(ctx, method, target, version):
+
+        ctx.client = None
+        ctx.clientproto = version
+
+        ctx.write('chunkedEncodingDisconnect')
+
+        # If the server disconnects before the proxy sends the response
+        # headers, the proxy may send a Content-Length header vs. a chunked
+        # response
+        reactor.callLater(1, ctx.transport.loseConnection)
+
+server = tcp.Port(0, factory())
+server.startListening()
+
+print '# Listening on {0}:{1}'.format(*server.socket.getsockname())
+
+class factory(protocol.ClientFactory):
+  def clientConnectionFailed(ctx, connector, reason):
+
+    print 'Bail out!'
+    reason.printTraceback()
+
+    reactor.stop()
+
+  class protocol(http.HTTPClient):
+    def connectionLost(ctx, reason):
+      try:
+        reactor.stop()
+
+      except error.ReactorNotRunning:
+        pass
+
+      else:
+        print 'ok 1 - The client connection closed'
+
+    connectionMade = lambda ctx: ctx.transport.write('GET {0}:{1} HTTP/1.1\r\n\r\n'.format(*server.socket.getsockname()))
+
+    def handleHeader(ctx, k, v):
+      if k.lower() == 'content-length':
+        print 'not ok 1 - Got a Content-Length header vs. a chunked response'
+
+        # Who cares what happens now?
+        reactor.stop()
+
+    # Avoid calling undefined handleResponse() at the end of the content (if
+    # the proxy sent a Content-Length header vs. a chunked response).
+    # (Override connectionLost() when the proxy closes the client connection or
+    # stop the reactor.)
+    def handleResponseEnd(ctx):
+      pass
+
+    def handleResponsePart(ctx, data):
+      if data.endswith('0\r\n\r\n'):
+        print 'not ok 1 - Got a final chunk'
+
+        # Who cares what happens now?
+        reactor.stop()
+
+tcp.Connector('localhost', 8080, factory(), 30, None, reactor).connect()
+
+reactor.run()

plugins/experimental/metalink/test/clientDisconnect

@@ -0,0 +1,78 @@
+#!/usr/bin/env python
+
+print '''1..1 clientDissconnect
+# The proxy doesn't crash if the client disconnects prematurely'''
+
+from twisted.internet import error, protocol, reactor, tcp
+from twisted.web import http
+
+def callback():
+  print 'not ok 1 - Why didn\'t the test finish yet?'
+
+  reactor.stop()
+
+reactor.callLater(3, callback)
+
+class factory(http.HTTPFactory):
+  class protocol(http.HTTPChannel):
+    class requestFactory(http.Request):
+      def requestReceived(ctx, method, target, version):
+
+        ctx.client = None
+        ctx.clientproto = version
+
+        ctx.write('clientDisconnect')
+
+        # The proxy crashes only after the response is complete
+        def callback():
+          try:
+            ctx.finish()
+
+          except RuntimeError:
+            pass
+
+          # Open another connection
+          class factory(protocol.ClientFactory):
+            def clientConnectionFailed(ctx, connector, reason):
+              print 'not ok 1 - Did the proxy crash?  (Can\'t open another connection to it.)'
+
+              reactor.stop()
+
+            class protocol(protocol.Protocol):
+              def connectionMade(ctx):
+                print 'ok 1 - The proxy didn\'t crash (opened another connection to it)'
+
+                reactor.stop()
+
+          reactor.callLater(1, tcp.Connector('localhost', 8080, factory(), 30, None, reactor).connect)
+
+        reactor.callLater(1, callback)
+
+server = tcp.Port(0, factory())
+server.startListening()
+
+print '# Listening on {0}:{1}'.format(*server.socket.getsockname())
+
+class factory(protocol.ClientFactory):
+  def clientConnectionFailed(ctx, connector, reason):
+
+    print 'Bail out!'
+    reason.printTraceback()
+
+    reactor.stop()
+
+  class protocol(http.HTTPClient):
+    def connectionMade(ctx):
+      ctx.transport.write('GET {0}:{1} HTTP/1.1\r\n\r\n'.format(*server.socket.getsockname()))
+
+      # Disconnect after the proxy sends the response headers
+      reactor.callLater(1, ctx.transport.loseConnection)
+
+    # Avoid calling undefined handleResponse() at the end of the content or
+    # when the connection closes
+    def handleResponseEnd(ctx):
+      pass
+
+tcp.Connector('localhost', 8080, factory(), 30, None, reactor).connect()
+
+reactor.run()