Fix crash for bag migration #239

Merged
merged 1 commit into from Jun 18, 2013

Conversation

Projects
None yet
2 participants
@cmansley
Contributor

cmansley commented Jun 10, 2013

This appears to be a copy and paste error. You should check the
rename rule order and not the last element of the list.

Fix crash for bag migration
This appears to be a copy and paste error. You should check the
rename rule order and not the last element of the list.
@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 10, 2013

Member

Can you please provide your migration rule file which exhibits the issue?

Member

dirk-thomas commented Jun 10, 2013

Can you please provide your migration rule file which exhibits the issue?

@cmansley

This comment has been minimized.

Show comment
Hide comment
@cmansley

cmansley Jun 10, 2013

Contributor

So, I have a migration rule that induces this bug. However, I cannot provide that information publicly.

I can describe in detail what happens. It appears as if around line 549, if the iteritems function provides the rules out of order, it is possible for a rename rule to be in the rulechain before an update rule is in the rule chain. Even though the ordering of the two rules is reversed.

Then, there is a check:

if rulechain.rename and (r.order >= rulechain.chain[-1]):

which crashes because there are no rules in the rule chain, only a single rename. If you look right before the diff in the file, you can see all of the other checks, ensure rulechain.chain exists before checking it.

All I did was make the if-check match the debug print statement it encloses and the offending file works just fine. I believe this is a copy and paste error with line 752.

Contributor

cmansley commented Jun 10, 2013

So, I have a migration rule that induces this bug. However, I cannot provide that information publicly.

I can describe in detail what happens. It appears as if around line 549, if the iteritems function provides the rules out of order, it is possible for a rename rule to be in the rulechain before an update rule is in the rule chain. Even though the ordering of the two rules is reversed.

Then, there is a check:

if rulechain.rename and (r.order >= rulechain.chain[-1]):

which crashes because there are no rules in the rule chain, only a single rename. If you look right before the diff in the file, you can see all of the other checks, ensure rulechain.chain exists before checking it.

All I did was make the if-check match the debug print statement it encloses and the offending file works just fine. I believe this is a copy and paste error with line 752.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 10, 2013

Member

By reading the source I agree with the changes of the pull request. But still I wanna try it with a rule file first before merging it. If you can't provide a simple version of yours I will create one for verifying it.

Member

dirk-thomas commented Jun 10, 2013

By reading the source I agree with the changes of the pull request. But still I wanna try it with a rule file first before merging it. If you can't provide a simple version of yours I will create one for verifying it.

@cmansley

This comment has been minimized.

Show comment
Hide comment
@cmansley

cmansley Jun 10, 2013

Contributor

Yeah, I apologize about that. I tried to create a "fake" migration rule that would trigger the failure, but I was unsuccessful. I think it has something to do with the ordering coming out of the dict. So, maybe the hashes need to be specific.

Contributor

cmansley commented Jun 10, 2013

Yeah, I apologize about that. I tried to create a "fake" migration rule that would trigger the failure, but I was unsuccessful. I think it has something to do with the ordering coming out of the dict. So, maybe the hashes need to be specific.

@dirk-thomas

This comment has been minimized.

Show comment
Hide comment
@dirk-thomas

dirk-thomas Jun 18, 2013

Member

I was able to reproduce the issue with two bag migration rules: one which renames message Foo into FooRenamed and one which just modifies the fields of Foo.

This currently raises IndexError: list index out of range since rulechain.chain in line 766 is an empty array. With the patch applied loading of the migration rules works. Depending on their order number the warning from line 767 is printed.

Member

dirk-thomas commented Jun 18, 2013

I was able to reproduce the issue with two bag migration rules: one which renames message Foo into FooRenamed and one which just modifies the fields of Foo.

This currently raises IndexError: list index out of range since rulechain.chain in line 766 is an empty array. With the patch applied loading of the migration rules works. Depending on their order number the warning from line 767 is printed.

dirk-thomas added a commit that referenced this pull request Jun 18, 2013

@dirk-thomas dirk-thomas merged commit cf413c9 into ros:groovy-devel Jun 18, 2013

pierriko pushed a commit to pierriko/robotpkg that referenced this pull request Feb 21, 2014

Anthony Mallet
[middleware/ros-comm] Update to 1.9.46
Changes since 1.8.15:

1.9.46 (2013-06-18)
-------------------

* add dependency on roslisp
  (`#240 <ros/ros_comm#240>`_)
* fix crash in bag migration
  (`#239 <ros/ros_comm#239)
* add CMake function roslaunch_add_file_check()
  (`#241 <ros/ros_comm#241>`_)
* fix rosnode_ping to check if new node uri is valid before using it
  (`#235 <ros/ros_comm#235)

1.9.45 (2013-06-06)
-------------------
* improve handling of UDP transport, when fragmented packets are lost or arive
  out-of-order the connection is not dropped anymore, onle a single message is
  lost (`#226 <ros/ros_comm#226)
* fix missing generation of constant definitions for services
  (`ros/gencpp#2 <ros/gencpp#2>`_)
* fix restoring thread context when callback throws an exception
  (`#219 <ros/ros_comm#219>`_)
* fix calling PollManager::shutdown() repeatedly
  (`#217 <ros/ros_comm#217>`_)
* add missing run_depend on python-yaml
* allow configuration of ports for XML RPCs and TCP ROS
* fix race condition where rospy subscribers do not connect to all publisher
* fix closing and deregistering connection when connect fails
  (`#128 <ros/ros_comm#128>`_)
* fix log level of RosOutHandler
  (`#210 <ros/ros_comm#210>`_)
* added option '--duration' to 'rosbag play'
  (`#121 <ros/ros_comm#121)
* fix missing newlines in rosbag error messages
  (`#237 <ros/ros_comm#237)
* fix flushing for tools like 'rosbag compress'
  (`#237 <ros/ros_comm#237)
* add warnings for obviously wrong environment variables ROS_HOSTNAME and
  ROS_IP (`#134 <ros/ros_comm#134)
* fix exception on netifaces.ifaddresses()
  (`#211 <ros/ros_comm#211,
  `#213 <ros/ros_comm#213)
  (regression from 1.9.42)
* modify rosnode_ping to check for changed node uri if connection is refused
  (`#221 <ros/ros_comm#221>`_)
* allow passing arguments to add_rostest(ARGS ...)
 (`#232 <ros/ros_comm#232>`_)
* modified roslaunch $(find PKG) to consider path behind it for resolve
  strategy (`#233 <ros/ros_comm#233>`_)
* add boolean attribute 'subst_value' to rosparam tag in launch files
  (`#218 <ros/ros_comm#218)
* add command line parameter to print out launch args
* fix missing import in arg_dump.py
* fix template syntax for signal_.template addCallback() to work with Intel
  compiler

1.9.44 (2013-03-21)
-------------------
* fix install destination for dll's under Windows
* fix various issues on Windows
  (`#189 <ros/ros_comm#189)
* fix 'roslaunch --files' with non-unique anononymous ids
  (`#186 <ros/ros_comm#186>`_)
* fix ROS_MASTER_URI for Windows

1.9.43 (2013-03-13)
-------------------
* implement process killer for Windows
* fix exports of message filter symbols for Windows

1.9.42 (2013-03-08)
-------------------
* improve speed of message generation in dry packages
  (`#183 <ros/ros_comm#183>`_)
* fix roscpp service call deadlock
  (`#149 <ros/ros_comm#149>`_)
* fix freezing service calls when returning false
  (`#168 <ros/ros_comm#168>`_)
* fix error message publishing wrong message type
  (`#178 <ros/ros_comm#178)
* fix missing explicit dependency on pthread
  (`#135 <ros/ros_comm#135>`_)
* fix compiler warning about wrong comparison of message md5 hashes
  (`#165 <ros/ros_comm#165>`_)
* make dependencies on rospy optional by refactoring RosStreamHandler to
  rosgraph (`#179 <ros/ros_comm#179>`_)
* added option '--duration' to 'rosrun rosbag play'
  (`#121 <ros/ros_comm#121)
* add error message to rosbag when using same in and out file
  (`#171 <ros/ros_comm#171>`_)
* fix handling spaces in folder names
  (`ros/catkin#375 <ros/catkin#375>`_)
* replace custom code with Python module netifaces
  (`#130 <ros/ros_comm#130)
* make dependencies on rospy optional by refactoring RosStreamHandler to
  rosgraph (`#179 <ros/ros_comm#179>`_)
* add option --skip-log-check
  (`#133 <ros/ros_comm#133>`_)
* update API doc to list raised exceptions in config.py
* fix invocation of Python scripts under Windows
  (`#54 <ros/ros_comm#54>`_)
* fix usage of rosservice from within a launch file
* fix missing run_depend on rosbag
  (`#179 <ros/ros_comm#179>`_)

1.9.41 (2013-01-24)
-------------------
* allow sending data exceeding 2GB in chunks
  (`#4049 <https://code.ros.org/trac/ros/ticket/4049>`_)
* update getParam() doc
  (`#1460 <https://code.ros.org/trac/ros/ticket/1460>`_)
* add param::get(float)
  (`#3754 <https://code.ros.org/trac/ros/ticket/3754>`_)
* update inactive assert when publishing message with md5sum *, update related
  tests (`#3714 <https://code.ros.org/trac/ros/ticket/3714>`_)
* fix ros master retry timeout
  (`#4024 <https://code.ros.org/trac/ros/ticket/4024>`_)
* fix inactive assert when publishing message with wrong type
  (`#3714 <https://code.ros.org/trac/ros/ticket/3714>`_)
* improve performance of $(find ...)

1.9.40 (2013-01-13)
-------------------
* add colorization for rospy log output
  (`#3691 <https://code.ros.org/trac/ros/ticket/3691>`_)
* fix socket polling under Windows
  (`#3959 <https://code.ros.org/trac/ros/ticket/3959>`_)
* fix bagsort script (`#42 <ros/ros_comm#42>`_)
* fix dependent packages by pass LOG4CXX include dirs and libraries along
* fix usage of variable arguments in vFormatToBuffer() function
* add colorization for rospy log output
  (`#3691 <https://code.ros.org/trac/ros/ticket/3691>`_)
* fix 'roslaunch --pid=' when pointing to ROS_HOME but folder does not exist
  (`#43 <ros/ros_comm#43>`_)
* fix 'roslaunch --pid=' to use shell expansion for the pid value
  (`#44 <ros/ros_comm#44>`_)
* fix output of 'rossrv --help'
  (`#3979 <https://code.ros.org/trac/ros/ticket/3979>`_)
* add support for boolean in 'rostopic -p'
  (`#3948 <https://code.ros.org/trac/ros/ticket/3948>`_)
* add checks for pip packages and rosdep
* fix check for catkin_pkg
* fix for thread race condition causes incorrect graph connectivity analysis

contradict pushed a commit to contradict/ros_comm that referenced this pull request Aug 12, 2016

Aaron Blasdel
Merge pull request #239 from trainman419/rqt_bag_plot
Implement a rqt_bag plotting plugin

rsinnet pushed a commit to MisoRobotics/ros_comm that referenced this pull request Jun 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment