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.

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

Fix out of bounds for heap-array in statistical outlier filter #5502

Merged
merged 1 commit into from
Nov 1, 2022

Conversation

simonlynen
Copy link
Contributor

The arrays need to hold one more item, since we want to skip over the query point in the results

The arrays need to hold one more item, since we want to skip over the query point in the results
@mvieth
Copy link
Member

mvieth commented Oct 31, 2022

Hi, thanks for this pull request. The changes look good, but I would be interested to know: did this actually cause a problem for you, i.e. did you notice an out-of-bounds access? To my knowledge, all search methods resize the indices and dists arrays anyway (no-op if they already have the correct size), so after the first call to nearestKSearch they would have length mean_k_ + 1.

@simonlynen
Copy link
Contributor Author

simonlynen commented Oct 31, 2022

Hi Markus,

I agree that often the knn-do resizing; but in this case the "nearestKSearch" API suggests that the output vectors must be resized to match k.

This triggered an asan error when we added this filtering the other day; here is the beginning of the asan output.

==856==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200030af1c at pc 0x7f45bddc6523 bp 0x7ffca37b6490 sp 0x7ffca37b6488
READ of size 4 at 0x60200030af1c thread T0
    #0 0x7f45bddc6522 in pcl::StatisticalOutlierRemoval<pcl::PointXYZL>::applyFilterIndices(std::__u::vector<int, std::__u::allocator<int>>&) pcl/filters/include/pcl/filters/impl/statistical_outlier_removal.hpp:116:25

@simonlynen
Copy link
Contributor Author

The OOB refers to the nn_dists vector:

0x60200030af1c is located 0 bytes after 12-byte region [0x60200030af10,0x60200030af1c)
allocated by thread T0 here:
    #0 0x561d9fc6e7fd in operator new(unsigned long)
  (snip)
    #7 0x7f45bddc61cf in pcl::StatisticalOutlierRemoval<pcl::PointXYZL>::applyFilterIndices(std::__u::vector<int, std::__u::allocator<int>>&) pcl/filters/include/pcl/filters/impl/statistical_outlier_removal.hpp:87:22

@larshg
Copy link
Contributor

larshg commented Nov 1, 2022

I guess #5092 would solve this as well?

@mvieth
Copy link
Member

mvieth commented Nov 1, 2022

I guess #5092 would solve this as well?

Maybe, but I think that needs some more discussion, so I would prefer to merge the fix from this pull request already.

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

Thank you @simonlynen !

@simonlynen
Copy link
Contributor Author

Thanks for the review! Looks like I'm not authorized to merge PRs onto master, can you please do that for me?

@mvieth
Copy link
Member

mvieth commented Nov 1, 2022

Thanks for the review! Looks like I'm not authorized to merge PRs onto master, can you please do that for me?

I will do that, but I wanted to wait for @larshg 's reply first

@mvieth mvieth merged commit 9502237 into PointCloudLibrary:master Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants