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.

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

Jump to bottom

Segmentation error to dereference nullptr #180

Merged
merged 5 commits into from
Dec 19, 2017
Merged

Segmentation error to dereference nullptr #180

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fe7e672
Segmentation error to dereference nullptr
gaoethan Dec 11, 2017
f60f0dd
Avoid null dereference to application crash
gaoethan Dec 11, 2017
8413f3e
change the way not equal to nullptr
gaoethan Dec 15, 2017
1b8dd88
reflect the failure to get EDP readers
gaoethan Dec 15, 2017
209cf63
check either edp_reader
gaoethan Dec 18, 2017
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
13 changes: 9 additions & 4 deletions rmw_fastrtps_cpp/src/rmw_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,13 @@ create_node(
node_impl->secondaryPubListener = tnat_2;

edp_readers = participant->getEDPReaders();
if (!(edp_readers.first->setListener(tnat_1) & edp_readers.second->setListener(tnat_2))) {
RMW_SET_ERROR_MSG("Failed to attach ROS related logic to the Participant");
if (edp_readers.first && edp_readers.second) {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe just my personal taste, but I would proof every potentially failing test individually. This makes the error message less ambiguous whether condition 1 or condition 2 fails.
Also, I like the style of testing for negative first and return/goto fail early.

if (!edp_readers.first) {
  RMW_SET_ERROR_MSG("edp_readers.first is null");
  got fail;
}
...

But then again, only my 2 cents.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, why I put them together is that:

  1. their definition is a std::pair<> a. k. a std::pair<StatefulReader *, StatefulReader *> edp_readers;
  2. getEDPReaders() returns them both NULL at the same time and no case to return one of them NULL

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the current documentation of this function here, one could argue there is no notion that they are always returned NULL - even though this might be the case in the current verison. However, this implementation may change over time, so that we can't certainly rely on this assumption.

if (!(edp_readers.first->setListener(tnat_1) & edp_readers.second->setListener(tnat_2))) {
RMW_SET_ERROR_MSG("Failed to attach ROS related logic to the Participant");
goto fail;
}
} else {
RMW_SET_ERROR_MSG("Failed to get valid reader for node subsciber and publisher");
goto fail;
}

Expand Down Expand Up @@ -272,12 +277,12 @@ rmw_destroy_node(rmw_node_t * node)

// Begin deleting things in the same order they were created in rmw_create_node().
std::pair<StatefulReader *, StatefulReader *> edp_readers = participant->getEDPReaders();
if (!edp_readers.first->setListener(nullptr)) {
if (!edp_readers.first || !edp_readers.first->setListener(nullptr)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above of testing the conditions individually.

RMW_SET_ERROR_MSG("failed to unset EDPReader listener");
result_ret = RMW_RET_ERROR;
}
delete impl->secondarySubListener;
if (!edp_readers.second->setListener(nullptr)) {
if (!edp_readers.second || !edp_readers.second->setListener(nullptr)) {
RMW_SET_ERROR_MSG("failed to unset EDPReader listener");
result_ret = RMW_RET_ERROR;
}
Expand Down
6 changes: 5 additions & 1 deletion rmw_fastrtps_cpp/src/rmw_service.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,10 @@ rmw_create_service(
}

rmw_service = rmw_service_allocate();
if (!rmw_service) {
RMW_SET_ERROR_MSG("failed to allocate memory for service");
goto fail;
}
rmw_service->implementation_identifier = eprosima_fastrtps_identifier;
rmw_service->data = info;
rmw_service->service_name = reinterpret_cast<const char *>(
Expand Down Expand Up @@ -237,7 +241,7 @@ rmw_create_service(
delete info;
}

if (rmw_service->service_name != nullptr) {
if (rmw_service && rmw_service->service_name != nullptr) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it looks to me like a redundant test, because rmw_service was checked for null in line 202. Also, try to use the same notation of (!rmw_service->service_name) instead of != nullptr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, if it goes to fail before rmw_service is really initialized (a. k. a assigned to a valid address at line 201), even though the rmw_service is nullptr, but rmw_service->service_name is actually not 0x0 and it should be some value like 0x10, then there will be "Segmentation fault (core dumped)" error

Copy link
Contributor

Choose a reason for hiding this comment

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

please apologize, I haven't noticed the modified condition was inside the fail. Your change is completely correct here.

rmw_free(const_cast<char *>(rmw_service->service_name));
rmw_service->service_name = nullptr;
}
Expand Down