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 · 2235d48 · 2235d48
1 parent 6e07030
commit 2235d48
Show file tree

Hide file tree

Showing 8 changed files with 266 additions and 168 deletions.
diff --git a/confdefs.h b/confdefs.h
@@ -0,0 +1,24 @@
+/* confdefs.h */
+#define PACKAGE_NAME "trRouting"
+#define PACKAGE_TARNAME "trrouting"
+#define PACKAGE_VERSION "1.0"
+#define PACKAGE_STRING "trRouting 1.0"
+#define PACKAGE_BUGREPORT "https://github.com/kaligrafy/trRouting"
+#define PACKAGE_URL ""
+#define PACKAGE "trrouting"
+#define VERSION "1.0"
+#define HAVE_STDIO_H 1
+#define HAVE_STDLIB_H 1
+#define HAVE_STRING_H 1
+#define HAVE_INTTYPES_H 1
+#define HAVE_STDINT_H 1
+#define HAVE_STRINGS_H 1
+#define HAVE_SYS_STAT_H 1
+#define HAVE_SYS_TYPES_H 1
+#define HAVE_UNISTD_H 1
+#define STDC_HEADERS 1
+#define HAVE_DLFCN_H 1
+#define LT_OBJDIR ".libs/"
+#define HAVE_CXX17 1
+#define HAVE_BOOST /**/
+#define HAVE_BOOST_SYSTEM /**/
diff --git a/conftest.cpp b/conftest.cpp
@@ -0,0 +1,34 @@
+/* confdefs.h */
+#define PACKAGE_NAME "trRouting"
+#define PACKAGE_TARNAME "trrouting"
+#define PACKAGE_VERSION "1.0"
+#define PACKAGE_STRING "trRouting 1.0"
+#define PACKAGE_BUGREPORT "https://github.com/kaligrafy/trRouting"
+#define PACKAGE_URL ""
+#define PACKAGE "trrouting"
+#define VERSION "1.0"
+#define HAVE_STDIO_H 1
+#define HAVE_STDLIB_H 1
+#define HAVE_STRING_H 1
+#define HAVE_INTTYPES_H 1
+#define HAVE_STDINT_H 1
+#define HAVE_STRINGS_H 1
+#define HAVE_SYS_STAT_H 1
+#define HAVE_SYS_TYPES_H 1
+#define HAVE_UNISTD_H 1
+#define STDC_HEADERS 1
+#define HAVE_DLFCN_H 1
+#define LT_OBJDIR ".libs/"
+#define HAVE_CXX17 1
+#define HAVE_BOOST /**/
+#define HAVE_BOOST_SYSTEM /**/
+/* end confdefs.h.  */
+#include <boost/regex.hpp>
+
+int
+main (void)
+{
+boost::regex r(); return 0;
+  ;
+  return 0;
+}
diff --git a/conftest.err b/conftest.err
diff --git a/connection_scan_algorithm/src/forward_journey.cpp b/connection_scan_algorithm/src/forward_journey.cpp
@@ -10,6 +10,7 @@
 #include "agency.hpp"
 #include "routing_result.hpp"
 #include "toolbox.hpp" //MAX_INT
+#include <iostream>
 
 namespace TrRouting
 {
@@ -79,7 +80,7 @@ namespace TrRouting
       int inVehicleDistance        {-1}; int totalInVehicleDistance { 0}; int totalWalkingDistance   { 0};
       int totalTransferDistance    {-1}; int accessDistance         { 0}; int egressDistance         { 0};
       int journeyStepTravelTime    {-1}; int accessWalkingTime      {-1}; int bestAccessNodeIndex    {-1};
-      int transferTime             {-1}; int egressWalkingTime      {-1};
+      int transferTime             {-1}; int egressWalkingTime      {-1}; int bestAccessNodePrevIndex{-1};
       int waitingTime              {-1}; int accessWaitingTime      {-1};
 
       for (auto & resultingNodeIndex : resultingNodes)
@@ -96,16 +97,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 +118,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
@@ -8,6 +8,7 @@
 #include "path.hpp"
 #include "agency.hpp"
 #include "routing_result.hpp"
+#include <iostream>
 
 namespace TrRouting
 {
@@ -68,14 +69,14 @@ namespace TrRouting
       Path *   journeyStepPath;
       Agency * journeyStepAgency;
 
-      int totalInVehicleTime       { 0}; int transferArrivalTime    {-1}; int firstDepartureTime   {-1};
-      int totalWalkingTime         { 0}; int transferReadyTime      {-1}; int numberOfTransfers    {-1};
-      int totalWaitingTime         { 0}; int departureTime          {-1}; int boardingSequence     {-1};
-      int totalTransferWalkingTime { 0}; int arrivalTime            {-1}; int unboardingSequence   {-1};
-      int totalTransferWaitingTime { 0}; int inVehicleTime          {-1}; int bestEgressNodeIndex  {-1};
-      int totalDistance            { 0}; int distance               {-1};
-      int inVehicleDistance        {-1}; int totalInVehicleDistance { 0}; int totalWalkingDistance { 0};
-      int totalTransferDistance    {-1}; int accessDistance         { 0}; int egressDistance       { 0};
+      int totalInVehicleTime       { 0}; int transferArrivalTime    {-1}; int firstDepartureTime     {-1};
+      int totalWalkingTime         { 0}; int transferReadyTime      {-1}; int numberOfTransfers      {-1};
+      int totalWaitingTime         { 0}; int departureTime          {-1}; int boardingSequence       {-1};
+      int totalTransferWalkingTime { 0}; int arrivalTime            {-1}; int unboardingSequence     {-1};
+      int totalTransferWaitingTime { 0}; int inVehicleTime          {-1}; int bestEgressNodeIndex    {-1};
+      int totalDistance            { 0}; int distance               {-1}; int bestEgressNodePrevIndex{-1};
+      int inVehicleDistance        {-1}; int totalInVehicleDistance { 0}; int totalWalkingDistance   { 0};
+      int totalTransferDistance    {-1}; int accessDistance         { 0}; int egressDistance         { 0};
       int journeyStepTravelTime    {-1}; int accessWalkingTime      {-1};
       int transferTime             {-1}; int egressWalkingTime      {-1};
       int waitingTime              {-1}; int accessWaitingTime      {-1};
@@ -94,14 +95,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 +125,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
@@ -320,7 +320,9 @@ namespace TrRouting
     // There is no service from origin with the query parameters
     NO_SERVICE_FROM_ORIGIN,
     // There is no service to destination with the query parameters
-    NO_SERVICE_TO_DESTINATION
+    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