verify that data is valid before generating journeys

the two while loops in forward and reverse journey can create memory leak (infinite push into a vector) when invalid data is found (like reverse travel times or incorrectly rounded/floored/ceiled travel time seconds that was imported as float, see #141, needs testing though to close issue
chairemobilite · May 27, 2022 · 18946c7 · 18946c7
1 parent 6e07030
commit 18946c7
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 17 deletions.
diff --git a/connection_scan_algorithm/src/forward_journey.cpp b/connection_scan_algorithm/src/forward_journey.cpp
@@ -96,16 +96,16 @@ namespace TrRouting
         modeShortnames.clear();
         inVehicleTravelTimesSeconds.clear();
 
-        totalInVehicleTime       =  0; transferArrivalTime    = -1; firstDepartureTime     = -1;
-        totalWalkingTime         =  0; transferReadyTime      = -1; minimizedDepartureTime = -1;
-        totalWaitingTime         =  0; departureTime          = -1; numberOfTransfers      = -1;
-        totalTransferWalkingTime =  0; arrivalTime            = -1; boardingSequence       = -1;
-        totalTransferWaitingTime =  0; inVehicleTime          = -1; unboardingSequence     = -1;
+        totalInVehicleTime       =  0; transferArrivalTime    = -1; firstDepartureTime      = -1;
+        totalWalkingTime         =  0; transferReadyTime      = -1; minimizedDepartureTime  = -1;
+        totalWaitingTime         =  0; departureTime          = -1; numberOfTransfers       = -1;
+        totalTransferWalkingTime =  0; arrivalTime            = -1; boardingSequence        = -1;
+        totalTransferWaitingTime =  0; inVehicleTime          = -1; unboardingSequence      = -1;
         totalDistance            =  0; distance               = -1;
-        inVehicleDistance        =  0; totalInVehicleDistance =  0; totalWalkingDistance   =  0;
-        totalTransferDistance    =  0; accessDistance         =  0; egressDistance         =  0;
-        journeyStepTravelTime    = -1; accessWalkingTime      = -1; bestAccessNodeIndex    = -1;
-        transferTime             = -1; egressWalkingTime      = -1;
+        inVehicleDistance        =  0; totalInVehicleDistance =  0; totalWalkingDistance    =  0;
+        totalTransferDistance    =  0; accessDistance         =  0; egressDistance          =  0;
+        journeyStepTravelTime    = -1; accessWalkingTime      = -1; bestAccessNodeIndex     = -1;
+        transferTime             = -1; egressWalkingTime      = -1; bestAccessNodePrevIndex = -1;
         waitingTime              = -1; accessWaitingTime      = -1;
 
         //std::cerr << nodes[resultingNodeIndex].get()->name << " : " << std::get<0>(forwardJourneysSteps[resultingNodeIndex]) << std::endl;
@@ -117,11 +117,19 @@ namespace TrRouting
           continue;
         }
 
+
         while ((std::get<journeyStepIndexes::FINAL_ENTER_CONNECTION>(resultingNodeJourneyStep) != -1 && std::get<journeyStepIndexes::FINAL_EXIT_CONNECTION>(resultingNodeJourneyStep) != -1))
         {
           journey.push_front(resultingNodeJourneyStep);
           bestAccessNodeIndex = std::get<connectionIndexes::NODE_DEP>(*forwardConnections[std::get<journeyStepIndexes::FINAL_ENTER_CONNECTION>(resultingNodeJourneyStep)].get());
+          if (bestAccessNodePrevIndex != -1 && bestAccessNodeIndex != -1 && bestAccessNodePrevIndex == bestAccessNodeIndex) {
+            // this should not happen, it means that there is an ambiguity in the data, which makes this loop runs forever and create a memory leak filling journey indefinitely.
+            // TODO: add tests
+            std::cerr << "Invalid data in path uuid: " << paths[trips[std::get<journeyStepIndexes::FINAL_TRIP>(resultingNodeJourneyStep)].get()->pathIdx].get()->uuid << std::endl;
+            throw InvalidDataException(NoRoutingReason::INVALID_DATA);
+          }
           resultingNodeJourneyStep = forwardJourneysSteps[bestAccessNodeIndex];
+          bestAccessNodePrevIndex = bestAccessNodeIndex;
         }
 
         if (!params.returnAllNodesResult)

diff --git a/connection_scan_algorithm/src/reverse_journey.cpp b/connection_scan_algorithm/src/reverse_journey.cpp
@@ -94,14 +94,14 @@ namespace TrRouting
         modeShortnames.clear();
         inVehicleTravelTimesSeconds.clear();
 
-        totalInVehicleTime          =  0; transferArrivalTime    = -1; firstDepartureTime   = -1;
-        totalWalkingTime            =  0; transferReadyTime      = -1; numberOfTransfers    = -1;
-        totalWaitingTime            =  0; departureTime          = -1; boardingSequence     = -1;
-        totalTransferWalkingTime    =  0; arrivalTime            = -1; unboardingSequence   = -1;
-        totalTransferWaitingTime    =  0; inVehicleTime          = -1; bestEgressNodeIndex  = -1;
-        totalDistance               =  0; distance               = -1;
-        inVehicleDistance           =  0; totalInVehicleDistance =  0; totalWalkingDistance =  0;
-        totalTransferDistance       =  0; accessDistance         =  0; egressDistance       =  0;
+        totalInVehicleTime          =  0; transferArrivalTime    = -1; firstDepartureTime      = -1;
+        totalWalkingTime            =  0; transferReadyTime      = -1; numberOfTransfers       = -1;
+        totalWaitingTime            =  0; departureTime          = -1; boardingSequence        = -1;
+        totalTransferWalkingTime    =  0; arrivalTime            = -1; unboardingSequence      = -1;
+        totalTransferWaitingTime    =  0; inVehicleTime          = -1; bestEgressNodeIndex     = -1;
+        totalDistance               =  0; distance               = -1; bestEgressNodePrevIndex = -1;
+        inVehicleDistance           =  0; totalInVehicleDistance =  0; totalWalkingDistance    =  0;
+        totalTransferDistance       =  0; accessDistance         =  0; egressDistance          =  0;
         journeyStepTravelTime       = -1; accessWalkingTime      = -1;
         transferTime                = -1; egressWalkingTime      = -1;
         waitingTime                 = -1; accessWaitingTime      = -1;
@@ -124,7 +124,14 @@ namespace TrRouting
           }
           journey.push_back(resultingNodeJourneyStep);
           bestEgressNodeIndex      = std::get<connectionIndexes::NODE_ARR>(*reverseConnections[std::get<journeyStepIndexes::FINAL_EXIT_CONNECTION>(resultingNodeJourneyStep)].get());
+          if (bestEgressNodePrevIndex != -1 && bestEgressNodeIndex != -1 && bestEgressNodePrevIndex == bestEgressNodeIndex) {
+            // this should not happen, it means that there is an ambiguity in the data, which makes this loop runs forever and create a memory leak filling journey indefinitely.
+            // TODO: add tests
+            std::cerr << "Invalid data in path uuid: " << paths[trips[std::get<journeyStepIndexes::FINAL_TRIP>(resultingNodeJourneyStep)].get()->pathIdx].get()->uuid << std::endl;
+            throw InvalidDataException(NoRoutingReason::INVALID_DATA);
+          }
           resultingNodeJourneyStep = reverseJourneysSteps[bestEgressNodeIndex];
+          bestEgressNodePrevIndex = bestEgressNodeIndex;
         }
 
         if (!params.returnAllNodesResult)

diff --git a/include/routing_result.hpp b/include/routing_result.hpp
@@ -321,6 +321,8 @@ namespace TrRouting
     NO_SERVICE_FROM_ORIGIN,
     // There is no service to destination with the query parameters
     NO_SERVICE_TO_DESTINATION
+    // Invalid data found, which could create memory leaks or incorrect results
+    INVALID_DATA
   };
 
   /**
@@ -337,6 +339,20 @@ namespace TrRouting
       NoRoutingReason reason;
   };
 
+  /**
+   * @brief Exception class thrown when getting invalid data
+   *
+   */
+  class InvalidDataException : public std::exception
+  {
+    public:
+      InvalidDataException(NoRoutingReason reason_) : std::exception(), reason(reason_) {};
+      NoRoutingReason getReason() const { return reason; };
+
+    private:
+      NoRoutingReason reason;
+  };
+
 }
 
 #endif // TR_ROUTING_RESULT