Skip to content

Commit

Permalink
verify that data is valid before generating journeys
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@tahini @kaligrafy
tahini authored and kaligrafy committed May 27, 2022
1 parent 4edacda commit 1fb8564
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 30 deletions.
29 changes: 19 additions & 10 deletions connection_scan_algorithm/src/forward_journey.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "agency.hpp"
#include "routing_result.hpp"
#include "toolbox.hpp" //MAX_INT
#include <iostream>

namespace TrRouting
{
Expand Down Expand Up @@ -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)
Expand All @@ -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;
Expand All @@ -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)
Expand Down
11 changes: 8 additions & 3 deletions connection_scan_algorithm/src/parameters.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ namespace TrRouting
odTripsActivities.clear();
odTripsModes.clear();

accessNodesIdx.clear();
accessNodeTravelTimesSeconds.clear();
accessNodeDistancesMeters.clear();
egressNodesIdx.clear();
egressNodeTravelTimesSeconds.clear();
egressNodeDistancesMeters.clear();

onlyDataSourceIdx = -1;

calculationName = "trRouting";
Expand Down Expand Up @@ -513,7 +520,6 @@ namespace TrRouting
continue;
}

/*
// not sure we want to keep this: or supply node indexes instead, to limit request size?
else if (parameterWithValueVector[0] == "access_node_uuids")
{
Expand All @@ -529,7 +535,7 @@ namespace TrRouting
}
continue;
}
else if (parameterWithValueVector[0] == "transfer_node_uuids")
else if (parameterWithValueVector[0] == "egress_node_uuids")
{
boost::split(egressNodeUuidsVector, parameterWithValueVector[1], boost::is_any_of(","));
boost::uuids::uuid egressNodeUuid;
Expand Down Expand Up @@ -563,7 +569,6 @@ namespace TrRouting
egressNodeTravelTimesSeconds = egressNodeTravelTimesSeconds;
continue;
}
*/

else if (parameterWithValueVector[0] == "date")
{
Expand Down
40 changes: 24 additions & 16 deletions connection_scan_algorithm/src/reverse_journey.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "path.hpp"
#include "agency.hpp"
#include "routing_result.hpp"
#include <iostream>

namespace TrRouting
{
Expand Down Expand Up @@ -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};
Expand All @@ -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;
Expand All @@ -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)
Expand Down
18 changes: 17 additions & 1 deletion include/routing_result.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
};

/**
Expand All @@ -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

0 comments on commit 1fb8564

Please sign in to comment.