kzampog / cilantro

A lean C++ library for working with point cloud data
MIT License
1.03k stars 208 forks source link

Peculiar memory error #32

Open wannesvanloock opened 5 years ago

wannesvanloock commented 5 years ago

I encountered strange behavior related to Eigen and nanoflann in cilantro. Consider the following example.

#include <cilantro/kd_tree.hpp>
#include <cilantro/io.hpp>
#include <iostream>

cilantro::NeighborSet<float> foobar(const Eigen::Matrix3Xf& points) {
  cilantro::KDTree3f tree(points);
  return tree.radiusSearch(Eigen::Vector3f(0.1, 0.1, 0.4), 1.001);
}

struct Foo {
  Foo(const Eigen::Matrix3Xf& points) : tree(points) {}

  cilantro::NeighborSet<float> bar() {
    return tree.radiusSearch(Eigen::Vector3f(0.1, 0.1, 0.4), 1.001);
  }

  cilantro::KDTree3f tree;
};

int main(int argc, char** argv) {
  Eigen::Matrix3Xf points(3, 8);
  points << 0, 1, 0, 0, 0, 1, 1, 1,
            0, 0, 1, 0, 1, 0, 1, 1,
            0, 0, 0, 1, 1, 1, 0, 1;

  cilantro::NeighborSet<float> nn;
  { // Causes invalid read
    Foo foo(points.colwise() - Eigen::Vector3f(0.5, 0.5, 0.5));
    nn = foo.bar();
  }
  { // no issues
    Eigen::Matrix3Xf centered =
        points.colwise() - Eigen::Vector3f(0.5, 0.5, 0.5);
    Foo foo(centered);
    nn = foo.bar();
  }
  { // no issues
    nn = foobar(points.colwise() - Eigen::Vector3f(0.5, 0.5, 0.5));
  }
  return 0;
}

Instantiating Foo with a temporary as in the first call leads to invalid reads that trace back to evalMetric in nanoflann. The invalid read does not show up in the second instantiation of Foo. Similarly the call to foobar does not cause an issue.

Seems more related to third party libs, but it would be great if we could catch these subtleties somehow. Lost quite some time over this one.

wannesvanloock commented 5 years ago

I think I understand what might be the issue. cilantro::KDTreeDataAdaptors takes an Eigen::Map and remaps the memory to a new matrix. Passing it as a temporary to the constructor means the temporary will be destroyed after the call to the constructor. Hence, the Eigen::Map is now pointing to unallocated memory. Tricky...

kzampog commented 5 years ago

Yes, KDTree needs the point set to be 'alive'. Maybe we could just disable building from temporaries or use some Eigen::Ref-like mechanism to keep them alive? Any suggestions are welcome! :D