(kudu) 01/02: [client] Enhance client side checks for invalid response

[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

commit 38522df9c2e7a4874c8ce79ff332d24d112ab6a1
Author: Ashwani <as...@apache.org>
AuthorDate: Wed Oct 14 15:55:19 2026 +0530

    [client] Enhance client side checks for invalid response
    
    The patch fixes three unverified accesses that could cause crash on
    client side:
    - In Batcher::ProcessWriteResponse, the per_row_errors loop uses a
      server-supplied row_index as a direct index into rpc.ops() and
      afterwards std::moves write_op out of the referenced in-flight RPCs.
      No guard exists for the case where write_op is already null - either
      due to invalid response from a malicious server or any other reason,
      which could cause undefined behaviour at the op->ToString() call
      (null dereference). A null check on write_op is inserted immediately
      after the existing bounds check, before std::move.
    - Response entry's rpc_addresses are accessed directly, with no check
      that the optional registration sub-message exists or that it
      contains any RPC addresses. Sequential guards are added before the
      access and an appropriate error message (Status::Corruption) is sent
      back to the caller.
    - Master-supplied ts info index with no bounds check could result in
      undefined behaviour / crash in a release build if the index is out of
      range. A bounds check is added before the access that returns
      Status::Corruption with the bad index and the actual count. This
      mirrors the identical guard already present in meta_cache.cc for the
      same field.
    
    Change-Id: I2034b1686d2d8dac1e21b38103303e8c1005e16a
    Partially-Generated-By: Cursor <cu...@cursor.com>
    Reviewed-on: http://gerrit.cloudera.org:8080/25148
    Tested-by: Ashwani Raina <ar...@gmail.com>
    Reviewed-by: Alexey Serbin <al...@apache.org>
---
 src/kudu/client/batcher.cc |  7 +++++++
 src/kudu/client/batcher.h  |  1 -
 src/kudu/client/client.cc  | 26 +++++++++++++++++++++++---
 3 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/src/kudu/client/batcher.cc b/src/kudu/client/batcher.cc
index b4490f4d5..62e591387 100644
--- a/src/kudu/client/batcher.cc
+++ b/src/kudu/client/batcher.cc
@@ -968,6 +968,13 @@ void Batcher::ProcessWriteResponse(const WriteRpc& rpc,
                  << SecureDebugString(rpc.resp());
       continue;
     }
+    if (!rpc.ops()[err_pb.row_index()]->write_op) {
+      LOG(ERROR) << "Received a per_row_error for op index " << err_pb.row_index()
+                 << " from tablet " << rpc.tablet_id()
+                 << " but write operation is missing: "
+                 << SecureShortDebugString(err_pb);
+      continue;
+    }
     unique_ptr<KuduWriteOperation> op = std::move(rpc.ops()[err_pb.row_index()]->write_op);
     VLOG(2) << "Error on op " << op->ToString() << ": "
             << SecureShortDebugString(err_pb.error());
diff --git a/src/kudu/client/batcher.h b/src/kudu/client/batcher.h
index be08e9c6e..ad16a78c1 100644
--- a/src/kudu/client/batcher.h
+++ b/src/kudu/client/batcher.h
@@ -29,7 +29,6 @@
 #include "kudu/client/write_op.h"
 #include "kudu/common/txn_id.h"
 #include "kudu/gutil/macros.h"
-#include "kudu/gutil/port.h"
 #include "kudu/gutil/ref_counted.h"
 #include "kudu/util/locks.h"
 #include "kudu/util/memory/arena.h"
diff --git a/src/kudu/client/client.cc b/src/kudu/client/client.cc
index 511218210..7f6549490 100644
--- a/src/kudu/client/client.cc
+++ b/src/kudu/client/client.cc
@@ -632,6 +632,21 @@ Status KuduClient::ListTabletServers(vector<KuduTabletServer*>* tablet_servers)
   RETURN_NOT_OK(data_->ListTabletServers(this, deadline, req, &resp));
   for (int i = 0; i < resp.servers_size(); i++) {
     const ListTabletServersResponsePB_Entry& e = resp.servers(i);
+    DCHECK(e.has_registration()) << "tserver " << e.instance_id().permanent_uuid()
+                                 << " has no registration";
+    if (PREDICT_FALSE(!e.has_registration())) {
+      return Status::Corruption(Substitute(
+          "invalid response from master: tserver $0 has no registration",
+          e.instance_id().permanent_uuid()));
+    }
+    DCHECK_GT(e.registration().rpc_addresses_size(), 0)
+        << "tserver " << e.instance_id().permanent_uuid()
+        << " has no RPC addresses";
+    if (PREDICT_FALSE(e.registration().rpc_addresses_size() == 0)) {
+      return Status::Corruption(Substitute(
+          "invalid response from master: tserver $0 has no RPC addresses",
+          e.instance_id().permanent_uuid()));
+    }
     HostPort hp = HostPortFromPB(e.registration().rpc_addresses(0));
     unique_ptr<KuduTabletServer> ts(new KuduTabletServer);
     ts->data_ = new KuduTabletServer::Data(e.instance_id().permanent_uuid(), hp, e.location());
@@ -731,9 +746,9 @@ Status KuduClient::GetTablet(const string& tablet_id, KuduTablet** tablet) {
   auto add_replica_func = [](const TSInfoPB& ts_info,
                              const RaftPeerPB::Role role,
                              vector<const KuduReplica*>* replicas) {
-    if (ts_info.rpc_addresses_size() == 0) {
-      return Status::IllegalState(Substitute(
-          "No RPC addresses found for tserver $0",
+    if (PREDICT_FALSE(ts_info.rpc_addresses_size() == 0)) {
+      return Status::Corruption(Substitute(
+          "invalid response from master: tserver $0 has no RPC addresses",
           ts_info.permanent_uuid()));
     }
     HostPort hp = HostPortFromPB(ts_info.rpc_addresses(0));
@@ -756,6 +771,11 @@ Status KuduClient::GetTablet(const string& tablet_id, KuduTablet** tablet) {
   }
   // Handle interned replicas.
   for (const auto& r : t.interned_replicas()) {
+    if (r.ts_info_idx() >= static_cast<uint32_t>(resp.ts_infos_size())) {
+      return Status::Corruption(Substitute(
+          "invalid response from master: referenced ts_info idx $0 but only $1 present",
+          r.ts_info_idx(), resp.ts_infos_size()));
+    }
     RETURN_NOT_OK(add_replica_func(resp.ts_infos(r.ts_info_idx()), r.role(), &replicas));
   }