ssl: fix SSL write bug

include/envoy/buffer/buffer.h

@@ -59,6 +59,13 @@ class Instance {
    * @return the actual number of slices needed, which may be greater than out_size. Passing
    *         nullptr for out and 0 for out_size will just return the size of the array needed
    *         to capture all of the slice data.
+   * TODO(mattklein123): WARNING: The underlying implementation of this function currently uses
+   * libevent's evbuffer. It has the infuriating property where calling getRawSlices(nullptr, 0)
+   * will return the slices that include all of the buffer data, but not any empty slices at the
+   * end. However, calling getRawSlices(iovec, SOME_CONST), WILL return potentially empty slices
+   * beyond the end of the buffer. Code that is trying to avoid stack overflow by limiting the
+   * number of returned slices needs to deal with this. When we get rid of evbuffer we can rework
+   * all of this.
    */
   virtual uint64_t getRawSlices(RawSlice* out, uint64_t out_size) const PURE;
 

source/common/ssl/connection_impl.cc

@@ -134,22 +134,21 @@ Network::ConnectionImpl::IoResult ConnectionImpl::doWriteToSocket() {
     }
   }
 
+  uint64_t original_buffer_length = write_buffer_.length();
   uint64_t total_bytes_written = 0;
   bool keep_writing = true;
-  while ((write_buffer_.length() > 0) && keep_writing) {
+  while ((original_buffer_length != total_bytes_written) && keep_writing) {
     // Protect against stack overflow if the buffer has a very large buffer chain.
-    // TODO(mattklein123): The current evbuffer Buffer::Instance implementation will iterate through
-    // the entire chain each time this is called to determine how many slices would be needed. In
-    // this case, we don't care, and only want to fill up to MAX_SLICES. When we swap out evbuffer
-    // we can change this behavior.
+    // TODO(mattklein123): See the comment on getRawSlices() for why we have to also check
+    // original_buffer_length != total_bytes_written during loop iteration.
     // TODO(mattklein123): As it relates to our fairness efforts, we might want to limit the number
     // of iterations of this loop, either by pure iterations, bytes written, etc.
     const uint64_t MAX_SLICES = 32;
     Buffer::RawSlice slices[MAX_SLICES];
-    uint64_t num_slices = std::min(MAX_SLICES, write_buffer_.getRawSlices(slices, MAX_SLICES));
+    uint64_t num_slices = write_buffer_.getRawSlices(slices, MAX_SLICES);
 
     uint64_t inner_bytes_written = 0;
-    for (uint64_t i = 0; i < num_slices; i++) {
+    for (uint64_t i = 0; (i < num_slices) && (original_buffer_length != total_bytes_written); i++) {
       // SSL_write() requires that if a previous call returns SSL_ERROR_WANT_WRITE, we need to call
       // it again with the same parameters. Most implementations keep track of the last write size.
       // In our case we don't need to do that because: a) SSL_write() will not write partial

test/common/ssl/connection_impl_test.cc

@@ -248,7 +248,7 @@ TEST(SslConnectionImplTest, SslError) {
 class SslReadBufferLimitTest : public testing::Test {
 public:
   void readBufferLimitTest(uint32_t read_buffer_limit, uint32_t expected_chunk_size,
-                           uint32_t write_size, uint32_t num_writes) {
+                           uint32_t write_size, uint32_t num_writes, bool reserve_write_space) {
     Stats::IsolatedStoreImpl stats_store;
     Event::DispatcherImpl dispatcher;
     Network::TcpListenSocket socket(uint32_t(10000), true);
@@ -325,21 +325,35 @@ class SslReadBufferLimitTest : public testing::Test {
 
     for (uint32_t i = 0; i < num_writes; i++) {
       Buffer::OwnedImpl data(std::string(write_size, 'a'));
+
+      // Incredibly contrived way of making sure that the write buffer has an empty chain in it.
+      if (reserve_write_space) {
+        Buffer::RawSlice iovecs[2];
+        EXPECT_EQ(2UL, data.reserve(16384, iovecs, 2));
+        iovecs[0].len_ = 0;
+        iovecs[1].len_ = 0;
+        data.commit(iovecs, 2);
+      }
+
       client_connection->write(data);
     }
 
     dispatcher.run(Event::Dispatcher::RunType::Block);
   }
 };
 
-TEST_F(SslReadBufferLimitTest, NoLimit) { readBufferLimitTest(0, 256 * 1024, 256 * 1024, 1); }
+TEST_F(SslReadBufferLimitTest, NoLimit) {
+  readBufferLimitTest(0, 256 * 1024, 256 * 1024, 1, false);
+}
+
+TEST_F(SslReadBufferLimitTest, NoLimitReserveSpace) { readBufferLimitTest(0, 512, 512, 1, true); }
 
 TEST_F(SslReadBufferLimitTest, NoLimitSmallWrites) {
-  readBufferLimitTest(0, 256 * 1024, 1, 256 * 1024);
+  readBufferLimitTest(0, 256 * 1024, 1, 256 * 1024, false);
 }
 
 TEST_F(SslReadBufferLimitTest, SomeLimit) {
-  readBufferLimitTest(32 * 1024, 32 * 1024, 256 * 1024, 1);
+  readBufferLimitTest(32 * 1024, 32 * 1024, 256 * 1024, 1, false);
 }
 
 } // Ssl