[impala] 02/02: IMPALA-5031: Out-of-range enum values are undefined behavior

[ta...@apache.org]

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

tarmstrong pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git

commit 7ea8c5706db5d2e7424d687465d702f01eef8824
Author: Jim Apple <jb...@apache.org>
AuthorDate: Sat May 25 23:06:52 2019 -0700

    IMPALA-5031: Out-of-range enum values are undefined behavior
    
    This patch handles an undefined behavior in the custom cluster tests
    in which a reference to an invalid enum value is used. To prevent the
    invalid value, the bytes are first copied into their underlying
    integer type.
    
    The undefined behavior happens in LdapJdbcTest. The relevant backtrace
    is:
    
        include/c++/4.9.2/bits/stl_algobase.h:199:11: runtime error: load
           of value 8, which is not a valid value for type 'const
           TProtocolVersion::type'
        #0 TProtocolVersion::type const&
           min<TProtocolVersion::type>(TProtocolVersion::type const&,
           TProtocolVersion::type const&)
           include/c++/4.9.2/bits/stl_algobase.h:199:11
        #1 ImpalaServer::OpenSession(TOpenSessionResp&, TOpenSessionReq
           const&) service/impala-hs2-server.cc:304:24
        #2 TCLIServiceProcessor::process_OpenSession(int, TProtocol*,
           TProtocol*, void*)
           generated-sources/gen-cpp/TCLIService.cpp:4953:13
        #3 TCLIServiceProcessor::dispatchCall(TProtocol*, TProtocol*,
           string const&, int, void*)
           generated-sources/gen-cpp/TCLIService.cpp:4926:3
        #4 ImpalaHiveServer2ServiceProcessor::dispatchCall(TProtocol*,
           TProtocol*, string const&, int, void*)
           generated-sources/gen-cpp/ImpalaHiveServer2Service.cpp:505:73
        #5 thrift::TDispatchProcessor::process
           (boost::shared_ptr<TProtocol>, boost::shared_ptr<TProtocol>,
           void*)
           toolchain/thrift-0.9.3-p5/include/thrift/TDispatchProcessor.h:121:12
        #6 thrift::server::TAcceptQueueServer::Task::run()
           rpc/TAcceptQueueServer.cpp:74:26
        #7 ThriftThread::RunRunnable(boost::shared_ptr
           <thrift::concurrency::Runnable>, Promise<unsigned long,
           (PromiseMode)0>*) rpc/thrift-thread.cc:74:13
    
    Change-Id: I63379b4c9d2e4738e729a556108c77fed85e6b64
    Reviewed-on: http://gerrit.cloudera.org:8080/13438
    Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/service/impala-hs2-server.cc | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/be/src/service/impala-hs2-server.cc b/be/src/service/impala-hs2-server.cc
index df1c093..4233f22 100644
--- a/be/src/service/impala-hs2-server.cc
+++ b/be/src/service/impala-hs2-server.cc
@@ -20,17 +20,19 @@
 #include "service/impala-server.inline.h"
 
 #include <algorithm>
+#include <type_traits>
+
+#include <boost/algorithm/string.hpp>
 #include <boost/algorithm/string/join.hpp>
+#include <boost/bind.hpp>
 #include <boost/date_time/posix_time/posix_time_types.hpp>
 #include <boost/unordered_set.hpp>
 #include <jni.h>
-#include <thrift/protocol/TDebugProtocol.h>
-#include <gtest/gtest.h>
-#include <boost/bind.hpp>
-#include <boost/algorithm/string.hpp>
 #include <gperftools/heap-profiler.h>
 #include <gperftools/malloc_extension.h>
+#include <gtest/gtest.h>
 #include <gutil/strings/substitute.h>
+#include <thrift/protocol/TDebugProtocol.h>
 
 #include "common/logging.h"
 #include "common/version.h"
@@ -301,7 +303,14 @@ void ImpalaServer::OpenSession(TOpenSessionResp& return_val,
   state->session_type = TSessionType::HIVESERVER2;
   state->network_address = ThriftServer::GetThreadConnectionContext()->network_address;
   state->last_accessed_ms = UnixMillis();
-  state->hs2_version = min(MAX_SUPPORTED_HS2_VERSION, request.client_protocol);
+  // request.client_protocol is not guaranteed to be a valid TProtocolVersion::type, so
+  // loading it can cause undefined behavior. Instead, we copy it to a value of the
+  // "underlying type" of the enum, then copy it back to state->hs_version only once we
+  // have clamped it to be at most MAX_SUPPORTED_HS2_VERSION.
+  std::underlying_type_t<decltype(request.client_protocol)> protocol_integer;
+  memcpy(&protocol_integer, &request.client_protocol, sizeof(request.client_protocol));
+  state->hs2_version = static_cast<TProtocolVersion::type>(
+      min<decltype(protocol_integer)>(MAX_SUPPORTED_HS2_VERSION, protocol_integer));
   state->kudu_latest_observed_ts = 0;
 
   // If the username was set by a lower-level transport, use it.