Skip to content

Fix potential bad memory access at polyroots function#1532

Open
MiraiHattori wants to merge 7 commits into
productionfrom
fixPolyRootsBadMemoryAccess
Open

Fix potential bad memory access at polyroots function#1532
MiraiHattori wants to merge 7 commits into
productionfrom
fixPolyRootsBadMemoryAccess

Conversation

@MiraiHattori

@MiraiHattori MiraiHattori commented Sep 24, 2025

Copy link
Copy Markdown
Collaborator
image

This PR fixes the attached compiler warning. Potentially if the input of those functions are per-joint linearly changing trajectories, then there may be segmentation fault.

Details

  • when the template parameter D=1, then this roots[1] is out of bound.
    roots[1] = complex<IKReal>(0.4,0.9); // any complex number not a root of unity works
  • caller with D=1. if the system accidentally hits it, segmentation fault will happen
    }
    else if( tempcoeffs[4] != 0 ) {
    polyroots<T, 1>(&tempcoeffs[4], rawroots, numroots);
    }

@MiraiHattori MiraiHattori self-assigned this Sep 24, 2025
Comment thread include/openrave/mathextra.h Outdated
@MiraiHattori

MiraiHattori commented Sep 24, 2025

Copy link
Copy Markdown
Collaborator Author

I added -Werror=array-bounds compiler options -> later reverted

@MiraiHattori

Copy link
Copy Markdown
Collaborator Author

bumped "minor" version (instead of patch version) / added changelog.

@MiraiHattori

Copy link
Copy Markdown
Collaborator Author

I added -Werror=array-bounds compiler options

compilation failed, taking a look.

@MiraiHattori
MiraiHattori marked this pull request as draft September 24, 2025 07:12
@cielavenir

Copy link
Copy Markdown
Collaborator

@MiraiHattori

Copy link
Copy Markdown
Collaborator Author

Could be https://qiita.com/cielavenir/items/3e52823a8193b9fb6989

With an older boost version, compiler did not catch the false positive out of bounds.

I think it's okay without -Werror=array-bounds in this PR, so reverted.

@MiraiHattori
MiraiHattori marked this pull request as ready for review September 24, 2025 07:42
@MiraiHattori

Copy link
Copy Markdown
Collaborator Author

@yoshikikanemoto could you review this MR?

@yoshikikanemoto

Copy link
Copy Markdown
Collaborator

@MiraiHattori why is minor version bump? looks like there are no ABI breaking changes? do you have unit tests that produce out-of-bounds access?

@MiraiHattori

MiraiHattori commented Sep 29, 2025

Copy link
Copy Markdown
Collaborator Author

@MiraiHattori why is minor version bump? looks like there are no ABI breaking changes? do you have unit tests that produce out-of-bounds access?

I reverted to patch version bump since there was no interface changes.

I don't have a unit test to reproduce the issue so far, let me make one.

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.

4 participants