Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add bounds-checking to ObjectProperty and appropriate tests against r… #3554

Merged
merged 3 commits into from
Sep 15, 2023

Conversation

adamkewley
Copy link
Contributor

@adamkewley adamkewley commented Sep 14, 2023

Fixes issue #3347

Brief summary of changes

  • Used .at to add bounds-checking to OpenSim::ObjectProperty, so that mis-indexing into one (e.g. by calling get_property() or similar) causes a runtime exception rather than a segfault
  • Added tests to testComponentInterface.cpp and testConstraints.cpp to ensure the behavior doesn't regress

Testing I've completed

  • I added the test to testConstraints.cpp
  • It did not segfault, because a previous fix hotfixed it (here: 4fa4493)
  • I then added tests to testComponentInterface.cpp
  • The SimpleProperty one did not segfault, because it was fixed in Patched segfault from reading invalid osim data (#3409) #3411
  • The ObjectProperty one did segfault
  • Then I added the bounds checking to Object.h (i.e. swap index accessors for .at)
  • Then the test passed

Looking for feedback on...

CHANGELOG.md

  • updated.

This change is Reviewable

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

(If it fails again, the Java Moco sliding mass test can be ignored)

@adamkewley adamkewley merged commit 2043469 into main Sep 15, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants