pcl
pcl copied to clipboard
PointCloudLibrary
Fix multiple memory corruption errors revealed by fuzzing
Fixes the following crashes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=32178 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=32228 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=33217 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=33762 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34134 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=35640 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=36663 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=37041 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=37125 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=39438 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=40058 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=40120 https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=41152
As using memcpy is not anymore recommend and you touching a lot of this lines: Can you rebase your MR to this #4299 (or wait until #4299 is merged and rebase it then) as there will be a lot of merge conflicts?
It is not that bad. https://github.com/PointCloudLibrary/pcl/pull/4299 didn't touch any memcpy in ply_io.cpp There are only 5 changes: one include, two trailing spaces removed and two memsets replaced with fills. But there you go - the merged version.
Related to https://github.com/PointCloudLibrary/pcl/pull/5333 and https://github.com/PointCloudLibrary/pcl/pull/5332 previously reviewed by @mvieth
If the idea is to keep this pull request until https://github.com/PointCloudLibrary/pcl/pull/4299 is merged, that PR was started in 2020 and is about coding style, meanwhile my PR fixes 13 unique crashes that happen because of the already existing memory corruptions.
Sorry for the delay, there were some other PRs and issues that needed attention. So I think it could make sense to add functions to PCLPointCloud2 for accessing data with bound checking. This could reduce code duplication and would also make code shorter. My idea would be something like this:
template<typename T> inline
const T& at(const pcl::uindex_t& point_index, const pcl::uindex_t& field_offset) const {
const auto position = point_index * point_step + field_offset;
if (data.size () >= (position + sizeof(T)))
return reinterpret_cast<const T&>(data[position]);
else
throw std::out_of_range("PCLPointCloud2::at");
}
template<typename T> inline
T& at(const pcl::uindex_t& point_index, const pcl::uindex_t& field_offset) {
const auto position = point_index * point_step + field_offset;
if (data.size () >= (position + sizeof(T)))
return reinterpret_cast<T&>(data[position]);
else
throw std::out_of_range("PCLPointCloud2::at");
}
These mimic std::vector::at. The reinterpret_cast replaces the memcpy needed otherwise. Both reading and writing is possible, usage would e.g. be like this: cloud.at<float>(0, y_offset) = 1.0f; (setting y coordinate of first point to 1).
@kunaltyagi @larshg @SunBlack What do you think?
Although the question wasn't for me, I think it is a great idea. The functions became shorter, more readable and no non-necessary memcpy. I went ahead and implemented your suggestion.
Looks good. @sashashura there are places where you still have memcpy, but I think you can use the new method to also assign there. Like @mvieth also wrote in the original proposal?
Ie.
memcpy (&cloud_->at<Scalar>(vertex_count_, vertex_offset_before_), &value, sizeof (Scalar));
To
cloud_->at<Scalar>(vertex_count_, vertex_offset_before_) = value;
Resolved merge conflict because of edited whitespace. Can we finally merge it?