Skip to content

Fix memory allocation bug in ForwardKinematics and RelativeTransform blocks #243

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

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

LoreMoretti
Copy link
Contributor

@LoreMoretti LoreMoretti commented Feb 6, 2024

The aforementioned blocks contain code like:

    Map<const Matrix4diDynTree> world_H_frame_RowMajor =
        toEigen(world_H_frame.asHomogeneousTransform());

that actually pass the pointer to a temporary object to the Eigen::Map, that actually then uses it without anything that ensures that the memory is still valid. We fix this by actually assigning the return value of world_H_frame.asHomogeneousTransform() to a proper local variable, that remains in scope as long as the corresponding Eigen::Map does.

@traversaro traversaro changed the title Fix memory allocation bug Fix memory allocation bug in ForwardKinematics and RelativeTransform block Feb 6, 2024
@isorrentino
Copy link

cc @DanielePucci

@traversaro
Copy link
Member

@LoreMoretti I added some details to the PR body, that is a good thing for future people.

Interestingly the bug has been present since the code was written, but it appeared more systematically on @LoreMoretti's setup in which he used a recent gcc compiler installed by conda (gcc 12.3.0, I guess), as opposed to other users that I guess compiled WB-Toolbox with gcc installed by apt (gcc 11.4.0 on Ubuntu 22.04). However, it is possible that some past non-determinism that we experienced with WB-Toolbox was related to this.

@traversaro traversaro changed the title Fix memory allocation bug in ForwardKinematics and RelativeTransform block Fix memory allocation bug in ForwardKinematics and RelativeTransform blocks Feb 6, 2024
@traversaro
Copy link
Member

Thanks @isorrentino @LoreMoretti! I added a few commits to be ready to release v5.6.1 right after merging this PR, so that we can release as soon as possible this fundamental fix. Please double check this, and if everything seems right just comment.

@isorrentino
Copy link

Everything seems ok to me. Thanks @traversaro!

@LoreMoretti
Copy link
Contributor Author

Everything seems ok to me too. Thanks @isorrentino and @traversaro

@traversaro
Copy link
Member

Just a side comment: something that we never did was to write a dummy blockfactory engine (see https://github.com/robotology/blockfactory/blob/22f62816df31a39b972ac135d186d4150458c3b1/doc/mkdocs/data/create_new_library.md?plain=1#L49) to run the blocks without MATLAB, if we did that we could have run the blocks under valgrind and we would have probably detected this problem automatically.

@traversaro traversaro merged commit 19b7408 into robotology:master Feb 6, 2024
@isorrentino
Copy link

Just a side comment: something that we never did was to write a dummy blockfactory engine (see https://github.com/robotology/blockfactory/blob/22f62816df31a39b972ac135d186d4150458c3b1/doc/mkdocs/data/create_new_library.md?plain=1#L49) to run the blocks without MATLAB, if we did that we could have run the blocks under valgrind and we would have probably detected this problem automatically.

I like your idea, having a way to run blocks under valgrind would be great.

@DanielePucci
Copy link
Contributor

Super! I'm very happy that at the end we spotted a (very old?) bug!

@LoreMoretti LoreMoretti deleted the bug_fix branch February 6, 2024 16:33
traversaro

This comment was marked as outdated.

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