(kudu) branch master updated: [subprocess] Fix buffer over-read/overflow in message discard

[al...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

asf-gitbox-commits pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git


The following commit(s) were added to refs/heads/master by this push:
     new c968d85cc [subprocess] Fix buffer over-read/overflow in message discard
c968d85cc is described below

commit c968d85cc9d5ea18cbabf7464cb48bbec51a63a6
Author: Ashwani <ar...@gmail.com>
AuthorDate: Tue Jun 9 21:09:43 2026 +0530

    [subprocess] Fix buffer over-read/overflow in message discard
    
    SubprocessProtocol::DoReadAndDiscard() clears an oversized message off
    the communication channel using a fixed 4K stack buffer, but bounded
    each read() with std::max<ssize_t>(rem, sizeof(buf)) instead of
    std::min(). Since this returns the greater of the two, the read length
    is never held to the buffer size. This causes two issues:
    * Stack buffer overflow: when the discarded payload is larger than
      4K (say, 8K message), a single read() is asked to copy up to 'rem'
      bytes into the 4K buffer, smashing the stack.
    * Valid message subsumption: when the oversized payload is smaller than
      4K, the read still pulls up to 4K, over-reading into and consuming the
      following well-formed message. The next ReceiveMessage() then loses
      that message, breaking the "discard to re-sync the channel" guarantee.
    
    The bug has existed since DoReadAndDiscard() was introduced in KUDU-3450.
    The patch fixes it by limiting each read to std::min(rem, sizeof(buf)),
    matching the Java implementation in doReadAndDiscard.
    
    Add two unit tests covering both scenarios: a deterministic over-read
    and an oversized-payload overflow test.
    
    Change-Id: I73da816132555cb3d29b7bbf8a6821ad3f2dec9c
    Reviewed-on: http://gerrit.cloudera.org:8080/24427
    Reviewed-by: Alexey Serbin <al...@apache.org>
    Tested-by: Alexey Serbin <al...@apache.org>
---
 src/kudu/subprocess/subprocess_protocol.cc    |   2 +-
 src/kudu/subprocess/subprocess_server-test.cc | 103 ++++++++++++++++++++++++++
 2 files changed, 104 insertions(+), 1 deletion(-)

diff --git a/src/kudu/subprocess/subprocess_protocol.cc b/src/kudu/subprocess/subprocess_protocol.cc
index 42667bb3c..943ec2080 100644
--- a/src/kudu/subprocess/subprocess_protocol.cc
+++ b/src/kudu/subprocess/subprocess_protocol.cc
@@ -210,7 +210,7 @@ Status SubprocessProtocol::DoReadAndDiscard(ssize_t size) const {
   ssize_t rem = size;
   while (rem > 0) {
     ssize_t r;
-    RETRY_ON_EINTR(r, read(read_fd_, buf, std::max<ssize_t>(rem, sizeof(buf))));
+    RETRY_ON_EINTR(r, read(read_fd_, buf, std::min<ssize_t>(rem, sizeof(buf))));
     if (r == -1) {
       const int err = errno;
       return Status::IOError("Error reading from pipe", "", err);
diff --git a/src/kudu/subprocess/subprocess_server-test.cc b/src/kudu/subprocess/subprocess_server-test.cc
index b6e234a50..1f8c94462 100644
--- a/src/kudu/subprocess/subprocess_server-test.cc
+++ b/src/kudu/subprocess/subprocess_server-test.cc
@@ -15,6 +15,9 @@
 // specific language governing permissions and limitations
 // under the License.
 
+#include <unistd.h>
+
+#include <cstdint>
 #include <memory>
 #include <ostream>
 #include <string>
@@ -33,6 +36,7 @@
 #include "kudu/subprocess/echo_subprocess.h"
 #include "kudu/subprocess/server.h"
 #include "kudu/subprocess/subprocess.pb.h"
+#include "kudu/subprocess/subprocess_protocol.h"
 #include "kudu/util/env.h"
 #include "kudu/util/metrics.h"
 #include "kudu/util/monotime.h"
@@ -466,6 +470,105 @@ TEST_F(SubprocessServerTest, LargePayloadSize) {
   }
 }
 
+// Regression test for the buffer-overflow / over-read bug in
+// SubprocessProtocol::DoReadAndDiscard(), which used
+// std::max<ssize_t>(rem, sizeof(buf)) for read instead of std::min().
+// These tests drive SubprocessProtocol directly over a pipe (no Java
+// subprocess involved) so the oversized-message discard path is
+// exercised deterministically.
+class SubprocessProtocolTest : public KuduTest {};
+
+// When an oversized message is discarded, DoReadAndDiscard() must consume
+// *exactly* the oversized payload and no more. With the 'std::max' bug, the
+// discard read pulls up to sizeof(buf) (4096) bytes even when the oversized
+// payload is smaller, swallowing the bytes of the *following* well-formed
+// message and desynchronizing the channel.
+//
+// This test fails deterministically against the buggy code (the follow-up
+// message is lost and the next read hits EOF) and passes once the read is
+// bounded by std::min().
+TEST_F(SubprocessProtocolTest, OversizedMessageDoesNotOverReadFollowingMessage) {
+  int fds[2];
+  PCHECK(pipe(fds) == 0);
+  const int read_fd = fds[0];
+  const int write_fd = fds[1];
+  SCOPED_CLEANUP({ close(read_fd); });
+
+  // Cap the reader at 150 bytes: large enough to accept a tiny request, small
+  // enough to reject one carrying a few hundred payload bytes.
+  constexpr uint32_t kMaxMsgBytes = 150;
+
+  // The oversized payload is intentionally well under sizeof(buf) (4096) so
+  // that the buggy std::max() read over-reads into the following message
+  // rather than overflowing the stack buffer (that case is covered below).
+  SubprocessRequestPB send_oversized_req = CreateEchoSubprocessRequestPB(string(300, 'x'));
+  SubprocessRequestPB send_valid_req = CreateEchoSubprocessRequestPB("legit request");
+
+  {
+    SubprocessProtocol writer(SubprocessProtocol::SerializationMode::PB,
+                              SubprocessProtocol::CloseMode::NO_CLOSE_ON_DESTROY,
+                              -1, write_fd, 0);
+    ASSERT_OK(writer.SendMessage(send_oversized_req));
+    ASSERT_OK(writer.SendMessage(send_valid_req));
+  }
+  // Close the write end so that, if the follow-up message is wrongly consumed
+  // while discarding, the reader observes EOF instead of blocking forever.
+  PCHECK(close(write_fd) == 0);
+
+  SubprocessProtocol reader(SubprocessProtocol::SerializationMode::PB,
+                            SubprocessProtocol::CloseMode::NO_CLOSE_ON_DESTROY,
+                            read_fd, -1, kMaxMsgBytes);
+
+  // The first message is rejected as oversized; its payload must be discarded.
+  SubprocessRequestPB received_oversized_req;
+  Status s = reader.ReceiveMessage(&received_oversized_req);
+  ASSERT_TRUE(s.IsIOError()) << s.ToString();
+  ASSERT_STR_CONTAINS(s.ToString(), "exceeds maximum message size");
+
+  // The following well-formed message must still be intact. With the bug it
+  // is swallowed during the discard, so this read returns EOF.
+  SubprocessRequestPB received_valid_req;
+  ASSERT_OK(reader.ReceiveMessage(&received_valid_req));
+  EchoRequestPB echo;
+  ASSERT_TRUE(received_valid_req.request().UnpackTo(&echo));
+  ASSERT_EQ("legit request", echo.data());
+}
+
+// When the oversized payload is larger than the 4096-byte stack buffer, the buggy
+// 'std::max()' limit check makes read() to copy 'rem' (the full payload size) bytes
+// into a 4096-byte stack array in a single call, smashing the stack. This is
+// caught by ASAN. With the 'std::min()' fix the payload is discarded in bounded
+// chunks and the call simply reports the oversized error. The std::min limit
+// aligns with java implementation of doReadAndDiscard as well.
+TEST_F(SubprocessProtocolTest, OversizedMessageDiscardDoesNotOverflowStackBuffer) {
+  int fds[2];
+  PCHECK(pipe(fds) == 0);
+  const int read_fd = fds[0];
+  const int write_fd = fds[1];
+  SCOPED_CLEANUP({ close(read_fd); });
+
+  constexpr uint32_t kMaxMsgBytes = 150;
+  // 8K payload: comfortably larger than the 4096-byte buffer so a single read()
+  // call flows over it.
+  SubprocessRequestPB send_oversized_req = CreateEchoSubprocessRequestPB(string(8 * 1024, 'x'));
+
+  {
+    SubprocessProtocol writer(SubprocessProtocol::SerializationMode::PB,
+                              SubprocessProtocol::CloseMode::NO_CLOSE_ON_DESTROY,
+                              -1, write_fd, 0);
+    ASSERT_OK(writer.SendMessage(send_oversized_req));
+  }
+  PCHECK(close(write_fd) == 0);
+
+  SubprocessProtocol reader(SubprocessProtocol::SerializationMode::PB,
+                            SubprocessProtocol::CloseMode::NO_CLOSE_ON_DESTROY,
+                            read_fd, -1, kMaxMsgBytes);
+  SubprocessRequestPB got;
+  Status s = reader.ReceiveMessage(&got);
+  ASSERT_TRUE(s.IsIOError()) << s.ToString();
+  ASSERT_STR_CONTAINS(s.ToString(), "exceeds maximum message size");
+}
+
 } // namespace subprocess
 } // namespace kudu