fix potential nullptr access

arangodb · jsteemann · Dec 11, 2020 · Oct 30, 2020 · Nov 9, 2020 · Nov 9, 2020
commit 0af7ff72963be71f228b155a84cb0edfacaed6b3
diff --git a/arangod/Agency/AsyncAgencyComm.cpp b/arangod/Agency/AsyncAgencyComm.cpp
@@ -297,10 +297,10 @@ arangodb::AsyncAgencyComm::FutureResult agencyAsyncSend(AsyncAgencyCommManager&
 
           auto& req = result.request;
           auto& resp = result.response;
-          auto& body = *req;
 
           switch (result.error) {
-            case Error::NoError:
+            case Error::NoError: { 
+              TRI_ASSERT(req != nullptr);
 
               LOG_TOPIC("aac88", TRACE, Logger::AGENCYCOMM)
                   << "agencyAsyncSend [" << meta.requestId
@@ -325,6 +325,7 @@ arangodb::AsyncAgencyComm::FutureResult agencyAsyncSend(AsyncAgencyCommManager&
                 redirectOrError(man, endpoint, location);
 
                 // send again
+                auto& body = *req;
                 return ::agencyAsyncSend(man, std::move(meta), std::move(body).moveBuffer());
               }
 
@@ -333,50 +334,64 @@ arangodb::AsyncAgencyComm::FutureResult agencyAsyncSend(AsyncAgencyCommManager&
                 return futures::makeFuture(
                     AsyncAgencyCommResult{result.error, std::move(resp)});
               }
-
-              [[fallthrough]];
-              /* fallthrough */
-            case Error::ConnectionCanceled:
-              if (man.server().isStopping()) {
+            }
+            [[fallthrough]];
+
+            case Error::ConnectionCanceled: {
+              if (man.server().isStopping() || req == nullptr) {
                 return futures::makeFuture(
                   AsyncAgencyCommResult{result.error, std::move(resp)});
               }
-              [[fallthrough]];
+
+              TRI_ASSERT(req != nullptr);
+            }
+            [[fallthrough]];
+
             case Error::RequestTimeout:
             case Error::ConnectionClosed:
             case Error::ProtocolError:
             case Error::WriteError:
             case Error::ReadError:
-            case Error::CloseRequested:
-              // inquiry the request
+            case Error::CloseRequested: {
+              TRI_ASSERT(req != nullptr);
+
+              // inquire the request
               man.reportError(endpoint);
               // in case of a write transaction we have to do inquiry
               if (meta.isInquiryOnNoResponse()) {
+                TRI_ASSERT(req != nullptr);
+                auto& body = *req;
                 return ::agencyAsyncInquiry(man, std::move(meta),
                                             std::move(body).moveBuffer());
               } else if (!meta.isRetryOnNoResponse()) {
                 // return error
                 return futures::makeFuture(
                     AsyncAgencyCommResult{result.error, std::move(resp)});
               }
-              // otherwise just send again
-              [[fallthrough]];
+            }
+            // otherwise just send again
+            [[fallthrough]];
 
-            case Error::CouldNotConnect:
+            case Error::CouldNotConnect: {
               LOG_TOPIC("aac90", TRACE, Logger::AGENCYCOMM)
                   << "agencyAsyncSend [" << meta.requestId << "] retry request soon";
               // retry to send the request
               man.reportError(endpoint);
-              [[fallthrough]];
+            }
+            [[fallthrough]];
 
-            case Error::QueueCapacityExceeded:
-                return ::agencyAsyncSend(man, std::move(meta),
-                                         std::move(body).moveBuffer());  // retry always
+            case Error::QueueCapacityExceeded: {
+              TRI_ASSERT(req != nullptr);
+              auto& body = *req;
+              return ::agencyAsyncSend(man, std::move(meta),
+                                       std::move(body).moveBuffer());  // retry always
+            }
 
-            case Error::VstUnauthorized:
+            case Error::VstUnauthorized: {
               return futures::makeFuture(
                 AsyncAgencyCommResult{result.error,
                                       std::move(resp)});  // unrecoverable error
+            }
           }
 
           ADB_UNREACHABLE;

diff --git a/arangod/Network/NetworkFeature.cpp b/arangod/Network/NetworkFeature.cpp
@@ -289,14 +289,17 @@ void NetworkFeature::sendRequest(network::ConnectionPool& pool,
                                  std::string const& endpoint,
                                  std::unique_ptr<fuerte::Request>&& req,
                                  RequestCallback&& cb) {
+  TRI_ASSERT(req != nullptr);
   prepareRequest(pool, req);
   auto conn = pool.leaseConnection(endpoint);
   conn->sendRequest(std::move(req),
                     [this, &pool,
                      cb = std::move(cb)](fuerte::Error err,
                                          std::unique_ptr<fuerte::Request> req,
                                          std::unique_ptr<fuerte::Response> res) {
+                      TRI_ASSERT(req != nullptr);
                       finishRequest(pool, err, req, res);
+                      TRI_ASSERT(req != nullptr);
                       cb(err, std::move(req), std::move(res));
                     });
 }
@@ -320,6 +323,7 @@ void NetworkFeature::finishRequest(network::ConnectionPool const& pool, fuerte::
         std::chrono::duration_cast<std::chrono::milliseconds>(res->timestamp() -
                                                               req->timestamp());
     std::chrono::milliseconds timeout = req->timeout();
+    TRI_ASSERT(timeout.count() > 0);
     double percentage = std::clamp(100.0 * static_cast<double>(duration.count()) /
                                        static_cast<double>(timeout.count()),
                                    0.0, 100.0);