From a75d0d138d2d56534a5c3f0f773d3567a2a63294 Mon Sep 17 00:00:00 2001 From: Keenon Werling Date: Tue, 24 Sep 2024 11:11:11 -0700 Subject: [PATCH 1/2] Attempting to add more useful error messages on out-of-bounds data accesses, to reduce the number of segfaults possible through the Python API --- dart/biomechanics/SubjectOnDisk.cpp | 171 ++++++++++++++++++ dart/biomechanics/SubjectOnDisk.hpp | 4 + dart/utils/AccelerationMinimizer.cpp | 7 + dart/utils/AccelerationTrackAndMinimize.cpp | 2 + .../biomechanics/SubjectOnDisk.cpp | 12 ++ 5 files changed, 196 insertions(+) diff --git a/dart/biomechanics/SubjectOnDisk.cpp b/dart/biomechanics/SubjectOnDisk.cpp index e76a1b944..48943fb9c 100644 --- a/dart/biomechanics/SubjectOnDisk.cpp +++ b/dart/biomechanics/SubjectOnDisk.cpp @@ -255,6 +255,15 @@ SubjectOnDisk::SubjectOnDisk(const std::string& path) fclose(file); } +SubjectOnDisk::SubjectOnDisk(std::shared_ptr header) +{ + mHeader = header; + mLoadedAllFrames = true; + mSensorFrameSize = 0; + mProcessingPassFrameSize = 0; + mDataSectionStart = 0; +} + /// This will write a B3D file to disk void SubjectOnDisk::writeB3D( const std::string& outputPath, std::shared_ptr header) @@ -610,6 +619,14 @@ std::vector SubjectOnDisk::readForcePlates(int trial) { std::vector plates; + if (trial >= getNumTrials() || trial < 0) + { + std::cout << "SubjectOnDisk::readForcePlates() called with invalid trial " + "number: " + << trial << std::endl; + return plates; + } + const int len = getTrialLength(trial); const int numPlates = getNumForcePlates(trial); for (int plate = 0; plate < numPlates; plate++) @@ -683,6 +700,13 @@ OpenSimFile SubjectOnDisk::readOpenSimFile( geometryFolder = common::Uri::createFromRelativeUri(mPath, "./Geometry/") .getFilesystemPath(); } + if (passNumberToLoad >= mHeader->mPasses.size() || passNumberToLoad < 0) + { + std::cout << "SubjectOnDisk::readOpenSimFile() called with invalid pass " + "number: " + << passNumberToLoad << std::endl; + return OpenSimFile(); + } tinyxml2::XMLDocument osimFile; osimFile.Parse(mHeader->mPasses[passNumberToLoad]->mOpenSimFileText.c_str()); @@ -704,6 +728,13 @@ OpenSimFile SubjectOnDisk::readOpenSimFile( /// it as a string std::string SubjectOnDisk::getOpensimFileText(int passNumberToLoad) { + if (passNumberToLoad >= mHeader->mPasses.size() || passNumberToLoad < 0) + { + std::cout << "SubjectOnDisk::getOpensimFileText() called with invalid pass " + "number: " + << passNumberToLoad << std::endl; + return ""; + } return mHeader->mPasses[passNumberToLoad]->mOpenSimFileText; } @@ -711,6 +742,20 @@ std::string SubjectOnDisk::getOpensimFileText(int passNumberToLoad) // frequency of that filter? s_t SubjectOnDisk::getLowpassCutoffFrequency(int trial, int processingPass) { + if (trial >= getNumTrials()) + { + std::cout << "SubjectOnDisk::getLowpassCutoffFrequency() called with " + "invalid trial number: " + << trial << std::endl; + return 0.0; + } + if (processingPass >= getTrialNumProcessingPasses(trial)) + { + std::cout << "SubjectOnDisk::getLowpassCutoffFrequency() called with " + "invalid processing pass number: " + << processingPass << std::endl; + return 0.0; + } return mHeader->mTrials[trial] ->mTrialPasses[processingPass] ->mLowpassCutoffFrequency; @@ -720,6 +765,20 @@ s_t SubjectOnDisk::getLowpassCutoffFrequency(int trial, int processingPass) // that (Butterworth) filter? int SubjectOnDisk::getLowpassFilterOrder(int trial, int processingPass) { + if (trial >= getNumTrials()) + { + std::cout << "SubjectOnDisk::getLowpassFilterOrder() called with invalid " + "trial number: " + << trial << std::endl; + return 0; + } + if (processingPass >= getTrialNumProcessingPasses(trial)) + { + std::cout << "SubjectOnDisk::getLowpassFilterOrder() called with invalid " + "processing pass number: " + << processingPass << std::endl; + return 0; + } return mHeader->mTrials[trial] ->mTrialPasses[processingPass] ->mLowpassFilterOrder; @@ -730,6 +789,20 @@ int SubjectOnDisk::getLowpassFilterOrder(int trial, int processingPass) std::vector SubjectOnDisk::getForceplateCutoffs( int trial, int processingPass) { + if (trial >= getNumTrials()) + { + std::cout << "SubjectOnDisk::getForceplateCutoffs() called with invalid " + "trial number: " + << trial << std::endl; + return std::vector(); + } + if (processingPass >= getTrialNumProcessingPasses(trial)) + { + std::cout << "SubjectOnDisk::getForceplateCutoffs() called with invalid " + "processing pass number: " + << processingPass << std::endl; + return std::vector(); + } return mHeader->mTrials[trial] ->mTrialPasses[processingPass] ->mForcePlateCutoffs; @@ -1329,6 +1402,9 @@ int SubjectOnDisk::getTrialLength(int trial) { if (trial < 0 || trial >= mHeader->mTrials.size()) { + std::cout << "SubjectOnDisk::getTrialLength() called with invalid trial " + "number: " + << trial << std::endl; return 0; } return mHeader->mTrials[trial]->mLength; @@ -1338,6 +1414,13 @@ int SubjectOnDisk::getTrialLength(int trial) /// split into multiple pieces std::string SubjectOnDisk::getTrialOriginalName(int trial) { + if (trial < 0 || trial >= mHeader->mTrials.size()) + { + std::cout << "SubjectOnDisk::getTrialOriginalName() called with invalid " + "trial number: " + << trial << std::endl; + return ""; + } return mHeader->mTrials[trial]->mOriginalTrialName; } @@ -1345,6 +1428,13 @@ std::string SubjectOnDisk::getTrialOriginalName(int trial) /// splitting an original trial into multiple pieces int SubjectOnDisk::getTrialSplitIndex(int trial) { + if (trial < 0 || trial >= mHeader->mTrials.size()) + { + std::cout << "SubjectOnDisk::getTrialSplitIndex() called with invalid " + "trial number: " + << trial << std::endl; + return -1; + } return mHeader->mTrials[trial]->mSplitIndex; } @@ -1353,6 +1443,9 @@ int SubjectOnDisk::getTrialNumProcessingPasses(int trial) { if (trial < 0 || trial >= mHeader->mTrials.size()) { + std::cout << "SubjectOnDisk::getTrialNumProcessingPasses() called with " + "invalid trial number: " + << trial << std::endl; return 0; } return mHeader->mTrials[trial]->mTrialPasses.size(); @@ -1363,6 +1456,9 @@ s_t SubjectOnDisk::getTrialTimestep(int trial) { if (trial < 0 || trial >= mHeader->mTrials.size()) { + std::cout << "SubjectOnDisk::getTrialTimestep() called with invalid trial " + "number: " + << trial << std::endl; return 0.01; } return mHeader->mTrials[trial]->mTimestep; @@ -1386,6 +1482,9 @@ std::vector SubjectOnDisk::getMissingGRF(int trial) { if (trial < 0 || trial >= mHeader->mTrials.size()) { + std::cout << "Requested getMissingGRF() for trial " << trial + << ", which is out of bounds. Returning empty vector." + << std::endl; return std::vector(); } return mHeader->mTrials[trial]->mMissingGRFReason; @@ -1402,6 +1501,15 @@ ProcessingPassType SubjectOnDisk::getProcessingPassType(int processingPass) { processingPass = mHeader->mPasses.size() - 1; } + if (processingPass < 0 || processingPass >= mHeader->mPasses.size()) + { + std::cout << "Requested getProcessingPassType() for processing pass " + << processingPass + << ", which is out of bounds. Returning " + "ProcessingPassType::kinematics." + << std::endl; + return ProcessingPassType::kinematics; + } return mHeader->mPasses[processingPass]->mType; } @@ -1410,6 +1518,9 @@ std::vector SubjectOnDisk::getDofPositionsObserved( { if (trial < 0 || trial >= mHeader->mTrials.size()) { + std::cout << "Requested getDofPositionsObserved() for trial " << trial + << ", which is out of bounds. Returning empty vector." + << std::endl; return std::vector(); } if (processingPass == -1) @@ -1426,6 +1537,9 @@ std::vector SubjectOnDisk::getDofVelocitiesFiniteDifferenced( { if (trial < 0 || trial >= mHeader->mTrials.size()) { + std::cout << "Requested getDofVelocitiesFiniteDifferenced() for trial " + << trial << ", which is out of bounds. Returning empty vector." + << std::endl; return std::vector(); } if (processingPass == -1) @@ -1442,6 +1556,9 @@ std::vector SubjectOnDisk::getDofAccelerationsFiniteDifferenced( { if (trial < 0 || trial >= mHeader->mTrials.size()) { + std::cout << "Requested getDofAccelerationsFiniteDifferenced() for trial " + << trial << ", which is out of bounds. Returning empty vector." + << std::endl; return std::vector(); } if (processingPass == -1) @@ -1458,6 +1575,9 @@ std::vector SubjectOnDisk::getTrialLinearResidualNorms( { if (trial < 0 || trial >= mHeader->mTrials.size()) { + std::cout << "Requested getTrialLinearResidualNorms() for trial " << trial + << ", which is out of bounds. Returning empty vector." + << std::endl; return std::vector(); } if (processingPass == -1) @@ -1472,6 +1592,9 @@ std::vector SubjectOnDisk::getTrialAngularResidualNorms( { if (trial < 0 || trial >= mHeader->mTrials.size()) { + std::cout << "Requested getTrialAngularResidualNorms() for trial " << trial + << ", which is out of bounds. Returning empty vector." + << std::endl; return std::vector(); } if (processingPass == -1) @@ -1516,12 +1639,26 @@ std::vector SubjectOnDisk::getTrialMarkerMaxs( { if (trial < 0 || trial >= mHeader->mTrials.size()) { + std::cout << "Requested getTrialMarkerMaxs() for trial " << trial + << ", which is out of bounds. Returning empty vector." + << std::endl; return std::vector(); } if (processingPass == -1) { processingPass = mHeader->mTrials[trial]->mTrialPasses.size() - 1; } + if (processingPass < 0 + || processingPass >= mHeader->mTrials[trial]->mTrialPasses.size()) + { + std::cout + << "Requested getTrialMarkerMaxs() for trial " << trial + << " and processing pass " << processingPass + << ", which is out of bounds on the existing processing passes (len=" + << mHeader->mTrials[trial]->mTrialPasses.size() + << "). Returning empty vector." << std::endl; + return std::vector(); + } return mHeader->mTrials[trial]->mTrialPasses[processingPass]->mMarkerMax; } @@ -1532,12 +1669,26 @@ std::vector SubjectOnDisk::getTrialMaxJointVelocity( { if (trial < 0 || trial >= mHeader->mTrials.size()) { + std::cout << "Requested getTrialMaxJointVelocity() for trial " << trial + << ", which is out of bounds. Returning empty vector." + << std::endl; return std::vector(); } if (processingPass == -1) { processingPass = mHeader->mTrials[trial]->mTrialPasses.size() - 1; } + if (processingPass < 0 + || processingPass >= mHeader->mTrials[trial]->mTrialPasses.size()) + { + std::cout + << "Requested getTrialMaxJointVelocity() for trial " << trial + << " and processing pass " << processingPass + << ", which is out of bounds on the existing processing passes (len=" + << mHeader->mTrials[trial]->mTrialPasses.size() + << "). Returning empty vector." << std::endl; + return std::vector(); + } return mHeader->mTrials[trial] ->mTrialPasses[processingPass] ->mJointsMaxVelocity; @@ -1581,6 +1732,9 @@ std::string SubjectOnDisk::getTrialName(int trial) { if (trial < 0 || trial >= mHeader->mTrials.size()) { + std::cout << "Requested getTrialName() for trial " << trial + << ", which is out of bounds. Returning empty string." + << std::endl; return ""; } return mHeader->mTrials[trial]->mName; @@ -1641,10 +1795,17 @@ std::vector SubjectOnDisk::getForcePlateCorners( { if (trial < 0 || trial >= mHeader->mTrials.size()) { + std::cout << "Requested getForcePlateCorners() for trial " << trial + << ", which is out of bounds. Returning empty vector." + << std::endl; return std::vector(); } if (forcePlate < 0 || forcePlate >= mHeader->mTrials[trial]->mNumForcePlates) { + std::cout << "Requested getForcePlateCorners() for force plate " + << forcePlate + << ", which is out of bounds. Returning empty vector." + << std::endl; return std::vector(); } return mHeader->mTrials[trial]->mForcePlateCorners[forcePlate]; @@ -2916,6 +3077,16 @@ s_t SubjectOnDiskTrial::getTimestep() return mTimestep; } +void SubjectOnDiskTrial::setTrialLength(int length) +{ + mLength = length; +} + +int SubjectOnDiskTrial::getTrialLength() +{ + return mLength; +} + void SubjectOnDiskTrial::setTrialTags(std::vector trialTags) { mTrialTags = trialTags; diff --git a/dart/biomechanics/SubjectOnDisk.hpp b/dart/biomechanics/SubjectOnDisk.hpp index 33c5d7499..af1f63c77 100644 --- a/dart/biomechanics/SubjectOnDisk.hpp +++ b/dart/biomechanics/SubjectOnDisk.hpp @@ -325,6 +325,8 @@ class SubjectOnDiskTrial void setName(const std::string& name); void setTimestep(s_t timestep); s_t getTimestep(); + void setTrialLength(int length); + int getTrialLength(); void setTrialTags(std::vector trialTags); std::string getOriginalTrialName(); void setOriginalTrialName(const std::string& name); @@ -512,6 +514,8 @@ class SubjectOnDisk public: SubjectOnDisk(const std::string& path); + SubjectOnDisk(std::shared_ptr header); + /// This will write a B3D file to disk static void writeB3D( const std::string& path, std::shared_ptr header); diff --git a/dart/utils/AccelerationMinimizer.cpp b/dart/utils/AccelerationMinimizer.cpp index 57f08d055..03e375467 100644 --- a/dart/utils/AccelerationMinimizer.cpp +++ b/dart/utils/AccelerationMinimizer.cpp @@ -40,6 +40,13 @@ AccelerationMinimizer::AccelerationMinimizer( mDebugIterationBackoff(false), mConvergenceTolerance(1e-10) { + if (mTimesteps < 3) + { + std::cout << "AccelerationMinimizer requires at least 3 timesteps to work." + << std::endl; + throw std::runtime_error( + "AccelerationMinimizer requires at least 3 timesteps to work."); + } Eigen::Vector3s stamp; stamp << -1, 2, -1; stamp *= mSmoothingWeight; diff --git a/dart/utils/AccelerationTrackAndMinimize.cpp b/dart/utils/AccelerationTrackAndMinimize.cpp index f2fc408af..083efc51d 100644 --- a/dart/utils/AccelerationTrackAndMinimize.cpp +++ b/dart/utils/AccelerationTrackAndMinimize.cpp @@ -35,6 +35,8 @@ AccelerationTrackAndMinimize::AccelerationTrackAndMinimize( { std::cout << "AccelerationTrackAndMinimize requires at least 3 timesteps" << std::endl; + throw std::runtime_error( + "AccelerationMinimizer requires at least 3 timesteps to work."); return; } if (mTrackAccelerationAtTimesteps.size() != mTimesteps) diff --git a/python/_nimblephysics/biomechanics/SubjectOnDisk.cpp b/python/_nimblephysics/biomechanics/SubjectOnDisk.cpp index 691f158ff..534117806 100644 --- a/python/_nimblephysics/biomechanics/SubjectOnDisk.cpp +++ b/python/_nimblephysics/biomechanics/SubjectOnDisk.cpp @@ -817,6 +817,13 @@ Note that these are specified in the local body frame, acting on the body at its .def( "getTimestep", &dart::biomechanics::SubjectOnDiskTrial::getTimestep) + .def( + "setTrialLength", + &dart::biomechanics::SubjectOnDiskTrial::setTrialLength, + ::py::arg("length")) + .def( + "getTrialLength", + &dart::biomechanics::SubjectOnDiskTrial::getTrialLength) .def( "setTrialTags", &dart::biomechanics::SubjectOnDiskTrial::setTrialTags, @@ -1004,6 +1011,11 @@ Note that these are specified in the local body frame, acting on the body at its m, "SubjectOnDisk") // SubjectOnDisk(const std::string& path); .def(::py::init(), ::py::arg("path")) + // SubjectOnDisk(const std::string& path); + .def( + ::py::init< + std::shared_ptr>(), + ::py::arg("header")) // /// This will write a B3D file to disk // static void writeB3D(const std::string& path, // SubjectOnDiskHeader& header); From bd66cec6bd3ae0df66899f4d90eebb7d38f6bf41 Mon Sep 17 00:00:00 2001 From: Keenon Werling Date: Tue, 24 Sep 2024 14:20:29 -0700 Subject: [PATCH 2/2] Adding negative trial checks --- dart/biomechanics/SubjectOnDisk.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dart/biomechanics/SubjectOnDisk.cpp b/dart/biomechanics/SubjectOnDisk.cpp index 48943fb9c..d87bf813a 100644 --- a/dart/biomechanics/SubjectOnDisk.cpp +++ b/dart/biomechanics/SubjectOnDisk.cpp @@ -742,7 +742,7 @@ std::string SubjectOnDisk::getOpensimFileText(int passNumberToLoad) // frequency of that filter? s_t SubjectOnDisk::getLowpassCutoffFrequency(int trial, int processingPass) { - if (trial >= getNumTrials()) + if (trial >= getNumTrials() || trial < 0) { std::cout << "SubjectOnDisk::getLowpassCutoffFrequency() called with " "invalid trial number: " @@ -765,7 +765,7 @@ s_t SubjectOnDisk::getLowpassCutoffFrequency(int trial, int processingPass) // that (Butterworth) filter? int SubjectOnDisk::getLowpassFilterOrder(int trial, int processingPass) { - if (trial >= getNumTrials()) + if (trial >= getNumTrials() || trial < 0) { std::cout << "SubjectOnDisk::getLowpassFilterOrder() called with invalid " "trial number: " @@ -789,7 +789,7 @@ int SubjectOnDisk::getLowpassFilterOrder(int trial, int processingPass) std::vector SubjectOnDisk::getForceplateCutoffs( int trial, int processingPass) { - if (trial >= getNumTrials()) + if (trial >= getNumTrials() || trial < 0) { std::cout << "SubjectOnDisk::getForceplateCutoffs() called with invalid " "trial number: "