Skip to content

Prevent segfault in PyKDL when asking for nonexisting segment of chain #228

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Prevent segfault in PyKDL when asking for nonexisting segment of chain #228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3663023
(PyKDL) Prevent segfault when asking for nonexisting segment in chain
DriesDM Feb 20, 2020
6eb2246
Merge branch 'master' into fix/prevent-pykdl-segfault
DriesDM Mar 13, 2020
7fdc4e4
Merge remote-tracking branch 'origin/master' into fix/prevent-pykdl-s…
MatthijsBurgh May 19, 2020
0816b90
Merge remote-tracking branch 'origin/master' into fix/prevent-pykdl-s…
MatthijsBurgh May 19, 2020
4c9b0af
Merge remote-tracking branch 'origin/master' into fix/prevent-pykdl-s…
MatthijsBurgh Aug 7, 2020
8698e70
(PyKDL) throw index error in getSegment
MatthijsBurgh Aug 7, 2020
114bad4
(PyKDL)(sip) fix getSegments
MatthijsBurgh Aug 7, 2020
ceb57a0
Merge remote-tracking branch 'origin/master' into fix/prevent-pykdl-s…
MatthijsBurgh Aug 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions python_orocos_kdl/PyKDL/pybind11/kinfam.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,12 @@ void init_kinfam(pybind11::module &m)
chain.def("addChain", &Chain::addChain);
chain.def("getNrOfJoints", &Chain::getNrOfJoints);
chain.def("getNrOfSegments", &Chain::getNrOfSegments);
chain.def("getSegment", (Segment& (Chain::*)(unsigned int)) &Chain::getSegment);
chain.def("getSegment", (const Segment& (Chain::*)(unsigned int) const) &Chain::getSegment);
chain.def("getSegment", [](const Chain &c, unsigned int nr)
Copy link
Member

@meyerj meyerj Sep 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I am not so familiar with PyBind11, but this lambda function returns a const Segment & unconditionally, because the input is a const Chain &. Does it make a difference? Is the returned Segment supposed to have reference semantics or does const-correctness matter at all for the Python bindings?

The previous version had a const and a non-const overload, and the SIP version below only has a non-const implementation of getSegment(nr) and the const declaration has been removed.

From this document in the pybind11 repo I understand that the const-ness of returned values is even cast away by pybind11, and that the returned Segment instance is still mutable in Python, with reference semantics? In that case, would

Suggested change
chain.def("getSegment", [](const Chain &c, unsigned int nr)
chain.def("getSegment", [](Chain &c, unsigned int nr)

have worked here, too, to emphasize that the returned Segment is mutable and the Chain instance can be modified through this method?

{
if (nr >= c.getNrOfSegments())
throw py::index_error("Chain index out of range");
return c.getSegment(nr);
});
chain.def("__repr__", [](const Chain &c)
{
std::ostringstream oss;
Expand Down
14 changes: 11 additions & 3 deletions python_orocos_kdl/PyKDL/sip/kinfam.sip
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,16 +177,24 @@ public:
unsigned int getNrOfJoints()const;
unsigned int getNrOfSegments()const;

Segment& getSegment(unsigned int nr);
%MethodCode
if (a0 < 0 || a0 >= sipCpp->getNrOfSegments()) {
PyErr_SetString(PyExc_IndexError, "Chain index out of range");
sipError = sipErrorFail;
}
else {
sipRes = &(sipCpp->getSegment(a0));
}
%End

const std::string* __repr__() const;
%MethodCode
std::ostringstream oss;
oss<<(*sipCpp);
std::string s(oss.str());
sipRes=&s;
%End

const Segment& getSegment(unsigned int nr)const /Factory/;

};

class Tree {
Expand Down