Fix potencial leak / seg fault #726
Conversation
|
rviz_default_plugins/src/rviz_default_plugins/displays/image/ros_image_texture.cpp
Outdated
Show resolved
Hide resolved
|
friendly ping @clalancette |
There was a problem hiding this comment.
I've left some comments inline. One other thing that I noticed in this piece of code is that it isn't clear to me what mutex_ is protecting. Is it just the image_ and new_image_? Is it all of the variables in the class? What can run on different threads here? I don't know the answer to all of that, but I think it is worth investigating to see if we need to fix up locking here.
| if (!bufferptr_) { | ||
| bufferptr_ = std::make_shared<std::vector<uint8_t>>(image_data.size_); | ||
| } else if (static_cast<size_t>(bufferptr_->size()) != image_data.size_) { | ||
| bufferptr_->resize(image_data.size_, 0); | ||
| } |
There was a problem hiding this comment.
A few different things about this code:
- I feel like we can simplify this idiom by allocating at least one byte to
bufferptr_in the constructor. Then all of the places we do this, we can change this to:
if (bufferptr_->size() != image_data.size_) {
bufferptr_->resize(image_data.size_, 0);
}
- I don't think we need the static_cast to size_t; both
bufferptr_->size()andimage_data.size_aresize_t. - Technically, we only need to resize the buffer if it is smaller than what we currently need. That could save some reallocation, at the expense of some wasted memory. I don't feel strongly about this point, though.
- I can't quite put my finger on it, but the resize logic looks incorrect. In particular, just prior to this code, we set
image_data.size_to the number of uint16_t elements in the incoming piece of data. But then we allocate that number of uint8_t bytes, which seems like we wouldn't have enough data in the bufferptr. For example, assume that we have 10 uint16_t elements. In that case, we'll allocate 10 bytes to the bufferptr, not 20. It feels like we can overrun this when we are normalizing, but I'm not sure.
There was a problem hiding this comment.
I think these two lines are calculating the uint_8 size of the buffer.
image_data.size_ /= sizeof(uint16_t);
...
image_data.size_ /= sizeof(float);
There was a problem hiding this comment.
I can't see the reason to divide here. From looking at the origin of the data, we'd just want to allocate image_data.size_? I believe it's already the number of bytes not the number of elements?
There was a problem hiding this comment.
We need to do this because we are creating an image to show in RVIZ which is a uint8_t. We need to know the number of elements for each one of the formats.
Take a look to the normalize() function.
rviz_default_plugins/src/rviz_default_plugins/displays/image/ros_image_texture.cpp
Outdated
Show resolved
Hide resolved
|
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2021-9-16/22372/1 |
|
friendly ping @clalancette |
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
Signed-off-by: ahcorde <ahcorde@gmail.com>
The main goal here is to stop using a pointer from a freed std::vector. Instead, we allocate memory on the heap and delete it when this piece of code no longer needs it (Ogre copies it internally). Along the way, we do a huge cleanup of this code to make it easier to understand and fix some obvious problems with it: 1. Improve the locking. 2. Make some methods const. 3. Move implementations in the C++ file. 4. Remove unnecessary functions. 5. Rename a few things to make their function clearer. Signed-off-by: Chris Lalancette <clalancette@gmail.com>
clalancette
force-pushed
the
ahcorde/improve_16_32_image
branch
from
a8c4153
to
6410bfc
Compare
|
@ahcorde I know this one has been sitting a long time. Sorry about that. I ended up taking some time to look at this in more detail, and I ended up with a much larger rewrite here. However, I believe that this rewrite takes things in the correct direction. In particular, what we do here is to only store copied data into the ImageData as needed, and only for as long as needed. That is enabled by the fact that Ogre::Image::load_raw_data() actually copies the data into itself. Thus, we only need to keep that data around long enough for the function to be called, and we can then free it. Further, we recognize that we only sometimes need to do a copy (really only when doing a conversion); otherwise, we can point to the data already stored in the message. Beyond that, I did a bunch of other cleanup to make this a lot easier to understand. I now think that this is in good shape, but I'd like you to take a look and let me know what you think about it. |
|
Signed-off-by: Chris Lalancette <clalancette@gmail.com>
|
There was a warning on Windows (which showed a real bug). That is now fixed. Here is new CI:
|
Signed-off-by: ahcorde ahcorde@gmail.com
This PR is a follow up of this other PR #701