From 19928af1abfad88736e41a57aa26fb40df560747 Mon Sep 17 00:00:00 2001
From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Thu, 21 Apr 2016 18:12:20 -0600
Subject: [PATCH] Lack of data at EOF is a parsing error, not a "need more"
 condition.

Branch changes did not distinguish "need more and can expect more" from
"need more but got everything" situations. When one of the inner parsers
that deal with "complete" buffers failed, it would throw
InsufficientInput exception and the higher level parsing code would
interpret that as a sign that more SSL records/fragments are needed,
resulting in stuck transactions instead of parsing errors.

Unlike parsing errors, stuck transactions cannot be bypassed or ignored
so the difference is important.
---
 src/parser/BinaryTokenizer.cc | 13 ++++++++-----
 src/parser/BinaryTokenizer.h  |  9 +++++----
 src/security/Handshake.cc     | 10 ++++++----
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/src/parser/BinaryTokenizer.cc b/src/parser/BinaryTokenizer.cc
index 8ee081657d..f6589a81a1 100644
--- a/src/parser/BinaryTokenizer.cc
+++ b/src/parser/BinaryTokenizer.cc
@@ -15,17 +15,19 @@ BinaryTokenizer::BinaryTokenizer(): BinaryTokenizer(SBuf())
 {
 }
 
-BinaryTokenizer::BinaryTokenizer(const SBuf &data):
+BinaryTokenizer::BinaryTokenizer(const SBuf &data, const bool expectMore):
     context(""),
     data_(data),
     parsed_(0),
-    syncPoint_(0)
+    syncPoint_(0),
+    expectMore_(expectMore)
 {
 }
 
 /// debugging helper that prints a "standard" debugs() trailer
 #define BinaryTokenizer_tail(size, start) \
-    " occupying " << (size) << " bytes @" << (start) << " in " << this;
+    " occupying " << (size) << " bytes @" << (start) << " in " << this << \
+    (expectMore_ ? ';' : '.');
 
 /// logs and throws if fewer than size octets remain; no other side effects
 void
@@ -34,6 +36,7 @@ BinaryTokenizer::want(uint64_t size, const char *description) const
     if (parsed_ + size > data_.length()) {
         debugs(24, 5, (parsed_ + size - data_.length()) << " more bytes for " <<
                context << description << BinaryTokenizer_tail(size, parsed_));
+        Must(expectMore_); // throw an error on premature input termination
         throw InsufficientInput();
     }
 }
@@ -76,9 +79,9 @@ BinaryTokenizer::octet()
 }
 
 void
-BinaryTokenizer::reset(const SBuf &data)
+BinaryTokenizer::reset(const SBuf &data, const bool expectMore)
 {
-    *this = BinaryTokenizer(data);
+    *this = BinaryTokenizer(data, expectMore);
 }
 
 void
diff --git a/src/parser/BinaryTokenizer.h b/src/parser/BinaryTokenizer.h
index 08af24607a..58016767fc 100644
--- a/src/parser/BinaryTokenizer.h
+++ b/src/parser/BinaryTokenizer.h
@@ -22,15 +22,15 @@ public:
     typedef uint64_t size_type; // enough for the largest supported offset
 
     BinaryTokenizer();
-    explicit BinaryTokenizer(const SBuf &data);
+    explicit BinaryTokenizer(const SBuf &data, const bool expectMore = false);
 
     /// restart parsing from the very beginning
     /// this method is for using one BinaryTokenizer to parse independent inputs
-    void reset(const SBuf &data);
+    void reset(const SBuf &data, const bool expectMore);
 
-    /// change input without changing parsing state
+    /// change input state without changing parsing state
     /// this method avoids append overheads during incremental parsing
-    void reinput(const SBuf &data) { data_ = data; }
+    void reinput(const SBuf &data, const bool expectMore) { data_ = data; expectMore_ = expectMore; }
 
     /// make progress: future parsing failures will not rollback beyond this point
     void commit();
@@ -75,6 +75,7 @@ private:
     SBuf data_;
     uint64_t parsed_; ///< number of data bytes parsed or skipped
     uint64_t syncPoint_; ///< where to re-start the next parsing attempt
+    bool expectMore_; ///< whether more data bytes may arrive in the future
 };
 
 #endif // SQUID_BINARY_TOKENIZER_H
diff --git a/src/security/Handshake.cc b/src/security/Handshake.cc
index 34487b0aea..b493078b32 100644
--- a/src/security/Handshake.cc
+++ b/src/security/Handshake.cc
@@ -197,11 +197,11 @@ Security::HandshakeParser::parseModernRecord()
     if (currentContentType != record.type) {
         Must(tkMessages.atEnd()); // no currentContentType leftovers
         fragments = record.fragment;
-        tkMessages.reset(fragments);
+        tkMessages.reset(fragments, true); // true because more fragments may come
         currentContentType = record.type;
     } else {
         fragments.append(record.fragment);
-        tkMessages.reinput(fragments);
+        tkMessages.reinput(fragments, true); // true because more fragments may come
         tkMessages.rollback();
     }
     parseMessages();
@@ -437,7 +437,8 @@ bool
 Security::HandshakeParser::parseServerHello(const SBuf &data)
 {
     try {
-        tkRecords.reinput(data); // data contains _everything_ read so far
+        // data contains everything read so far, but we may read more later
+        tkRecords.reinput(data, true);
         tkRecords.rollback();
         while (!tkRecords.atEnd() && !parseDone)
             parseRecord();
@@ -459,7 +460,8 @@ bool
 Security::HandshakeParser::parseClientHello(const SBuf &data)
 {
     try {
-        tkRecords.reinput(data); // data contains _everything_ read so far
+        // data contains everything read so far, but we may read more later
+        tkRecords.reinput(data, true);
         tkRecords.rollback();
         while (!tkRecords.atEnd() && !parseDone)
             parseRecord();
-- 
2.47.2