Use correct functions to resize and get an item, avoiding memory leaks in typesupport code #228

iuhilnehc-ynos · 2020-08-28T03:18:12Z

This PR is related to #219

NOTE: I used multiple commits in a PR because it'll easy for me to revert some of them if they're unnecessary.

Signed-off-by: Chen.Lihui lihui.chen@sony.com

fujitatomoya

LGTM

ivanpauno

I have left some comments here and there.
Thanks for this @iuhilnehc-ynos !!!

rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/TypeSupport_impl.hpp

ivanpauno · 2020-08-28T12:56:41Z

NOTE: I used multiple commits in a PR because it'll easy for me to revert some of them if they're unnecessary.

That's perfect. We (almost) always squash and merge at the end, so the amount of commits in the PR doesn't matter.

eboasson

Thanks so much @iuhilnehc-ynos and @ivanpauno !

ivanpauno

LGTM, thanks @iuhilnehc-ynos !!!

ivanpauno · 2020-08-31T12:56:00Z

Linux
Linux-aarch64
macOS
Windows

ivanpauno · 2020-08-31T15:51:21Z

Same job with master to compare failures:

Linux

ivanpauno · 2020-08-31T16:37:04Z

@iuhilnehc-ynos some tests are segfaulting, and don't fail with master.
The test_communication failures seem like a good place to investigate.

ivanpauno · 2020-08-31T16:39:16Z

rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/TypeSupport.hpp

@@ -78,7 +78,7 @@ struct StringHelper<rosidl_typesupport_introspection_c__MessageMembers>
    return std::string(data.data);
  }

-  static void assign(cycdeser & deser, void * field, bool)
+  static void assign(cycdeser & deser, void * field)


don't you need to do the same in L101?

Yes, we need to remove it. Sorry for my mistake.
I'll retest them(including rcl, rclcpp, system_tests) on my local environment.
If errors still exist, I'll investigate on them.

iuhilnehc-ynos · 2020-09-01T06:02:33Z

@ivanpauno

After debugging test_messages_c__rmw_cyclonedds_cpp of test_communication and checking source code, I found we should keep the original source code for rosidl_typesupport_introspection_c__MessageMember structure.

rmw_cyclonedds/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/TypeSupport_impl.hpp

Lines 525 to 528 in b403b42

    
           void * subros_message = nullptr; 
        
           size_t array_size = 0; 
        
           size_t sub_members_size = sub_members->size_of_; 
        
           size_t max_align = calculateMaxAlign(sub_members);

and

rmw_cyclonedds/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/TypeSupport_impl.hpp

Lines 543 to 544 in b403b42

    
           subros_message = static_cast<char *>(subros_message) + sub_members_size; 
        
           subros_message = align_ptr_(max_align, subros_message);

Maybe we can update as following,

item 1 (make sure to use the resize_function if member is rosidl_typesupport_introspection_cpp::MessageMember)
keep get_submessage_array_deserialize, update it
from

rmw_cyclonedds/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/TypeSupport_impl.hpp

Lines 430 to 448 in b403b42

    
           inline size_t get_submessage_array_deserialize( 
        
             const rosidl_typesupport_introspection_cpp::MessageMember * member, 
        
             cycdeser & deser, 
        
             void * field, 
        
             void * & subros_message, 
        
             bool call_new, 
        
             size_t sub_members_size, 
        
             size_t max_align) 
        
           { 
        
             (void)member; 
        
             uint32_t vsize = deser.deserialize_len(1); 
        
             auto vector = reinterpret_cast<std::vector<unsigned char> *>(field); 
        
             if (call_new) { 
        
               new(vector) std::vector<unsigned char>; 
        
             } 
        
             vector->resize(vsize * align_int_(max_align, sub_members_size)); 
        
             subros_message = reinterpret_cast<void *>(vector->data()); 
        
             return vsize; 
        
           }

to

inline size_t get_submessage_array_deserialize(
  const rosidl_typesupport_introspection_cpp::MessageMember * member,
  cycdeser & deser,
  void * field,
  void * & subros_message,
  size_t,
  size_t)
{
  uint32_t vsize = deser.deserialize_len(1);
  member->resize_function(field, vsize);
  subros_message = member->get_function(field, 0);
  return vsize;
}

item 2 (add a function get_submessage_pointer to get pointer for rosidl_typesupport_introspection_cpp::MessageMember and rosidl_typesupport_introspection_c__MessageMember)

from

rmw_cyclonedds/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/TypeSupport_impl.hpp

Lines 541 to 545 in b403b42

    
           for (size_t index = 0; index < array_size; ++index) { 
        
             deserializeROSmessage(deser, sub_members, subros_message, recall_new); 
        
             subros_message = static_cast<char *>(subros_message) + sub_members_size; 
        
             subros_message = align_ptr_(max_align, subros_message); 
        
           }

to

            for (size_t index = 0; index < array_size; ++index) {
              deserializeROSmessage(deser, sub_members, subros_message);
              subros_message = get_submessage_pointer(member, field, index+1, subros_message, sub_members_size, max_align);
            }

add new function get_submessage_pointer

inline void* get_submessage_pointer(
  const rosidl_typesupport_introspection_cpp::MessageMember * member,
  void * field,
  size_t index,
  void *,
  size_t,
  size_t)
{
  return member->get_function(field, index);
}

inline void* get_submessage_pointer(
  const rosidl_typesupport_introspection_c__MessageMember *,
  void *,
  size_t,
  void * subros_message,
  size_t sub_members_size,
  size_t max_align)
{
    subros_message = static_cast<char *>(subros_message) + sub_members_size;
    return align_ptr_(max_align, subros_message);
}

What do you think about it?

iuhilnehc-ynos · 2020-09-01T06:07:31Z

BTW, I have also tested rcl by using master branch on my local environment, but it caused segment fault while testing test_node__rmw_cyclonedds_cpp because of test_rcl_node_init_with_internal_errors.

iuhilnehc-ynos · 2020-09-01T06:47:47Z

it caused segment fault while testing test_node__rmw_cyclonedds_cpp because of test_rcl_node_init_with_internal_errors

Ignore it. The testing master branch is behind of remote.
NOTE: Before testing with CI, I need to rebase rmw_cyclonedds_cpp from master of ros2/rmw_cyclonedds.

This reverts commit 416c01d. Signed-off-by: Chen.Lihui <lihui.chen@sony.com>

Signed-off-by: Chen.Lihui <lihui.chen@sony.com>

Signed-off-by: Chen Lihui <lihui.chen@sony.com> Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

ivanpauno · 2020-09-01T13:05:37Z

After debugging test_messages_c__rmw_cyclonedds_cpp of test_communication and checking source code, I found we should keep the original source code for rosidl_typesupport_introspection_c__MessageMember structure.

Sounds ok to me, but why is that the case?

iuhilnehc-ynos · 2020-09-01T13:49:05Z

Sounds ok to me, but why is that the case?

Because test_messages_c__rmw_cyclonedds_cpp tests c structure of message which will use rmw_cyclonedds_cpp::TypeSupport<rosidl_typesupport_introspection_c__MessageMembers>::deserializeROSmessage

backtrace log

(gdb) bt
#0  0x00007ffff6ebedad in cycdeser::deserialize (this=0x7fffffff46f0, x=<error reading variable>)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/serdes.hpp:141
#1  0x00007ffff6ebe6a7 in cycdeser::operator>> (this=0x7fffffff46f0, x=<error reading variable>)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/serdes.hpp:101
#2  0x00007ffff6ed3b3c in rmw_cyclonedds_cpp::deserialize_field<bool> (member=0x7ffff6c3bfe0 <BasicTypes__rosidl_typesupport_introspection_c__BasicTypes_message_member_array>, 
    field=0xff000000, deser=...) at /home/chenlh/Projects/ROS2/ros2_ws_master/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/TypeSupport_impl.hpp:198
#3  0x00007ffff6eca107 in rmw_cyclonedds_cpp::TypeSupport<rosidl_typesupport_introspection_c__MessageMembers>::deserializeROSmessage (this=0x5555559422c0, deser=..., 
    members=0x7ffff6c3a5a0 <BasicTypes__rosidl_typesupport_introspection_c__BasicTypes_message_members>, ros_message=0xff000000)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/TypeSupport_impl.hpp:298
#4  0x00007ffff6eca39f in rmw_cyclonedds_cpp::TypeSupport<rosidl_typesupport_introspection_c__MessageMembers>::deserializeROSmessage (this=0x5555559422c0, deser=..., 
    members=0x7ffff6c3a560 <Arrays__rosidl_typesupport_introspection_c__Arrays_message_members>, ros_message=0x7fffffff64d0)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/TypeSupport_impl.hpp:362
#5  0x00007ffff6ec3f69 in rmw_cyclonedds_cpp::TypeSupport<rosidl_typesupport_introspection_c__MessageMembers>::deserializeROSmessage(cycdeser&, void*, std::function<void (cycdeser&)>) (this=0x5555559422c0, deser=..., ros_message=0x7fffffff64d0, prefix=...)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/TypeSupport_impl.hpp:487
#6  0x00007ffff6f191e5 in serdata_rmw_to_sample (dcmn=0x55555594ae90, sample=0x7fffffff64d0, bufptr=0x0, buflim=0x0)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/serdata.cpp:284
#7  0x00007ffff6cf2ad9 in ddsi_serdata_to_sample (d=0x55555594ae90, sample=0x7fffffff64d0, bufptr=0x0, buflim=0x0)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/eclipse-cyclonedds/cyclonedds/src/security/api/../../core/ddsi/include/dds/ddsi/ddsi_serdata.h:230
#8  0x00007ffff6d739ed in read_take_to_sample (d=0x55555594ae90, sample=0x7fffffff4c30, bufptr=0x0, buflim=0x0)
--Type <RET> for more, q to quit, c to continue without paging--
   lonedds/src/core/ddsc/src/dds_rhc_default.c:1956
#9  0x00007ffff6d7415c in take_w_qminv_inst (rhc=0x55555594a870, instptr=0x7fffffff4910, values=0x7fffffff4c30, info_seq=0x7fffffff4c50, max_samples=1, qminv=0, qcmask=0, 
    ntriggers=0x7fffffff4908, to_sample=0x7ffff6d739b6 <read_take_to_sample>, to_invsample=0x7ffff6d739ef <read_take_to_invsample>)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsc/src/dds_rhc_default.c:2091
#10 0x00007ffff6d74cae in take_w_qminv (rhc=0x55555594a870, lock=true, values=0x7fffffff4c30, info_seq=0x7fffffff4c50, max_samples=1, qminv=0, handle=0, cond=0x0, 
    to_sample=0x7ffff6d739b6 <read_take_to_sample>, to_invsample=0x7ffff6d739ef <read_take_to_invsample>)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsc/src/dds_rhc_default.c:2227
#11 0x00007ffff6d74ec3 in dds_rhc_take_w_qminv (rhc=0x55555594a870, lock=true, values=0x7fffffff4c30, info_seq=0x7fffffff4c50, max_samples=1, qminv=0, handle=0, cond=0x0)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsc/src/dds_rhc_default.c:2251
#12 0x00007ffff6d76a5c in dds_rhc_default_take (rhc_common=0x55555594a870, lock=true, values=0x7fffffff4c30, info_seq=0x7fffffff4c50, max_samples=1, mask=128, handle=0, cond=0x0)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsc/src/dds_rhc_default.c:2663
#13 0x00007ffff6d6ec71 in dds_rhc_take (rhc=0x55555594a870, lock=true, values=0x7fffffff4c30, info_seq=0x7fffffff4c50, max_samples=1, mask=128, handle=0, cond=0x0)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsc/include/dds/ddsc/dds_rhc.h:83
#14 0x00007ffff6d83767 in dds_read_impl (take=true, reader_or_condition=1794363109, buf=0x7fffffff4c30, bufsz=1, maxs=1, si=0x7fffffff4c50, mask=128, hand=0, lock=true, 
    only_reader=false) at /home/chenlh/Projects/ROS2/ros2_ws_master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsc/src/dds_read.c:111
#15 0x00007ffff6d83fc4 in dds_take (rd_or_cnd=1794363109, buf=0x7fffffff4c30, si=0x7fffffff4c50, bufsz=1, maxs=1)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/eclipse-cyclonedds/cyclonedds/src/core/ddsc/src/dds_read.c:332
#16 0x00007ffff6eb3393 in rmw_take_int (subscription=0x555555954eb0, ros_message=0x7fffffff64d0, taken=0x7fffffff4dab, message_info=0x7fffffff4dc0)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp:2508
#17 0x00007ffff6eb3db5 in rmw_take_with_info (subscription=0x555555954eb0, ros_message=0x7fffffff64d0, taken=0x7fffffff4dab, message_info=0x7fffffff4dc0, allocation=0x0)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/ros2/rmw_cyclonedds/rmw_cyclonedds_cpp/src/rmw_node.cpp:2675
#18 0x00007ffff7223f24 in rmw_take_with_info (v5=0x555555954eb0, v4=0x7fffffff64d0, v3=0x7fffffff4dab, v2=0x7fffffff4dc0, v1=0x0)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/ros2/rmw_implementation/rmw_implementation/src/functions.cpp:350
#19 0x00007ffff77a0dee in rcl_take (subscription=0x7fffffff6268, ros_message=0x7fffffff64d0, message_info=0x0, allocation=0x0)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/ros2/rcl/rcl/src/rcl/subscription.c:276
#20 0x00005555555875f4 in TestMessagesFixture::test_message_type<test_msgs__msg__Arrays> (this=0x555555634b80, topic_name=0x5555555dde0a "test_arrays", 
    ts=0x7ffff7758070 <test_msgs::msg::rosidl_typesupport_c::Arrays_message_type_support_handle>, context=0x555555634ec0)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/ros2/system_tests/test_communication/test/test_messages_c.cpp:229
#21 0x000055555556b4f2 in TestMessagesFixture_test_arrays_Test::TestBody (this=0x555555634b80)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/src/ros2/system_tests/test_communication/test/test_messages_c.cpp:747
#22 0x00005555555c8e88 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void> (object=0x555555634b80, method=&virtual testing::Test::TestBody(), 
--Type <RET> for more, q to quit, c to continue without paging--
    location=0x5555555dfd73 "the test body") at /home/chenlh/Projects/ROS2/ros2_ws_master/local_install_test/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2447
#23 0x00005555555c1885 in testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void> (object=0x555555634b80, method=&virtual testing::Test::TestBody(), 
    location=0x5555555dfd73 "the test body") at /home/chenlh/Projects/ROS2/ros2_ws_master/local_install_test/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2483
#24 0x000055555559cef4 in testing::Test::Run (this=0x555555634b80) at /home/chenlh/Projects/ROS2/ros2_ws_master/local_install_test/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2522
#25 0x000055555559d8ed in testing::TestInfo::Run (this=0x555555612740)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/local_install_test/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2703
#26 0x000055555559dfde in testing::TestCase::Run (this=0x555555611a80)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/local_install_test/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2825
#27 0x00005555555a96c7 in testing::internal::UnitTestImpl::RunAllTests (this=0x555555611650)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/local_install_test/gtest_vendor/src/gtest_vendor/./src/gtest.cc:5216
#28 0x00005555555ca382 in testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x555555611650, 
    method=(bool (testing::internal::UnitTestImpl::*)(testing::internal::UnitTestImpl * const)) 0x5555555a941e <testing::internal::UnitTestImpl::RunAllTests()>, 
    location=0x5555555e0778 "auxiliary test code (environments or event listeners)")
    at /home/chenlh/Projects/ROS2/ros2_ws_master/local_install_test/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2447
#29 0x00005555555c2999 in testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool> (object=0x555555611650, 
    method=(bool (testing::internal::UnitTestImpl::*)(testing::internal::UnitTestImpl * const)) 0x5555555a941e <testing::internal::UnitTestImpl::RunAllTests()>, 
    location=0x5555555e0778 "auxiliary test code (environments or event listeners)")
    at /home/chenlh/Projects/ROS2/ros2_ws_master/local_install_test/gtest_vendor/src/gtest_vendor/./src/gtest.cc:2483
#30 0x00005555555a80a6 in testing::UnitTest::Run (this=0x555555610420 <testing::UnitTest::GetInstance()::instance>)
    at /home/chenlh/Projects/ROS2/ros2_ws_master/local_install_test/gtest_vendor/src/gtest_vendor/./src/gtest.cc:4824
#31 0x0000555555594b44 in RUN_ALL_TESTS () at /home/chenlh/Projects/ROS2/ros2_ws_master/local_install_test/gtest_vendor/src/gtest_vendor/include/gtest/gtest.h:2370
#32 0x0000555555594ac6 in main (argc=1, argv=0x7fffffff70f8) at /home/chenlh/Projects/ROS2/ros2_ws_master/local_install_test/gtest_vendor/src/gtest_vendor/src/gtest_main.cc:36

iuhilnehc-ynos · 2020-09-01T14:13:57Z

@ivanpauno

I have tested on my local environment.

100% tests passed, 0 tests failed out of 96

Label Time Summary:
copyright     =   6.95 sec*proc (1 test)
cppcheck      =   3.27 sec*proc (1 test)
cpplint       =   5.49 sec*proc (1 test)
gtest         =  97.30 sec*proc (84 tests)
lint_cmake    =   0.26 sec*proc (1 test)
linter        =  18.62 sec*proc (6 tests)
uncrustify    =   0.89 sec*proc (1 test)
xfail         =   2.19 sec*proc (1 test)
xmllint       =   1.77 sec*proc (1 test)

Total Test time (real) = 118.14 sec
Finished <<< rcl [1min 58s]  


100% tests passed, 0 tests failed out of 69

Label Time Summary:
copyright     =  14.81 sec*proc (1 test)
cppcheck      =  51.79 sec*proc (1 test)
cpplint       =   7.81 sec*proc (1 test)
gmock         =   0.46 sec*proc (2 tests)
gtest         = 103.00 sec*proc (61 tests)
lint_cmake    =   0.27 sec*proc (1 test)
linter        =  77.35 sec*proc (6 tests)
uncrustify    =   1.01 sec*proc (1 test)
xmllint       =   1.67 sec*proc (1 test)

Total Test time (real) = 180.83 sec
Finished <<< rclcpp [3min 1s]


100% tests passed, 0 tests failed out of 164

Label Time Summary:
copyright     =   1.31 sec*proc (1 test)
cppcheck      =   0.40 sec*proc (1 test)
cpplint       =   0.70 sec*proc (1 test)
flake8        =   0.56 sec*proc (1 test)
gtest         =   7.78 sec*proc (9 tests)
lint_cmake    =   0.27 sec*proc (1 test)
linter        =   5.84 sec*proc (8 tests)
pep257        =   0.32 sec*proc (1 test)
uncrustify    =   0.33 sec*proc (1 test)
xmllint       =   1.95 sec*proc (1 test)

Total Test time (real) = 283.13 sec
Finished <<< test_communication [4min 43s]

ivanpauno · 2020-09-01T15:45:01Z

Because test_messages_c__rmw_cyclonedds_cpp tests c structure of message which will use rmw_cyclonedds_cpp::TypeSupport<rosidl_typesupport_introspection_c__MessageMembers>::deserializeROSmessage

That backtrace shows where it is failing, but it doesn't explain why it is failing.
Ideally, we shouldn't have any alignment code or weird stuff here, the generated typesupport should do that well.

I'm reading the generated "get_function" for C typesupport:

void * Arrays__rosidl_typesupport_introspection_c__get_function__BasicTypes__basic_types_values(
  void * untyped_member, size_t index)
{
  test_msgs__msg__BasicTypes ** member =
    (test_msgs__msg__BasicTypes **)(untyped_member);
  return &(*member)[index];
}

and it looks ok to me, so I'm not quite sure of what caused the segfaults.

ivanpauno · 2020-09-01T22:58:54Z

and it looks ok to me

actually, isn't that expecting &field instead of field?
I need to double check this tomorrow again, I'm a bit confused now 🤯.
I hope it isn't the case (that would be a bizarre inconsistency between c and cpp typesupport).

iuhilnehc-ynos · 2020-09-02T02:18:16Z

inconsistency between c and cpp typesupport

Yes, I think we need to go deep into c typesupport, and find out why the get_function is not working. Maybe I lost something important.

iuhilnehc-ynos · 2020-09-02T04:02:44Z

@ivanpauno

I'm reading the generated "get_function" for C typesupport:
isn't that expecting &field instead of field?

I also debug it, and I think you're right. After deserializing the members of test_msgs__msg__Arrays from bool_values to string_values, it reached the basic_types_values, at the time, the field is pointed the right address (msg->basic_types_values address), and then to use member->get_function(field, index) to call the following function

void * Arrays__rosidl_typesupport_introspection_c__get_function__BasicTypes__basic_types_values(
  void * untyped_member, size_t index)
{
  test_msgs__msg__BasicTypes ** member =
    (test_msgs__msg__BasicTypes **)(untyped_member);
  return &(*member)[index];        // member is (test_msgs__msg__BasicTypes ** ), after dereferencing member, it gets error address.
}

but I think maybe it could be updated as following, so it could keep the same logic (to use get_function with field, not &field) with c++ typesupport.

  test_msgs__msg__BasicTypes * member =
    (test_msgs__msg__BasicTypes *)(untyped_member);
  return &member[index];

If so, we need to update the template of rosidl,
https://github.com/ros2/rosidl/blob/ae9a3d5bd15d3ddf96cf25bf1663442ad1532a32/rosidl_typesupport_introspection_c/resource/msg__type_support.c.em#L140-L142
and
https://github.com/ros2/rosidl/blob/ae9a3d5bd15d3ddf96cf25bf1663442ad1532a32/rosidl_typesupport_introspection_c/resource/msg__type_support.c.em#L154-L156

BTW: I have already updated the arrays__type_support.c by manual, and tested it successfully.

ivanpauno · 2020-09-02T14:28:46Z

but I think maybe it could be updated as following, so it could keep the same logic (to use get_function with field, not &field) with c++ typesupport.

Yeah, I think that fix is correct, but it would be great to first merge a fix here (I don't think that change in rosidl_typesupport_introspection is backportable).

This is unfortunately only happening with C typesupports arrays (not with bounded sequences/sequences), so you'll need special logic to handle that here (it's a bit annoying to add that extra logic, but possible).

iuhilnehc-ynos · 2020-09-03T01:09:43Z

so you'll need special logic to handle that here (it's a bit annoying to add that extra logic, but possible).

OK, I'll fix it in this way.

Signed-off-by: Chen.Lihui <lihui.chen@sony.com> Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

iuhilnehc-ynos · 2020-09-03T02:18:46Z

@ivanpauno

I force-pushed my branch because I removed the keep original implementation for C typesupport commits that you didn't review. I think it will make us clear.

ivanpauno · 2020-09-03T13:19:03Z

CI 🤞 :

Linux
Linux-aarch64
macOS
Windows

ivanpauno · 2020-09-03T16:20:29Z

Failures are unrelated, see:

ivanpauno · 2020-09-03T16:21:15Z

Merging! Thanks @iuhilnehc-ynos for this great fix and your patience in the reviewing process!!!!

jacobperron · 2020-11-10T20:22:59Z

Looks like this PR is changing API in publicly accessible headers. On these grounds I don't think we should backport it so I'm removing it from the Foxy release board.

eboasson · 2020-11-11T09:08:44Z

@jacobperron you mean the "TypeSupport.hpp" and "TypeSupport_impl.hpp"? They are meant to be private to the RMW implementation, they only reside in the public include directory because of a historical accident: they happened to be in that directory when I started the RMW layer without knowing anything about ROS 2 and its directory structure.

Note that I am fine with the decision not to merge this fix into Foxy on the off-chance that someone uses those header files — such are the rules of the game. However, the likelihood of anyone actually using them in such a way appears to me to be vanishingly small, and in my view the benefits from fixing the leaks therefore outweigh the downside. But, again, that's just my view.

jacobperron · 2020-11-11T18:01:51Z

@eboasson On second glance, the only "problematic" file is "TypeSupport_impl.hpp". I agree that the likelihood of anyone using it is slim. I could be convinced of backporting this change along with a release note if others want to move forward with the backport.

…s in typesupport code (ros2#228) Signed-off-by: Chen.Lihui <lihui.chen@sony.com>

fujitatomoya approved these changes Aug 28, 2020

View reviewed changes

ivanpauno reviewed Aug 28, 2020

View reviewed changes

rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/TypeSupport_impl.hpp Show resolved Hide resolved

rmw_cyclonedds_cpp/include/rmw_cyclonedds_cpp/TypeSupport_impl.hpp Outdated Show resolved Hide resolved

ivanpauno added the bug Something isn't working label Aug 28, 2020

iuhilnehc-ynos requested a review from ivanpauno August 31, 2020 01:19

eboasson approved these changes Aug 31, 2020

View reviewed changes

ivanpauno approved these changes Aug 31, 2020

View reviewed changes

ivanpauno reviewed Aug 31, 2020

View reviewed changes

Chen.Lihui and others added 5 commits September 1, 2020 17:54

Revert "Fix memory leak that string not deleted. (ros2#224)"

1a6fd3a

This reverts commit 416c01d. Signed-off-by: Chen.Lihui <lihui.chen@sony.com>

Use correct functions to resize and get pointer

a059257

Signed-off-by: Chen.Lihui <lihui.chen@sony.com>

Remove unnecessary get_submessage_array_deserialize

a6e60b2

Signed-off-by: Chen.Lihui <lihui.chen@sony.com>

Get rid of call_new

179794c

Signed-off-by: Chen.Lihui <lihui.chen@sony.com>

Fixed based on suggestion

c370d9b

Signed-off-by: Chen Lihui <lihui.chen@sony.com> Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

iuhilnehc-ynos force-pushed the topic-correct-resize-func branch from 7c08f00 to 4d55d82 Compare September 1, 2020 13:51

Add extra logic for array of C typesupport to get pointer

16e7b1d

Signed-off-by: Chen.Lihui <lihui.chen@sony.com> Co-authored-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

iuhilnehc-ynos force-pushed the topic-correct-resize-func branch from 4d55d82 to 16e7b1d Compare September 3, 2020 02:12

ivanpauno approved these changes Sep 3, 2020

View reviewed changes

ivanpauno changed the title ~~Use correct functions to resize and get pointer to avoid memory leak~~ Use correct functions to resize and get an item, avoiding memory leaks in typesupport code Sep 3, 2020

ivanpauno merged commit 6d84fc3 into ros2:master Sep 3, 2020

MiguelCompany mentioned this pull request Sep 7, 2020

Avoid memory leaks and undefined behavior in rmw_fastrtps_dynamic_cpp typesupport code ros2/rmw_fastrtps#429

Merged

This was referenced Sep 24, 2020

Fix array get_function semantics #248

Merged

[rosidl_typesupport_introspection_c] Fix get_function and get_const_function semantics for arrays ros2/rosidl#531

Merged

eboasson mentioned this pull request Feb 2, 2021

abort() in test_subscription__rmw_cyclonedds_cpp #279

Closed

eboasson pushed a commit to eboasson/rmw_cyclonedds that referenced this pull request May 17, 2021

Use correct functions to resize and get an item, avoiding memory leak…

802058c

…s in typesupport code (ros2#228) Signed-off-by: Chen.Lihui <lihui.chen@sony.com>

ito-san pushed a commit to tier4/rmw_cyclonedds that referenced this pull request Jun 3, 2021

Use correct functions to resize and get an item, avoiding memory leak…

7c569f4

…s in typesupport code (ros2#228) Signed-off-by: Chen.Lihui <lihui.chen@sony.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use correct functions to resize and get an item, avoiding memory leaks in typesupport code #228

Use correct functions to resize and get an item, avoiding memory leaks in typesupport code #228

iuhilnehc-ynos commented Aug 28, 2020

fujitatomoya left a comment

ivanpauno left a comment

ivanpauno commented Aug 28, 2020

eboasson left a comment

ivanpauno left a comment

ivanpauno commented Aug 31, 2020 •

edited

Loading

ivanpauno commented Aug 31, 2020

ivanpauno commented Aug 31, 2020

ivanpauno Aug 31, 2020

iuhilnehc-ynos Sep 1, 2020 •

edited

Loading

iuhilnehc-ynos commented Sep 1, 2020

iuhilnehc-ynos commented Sep 1, 2020

iuhilnehc-ynos commented Sep 1, 2020

ivanpauno commented Sep 1, 2020

iuhilnehc-ynos commented Sep 1, 2020

iuhilnehc-ynos commented Sep 1, 2020

ivanpauno commented Sep 1, 2020

ivanpauno commented Sep 1, 2020

iuhilnehc-ynos commented Sep 2, 2020

iuhilnehc-ynos commented Sep 2, 2020 •

edited

Loading

ivanpauno commented Sep 2, 2020

iuhilnehc-ynos commented Sep 3, 2020

iuhilnehc-ynos commented Sep 3, 2020

ivanpauno commented Sep 3, 2020 •

edited

Loading

ivanpauno commented Sep 3, 2020

ivanpauno commented Sep 3, 2020

jacobperron commented Nov 10, 2020

eboasson commented Nov 11, 2020

jacobperron commented Nov 11, 2020

Use correct functions to resize and get an item, avoiding memory leaks in typesupport code #228

Use correct functions to resize and get an item, avoiding memory leaks in typesupport code #228

Conversation

iuhilnehc-ynos commented Aug 28, 2020

fujitatomoya left a comment

Choose a reason for hiding this comment

ivanpauno left a comment

Choose a reason for hiding this comment

ivanpauno commented Aug 28, 2020

eboasson left a comment

Choose a reason for hiding this comment

ivanpauno left a comment

Choose a reason for hiding this comment

ivanpauno commented Aug 31, 2020 • edited Loading

ivanpauno commented Aug 31, 2020

ivanpauno commented Aug 31, 2020

ivanpauno Aug 31, 2020

Choose a reason for hiding this comment

iuhilnehc-ynos Sep 1, 2020 • edited Loading

Choose a reason for hiding this comment

iuhilnehc-ynos commented Sep 1, 2020

iuhilnehc-ynos commented Sep 1, 2020

iuhilnehc-ynos commented Sep 1, 2020

ivanpauno commented Sep 1, 2020

iuhilnehc-ynos commented Sep 1, 2020

iuhilnehc-ynos commented Sep 1, 2020

ivanpauno commented Sep 1, 2020

ivanpauno commented Sep 1, 2020

iuhilnehc-ynos commented Sep 2, 2020

iuhilnehc-ynos commented Sep 2, 2020 • edited Loading

ivanpauno commented Sep 2, 2020

iuhilnehc-ynos commented Sep 3, 2020

iuhilnehc-ynos commented Sep 3, 2020

ivanpauno commented Sep 3, 2020 • edited Loading

ivanpauno commented Sep 3, 2020

ivanpauno commented Sep 3, 2020

jacobperron commented Nov 10, 2020

eboasson commented Nov 11, 2020

jacobperron commented Nov 11, 2020

ivanpauno commented Aug 31, 2020 •

edited

Loading

iuhilnehc-ynos Sep 1, 2020 •

edited

Loading

iuhilnehc-ynos commented Sep 2, 2020 •

edited

Loading

ivanpauno commented Sep 3, 2020 •

edited

Loading