Changed memcpy/memset for non-POD objects [PCL-3368] #4299

gnawme · 2020-07-30T23:47:24Z

memcpy and memcmp violate the type system. Using memcpy to copy objects is like making money using a photocopier. Using memcmp to compare objects is like comparing leopards by counting their spots. The tools and methods might appear to do the job, but they are too coarse to do it acceptably.

memset is also considered harmful.

For this PR:

For the most part, memset was replaced by std::fill_n or, in many cases, by zero initialization
There are many, many instances of memcpy in PCL that are used to copy various kinds of Point data as uint8_t to float; I left these alone.
Otherwise, where possible, memcpy was replaced by std::copy or std::copy_n
The only instances of memcmp (outside of third-party code) were in CUDA code; I left those alone.

#3368

kunaltyagi

Thanks for taking the initiative 😄

kunaltyagi · 2020-07-31T15:00:51Z

common/include/pcl/common/impl/io.hpp

-  if (isSamePointType<PointInT, PointOutT> ())
-    // Copy the whole memory block
-    memcpy (&cloud_out[0], &cloud_in[0], cloud_in.size () * sizeof (PointInT));
+#if (__cplusplus == 201703L)


Please use PCL_MACROS for a PCL_CONSTEXPR like solution.

Moreover, why did you stick with memcpy in non C++17 branch?

This relates to the issue I mentioned where compilation failed due to trying to use std::copy on two different Point objects, despite the isSamePoint test

Ofc. Silly of me. Please let this issue still remain unresolved till I get to think over it a bit

visualization/src/vtk/pcl_context_item.cpp

kunaltyagi

9/98

common/include/pcl/common/impl/io.hpp

common/include/pcl/common/io.h

common/include/pcl/conversions.h

common/include/pcl/pcl_macros.h

common/include/pcl/range_image/impl/range_image.hpp

kunaltyagi

15/100

common/include/pcl/common/io.h

common/include/pcl/conversions.h

common/include/pcl/range_image/impl/range_image.hpp

common/src/io.cpp

kunaltyagi

34/110

io/src/lzf.cpp

io/src/lzf_image_io.cpp

recognition/include/pcl/recognition/ransac_based/bvh.h

kunaltyagi · 2020-08-02T08:45:28Z

PS: Builds are failing (Ignore MSVC for a while)

gnawme · 2020-08-02T18:25:33Z

PS: Builds are failing (Ignore MSVC for a while)

Yes, unit tests are failing in CovarianceSampling.Filters. So far, I'm only able to resolve it to something in Eigen.

filters/include/pcl/filters/impl/conditional_removal.hpp

kunaltyagi

43/110

gpu/people/src/cuda/nvidia/NPP_staging.cu

io/src/ifs_io.cpp

io/src/image_depth.cpp

io/src/pcd_grabber.cpp

kunaltyagi

56/110

visualization/include/pcl/visualization/impl/point_cloud_color_handlers.hpp

common/src/io.cpp

features/include/pcl/features/impl/integral_image_normal.hpp

filters/include/pcl/filters/impl/covariance_sampling.hpp

gpu/people/src/cuda/nvidia/NPP_staging.cu

kunaltyagi

67/110

filters/include/pcl/filters/impl/covariance_sampling.hpp

gpu/people/src/cuda/nvidia/NPP_staging.cu

io/include/pcl/compression/impl/entropy_range_coder.hpp

io/src/lzf_image_io.cpp

io/src/pcd_grabber.cpp

ml/src/svm.cpp

kunaltyagi

76/110

surface/include/pcl/surface/impl/convex_hull.hpp

surface/include/pcl/surface/3rdparty/poisson4/mat.hpp

kunaltyagi

86/110

io/src/pcd_io.cpp

io/src/ply_io.cpp

keypoints/include/pcl/keypoints/impl/iss_3d.hpp

octree/include/pcl/octree/octree_key.h

outofcore/src/cJSON.cpp

surface/include/pcl/surface/impl/convex_hull.hpp

ml/include/pcl/ml/permutohedral.h

outofcore/src/cJSON.cpp

kunaltyagi

94/110

outofcore/src/cJSON.cpp

gpu/people/src/cuda/nvidia/NPP_staging.cu

surface/include/pcl/surface/impl/convex_hull.hpp

segmentation/include/pcl/segmentation/impl/sac_segmentation.hpp

segmentation/include/pcl/segmentation/impl/unary_classifier.hpp

sample_consensus/include/pcl/sample_consensus/sac_model.h

recognition/src/linemod.cpp

kunaltyagi

102/110 files are good to go

surface/include/pcl/surface/impl/convex_hull.hpp

kunaltyagi · 2020-08-14T18:02:01Z

segmentation/include/pcl/segmentation/impl/unary_classifier.hpp

@@ -309,8 +307,6 @@ pcl::UnaryClassifier<PointT>::queryFeatureDistances (std::vector<pcl::PointCloud
  {
    // Query point  
    flann::Matrix<float> p = flann::Matrix<float>(new float[n_col], 1, n_col);
-    memcpy (&p.ptr ()[0], (*query_features)[i].histogram, p.cols * p.rows * sizeof (float));


This line should not be removed (I think).

None of the other flann::Matrix variables are initialized; does knnSearch return garbage if it's not?

🤷‍♂️ I don't know. Will take a deeper look and get back to you

This definitely has to be initialized since its an input for knnSearch (indices and distances are output).
std::copy((*query_features)[i].histogram, (*query_features)[i].histogram + n_col, p.ptr()) should work (1*n_col is the same as p.cols*p.rows)

io/src/pcd_io.cpp

outofcore/src/cJSON.cpp

io/src/ply_io.cpp

outofcore/src/cJSON.cpp

kunaltyagi

3 files left

io/src/lzf_image_io.cpp

SunBlack

Maybe could make the std::array chnges also in another MR - just saw in this MR, that we could modernize them here.

SunBlack · 2022-07-21T12:58:50Z

keypoints/include/pcl/keypoints/impl/iss_3d.hpp

@@ -186,15 +185,13 @@ pcl::ISSKeypoint3D<PointInT, PointOutT, NormalT>::getScatterMatrix (const int& c
  if (n_neighbors < min_neighbors_)
    return;

-  double cov[9];
-  memset(cov, 0, sizeof(double) * 9);
+  double cov[9]{};


Suggested change

double cov[9]{};

std::array<double, 9> cov{};

SunBlack · 2022-07-21T13:00:24Z

keypoints/include/pcl/keypoints/impl/iss_3d.hpp

@@ -166,8 +166,7 @@ pcl::ISSKeypoint3D<PointInT, PointOutT, NormalT>::getScatterMatrix (const int& c
 {
  const PointInT& current_point = (*input_)[current_index];

-  double central_point[3];
-  memset(central_point, 0, sizeof(double) * 3);
+  double central_point[3]{};


Suggested change

double central_point[3]{};

std::array<double, 3> central_point{};

SunBlack · 2022-07-21T13:02:48Z

keypoints/include/pcl/keypoints/impl/iss_3d.hpp


  for (const auto& n_idx : nn_indices)
  {
    const PointInT& n_point = (*input_)[n_idx];

-    double neigh_point[3];
-    memset(neigh_point, 0, sizeof(double) * 3);
+    double neigh_point[3]{};


Suggested change

double neigh_point[3]{};

std::array<double, 3> neigh_point{};

SunBlack · 2022-07-21T13:03:50Z

keypoints/include/pcl/keypoints/impl/iss_3d.hpp

@@ -252,8 +249,7 @@ pcl::ISSKeypoint3D<PointInT, PointOutT, NormalT>::initCompute ()

    delete[] third_eigen_value_;

-  third_eigen_value_ = new double[input_->size ()];
-  memset(third_eigen_value_, 0, sizeof(double) * input_->size ());
+  third_eigen_value_ = new double[input_->size ()]{};


Not sure about the performance, but couldn't you use here std::vector? Same below

SunBlack · 2022-07-21T13:04:26Z

recognition/include/pcl/recognition/color_modality.h

@@ -185,8 +185,7 @@ void pcl::ColorModality<PointInT>::extractFeatures (const MaskMap & mask,
  for (std::size_t map_index = 0; map_index < 8; ++map_index)
    mask_maps[map_index].resize (width, height);

-  unsigned char map[255];
-  memset(map, 0, 255);
+  unsigned char map[255]{};


Suggested change

unsigned char map[255]{};

std::array<unsigned char, 255> map{};

SunBlack · 2022-07-21T13:05:14Z

recognition/include/pcl/recognition/surface_normal_modality.h

@@ -931,8 +930,7 @@ pcl::SurfaceNormalModality<PointInT>::computeAndQuantizeSurfaceNormals2 ()
  }
  /*cvSmooth(m_dep[0], m_dep[0], CV_MEDIAN, 5, 5);*/

-  unsigned char map[255];
-  memset(map, 0, 255);
+  unsigned char map[255]{};


Suggested change

unsigned char map[255]{};

std::array<unsigned char, 255> map{};

SunBlack · 2022-07-21T13:05:21Z

recognition/include/pcl/recognition/surface_normal_modality.h

@@ -981,8 +979,7 @@ pcl::SurfaceNormalModality<PointInT>::extractFeatures (const MaskMap & mask,
  for (auto &mask_map : mask_maps)
    mask_map.resize (width, height);

-  unsigned char map[255];
-  memset(map, 0, 255);
+  unsigned char map[255]{};


Suggested change

unsigned char map[255]{};

std::array<unsigned char, 255> map{};

SunBlack · 2022-07-21T13:05:26Z

recognition/include/pcl/recognition/surface_normal_modality.h

@@ -1252,8 +1249,7 @@ pcl::SurfaceNormalModality<PointInT>::extractAllFeatures (
  for (auto &mask_map : mask_maps)
    mask_map.resize (width, height);

-  unsigned char map[255];
-  memset(map, 0, 255);
+  unsigned char map[255]{};


Suggested change

unsigned char map[255]{};

std::array<unsigned char, 255> map{};

SunBlack · 2022-07-21T13:07:00Z

tools/png2pcd.cpp

@@ -226,8 +226,7 @@ main (int argc, char** argv)
  }

  // Retrieve the entries from the image data and copy them into the output RGB cloud
-  double* pixel = new double [4];
-  memset (pixel, 0, sizeof (double) * 4);
+  double* pixel = new double [4]{0.0};


Suggested change

double* pixel = new double [4]{0.0};

std::array<double, 4> pixel{};

SunBlack · 2022-07-21T13:09:54Z

test/features/test_rift_estimation.cpp

-                            0.0147f, 0.1222f, 0.3568f, 0.4348f, 0.0149f, 0.0806f, 0.2787f, 0.6864f};
-    std::copy_n (data, 32, correct_rift_feature_values);
-  }
+  const float correct_rift_feature_values[32] =


Suggested change

const float correct_rift_feature_values[32] =

std::array<float, 32> correct_rift_feature_values{};

On this way you can use a for-ranges loop below.

segmentation/include/pcl/segmentation/impl/region_growing.hpp

apps/point_cloud_editor/src/common.cpp

mvieth · 2022-07-26T13:30:32Z

Maybe could make the std::array chnges also in another MR - just saw in this MR, that we could modernize them here.

I would prefer to not make those changes here since this is already quite big and dragging on for two years 🙂

mvieth · 2022-07-28T07:50:39Z

I still found instances of std::copy changed to std::copy_n in

And pcl:: removed in

Please check again and undo.

common/src/projection_matrix.cpp

common/include/pcl/conversions.h

ml/include/pcl/ml/permutohedral.h

gnawme · 2022-07-28T20:47:44Z

I still found instances of std::copy changed to std::copy_n

Sorry, I didn't realize that I had a filter set on my Find in Files...

common/src/projection_matrix.cpp

segmentation/include/pcl/segmentation/impl/region_growing.hpp

registration/include/pcl/registration/distances.h

mvieth

I found some more instances where e.g. floats are copied to or from a data blob (vector of std::uint8_t). I would prefer to keep memcpy for such cases (makes no sense to see it as "copy four individual and independent std::uint8_t"):

apps/src/grabcut_2d.cpp

mvieth

About 20 files left to review

keypoints/include/pcl/keypoints/impl/iss_3d.hpp

mvieth · 2022-08-03T08:49:27Z

segmentation/include/pcl/segmentation/impl/unary_classifier.hpp

@@ -309,8 +307,6 @@ pcl::UnaryClassifier<PointT>::queryFeatureDistances (std::vector<pcl::PointCloud
  {
    // Query point  
    flann::Matrix<float> p = flann::Matrix<float>(new float[n_col], 1, n_col);
-    memcpy (&p.ptr ()[0], (*query_features)[i].histogram, p.cols * p.rows * sizeof (float));


This definitely has to be initialized since its an input for knnSearch (indices and distances are output).
std::copy((*query_features)[i].histogram, (*query_features)[i].histogram + n_col, p.ptr()) should work (1*n_col is the same as p.cols*p.rows)

mvieth

I found a few more places (mostly in the io module) where bytes/data blobs are copied and IMO it doesn't make much sense to replace memcpy with copy (in ply_io.cpp: memset/fill_n). Please check and undo if you agree:

recognition/src/linemod.cpp

gnawme · 2022-08-05T19:05:18Z

Please check and undo if you agree

I retained std::copy if it was like-type to like-type, otherwise I reverted

io/src/ply_io.cpp

common/include/pcl/conversions.h

io/src/hdl_grabber.cpp

mvieth

Ready to merge from my side! Thank you for your endurance 😄
I will wait a few more days and then merge if nothing has come up until then

gnawme force-pushed the norm.evangelista/PCL-3368-change-memcpy branch from 3bfb1d7 to 97061fc Compare July 31, 2020 01:08

kunaltyagi requested changes Jul 31, 2020

View reviewed changes

kunaltyagi reviewed Aug 1, 2020

View reviewed changes

common/include/pcl/common/io.h Show resolved Hide resolved

common/include/pcl/conversions.h Outdated Show resolved Hide resolved

common/include/pcl/range_image/impl/range_image.hpp Outdated Show resolved Hide resolved

common/src/io.cpp Outdated Show resolved Hide resolved

kunaltyagi reviewed Aug 2, 2020

View reviewed changes

io/src/lzf.cpp Show resolved Hide resolved

io/src/lzf_image_io.cpp Outdated Show resolved Hide resolved

recognition/include/pcl/recognition/ransac_based/bvh.h Outdated Show resolved Hide resolved

kunaltyagi reviewed Aug 2, 2020

View reviewed changes

filters/include/pcl/filters/impl/conditional_removal.hpp Outdated Show resolved Hide resolved

kunaltyagi reviewed Aug 3, 2020

View reviewed changes

gpu/people/src/cuda/nvidia/NPP_staging.cu Outdated Show resolved Hide resolved

io/src/ifs_io.cpp Outdated Show resolved Hide resolved

io/src/image_depth.cpp Outdated Show resolved Hide resolved

io/src/pcd_grabber.cpp Outdated Show resolved Hide resolved

kunaltyagi reviewed Aug 4, 2020

View reviewed changes

visualization/include/pcl/visualization/impl/point_cloud_color_handlers.hpp Show resolved Hide resolved

kunaltyagi reviewed Aug 4, 2020

View reviewed changes

kunaltyagi reviewed Aug 5, 2020

View reviewed changes

surface/include/pcl/surface/impl/convex_hull.hpp Outdated Show resolved Hide resolved

surface/include/pcl/surface/impl/convex_hull.hpp Outdated Show resolved Hide resolved

surface/include/pcl/surface/3rdparty/poisson4/mat.hpp Outdated Show resolved Hide resolved

kunaltyagi reviewed Aug 5, 2020

View reviewed changes

ml/include/pcl/ml/permutohedral.h Show resolved Hide resolved

outofcore/src/cJSON.cpp Show resolved Hide resolved

kunaltyagi self-requested a review August 6, 2020 02:54

gnawme force-pushed the norm.evangelista/PCL-3368-change-memcpy branch from 63472c7 to 5f1b5a4 Compare August 6, 2020 14:55

kunaltyagi reviewed Aug 6, 2020

View reviewed changes

gnawme force-pushed the norm.evangelista/PCL-3368-change-memcpy branch 3 times, most recently from 15da3fd to 4898c95 Compare August 13, 2020 00:02

kunaltyagi reviewed Aug 13, 2020

View reviewed changes

kunaltyagi reviewed Aug 14, 2020

View reviewed changes

kunaltyagi reviewed Aug 15, 2020

View reviewed changes

outofcore/src/cJSON.cpp Outdated Show resolved Hide resolved

kunaltyagi reviewed Aug 15, 2020

View reviewed changes

io/src/ply_io.cpp Outdated Show resolved Hide resolved

kunaltyagi reviewed Aug 15, 2020

View reviewed changes

io/src/ply_io.cpp Outdated Show resolved Hide resolved

kunaltyagi reviewed Aug 15, 2020

View reviewed changes

io/src/ply_io.cpp Outdated Show resolved Hide resolved

kunaltyagi reviewed Aug 15, 2020

View reviewed changes

outofcore/src/cJSON.cpp Outdated Show resolved Hide resolved

kunaltyagi reviewed Aug 19, 2020

View reviewed changes

io/src/lzf_image_io.cpp Outdated Show resolved Hide resolved

gnawme force-pushed the norm.evangelista/PCL-3368-change-memcpy branch from 1fbddd5 to e1b8278 Compare August 22, 2020 05:09

Addressed review comments [PCL-3368]

ed10e76

SunBlack reviewed Jul 21, 2022

View reviewed changes

SunBlack mentioned this pull request Jul 22, 2022

Fix multiple memory corruption errors revealed by fuzzing #5342

Merged

sashashura added a commit to sashashura/pcl that referenced this pull request Jul 22, 2022

Merge with ply_io.cpp from PointCloudLibrary#4299

4282c7f

mvieth reviewed Jul 26, 2022

View reviewed changes

segmentation/include/pcl/segmentation/impl/region_growing.hpp Outdated Show resolved Hide resolved

mvieth reviewed Jul 26, 2022

View reviewed changes

apps/point_cloud_editor/src/common.cpp Show resolved Hide resolved

Reverted std::copy_n to std::copy per review [PCL-3368]

f97b311

mvieth reviewed Jul 28, 2022

View reviewed changes

common/src/projection_matrix.cpp Outdated Show resolved Hide resolved

common/include/pcl/conversions.h Show resolved Hide resolved

ml/include/pcl/ml/permutohedral.h Show resolved Hide resolved

More reversions per review [PCL-3368]

20c38d5

Fixed more std::copy_n escapes [PCL-3368]

f3e0de2

mvieth reviewed Aug 2, 2022

View reviewed changes

common/src/projection_matrix.cpp Outdated Show resolved Hide resolved

segmentation/include/pcl/segmentation/impl/region_growing.hpp Outdated Show resolved Hide resolved

registration/include/pcl/registration/distances.h Show resolved Hide resolved

mvieth reviewed Aug 2, 2022

View reviewed changes

apps/src/grabcut_2d.cpp Outdated Show resolved Hide resolved

Reversions and corrections per review [PCL-3368]

a64ee9a

mvieth reviewed Aug 3, 2022

View reviewed changes

Reverted unnecessary zero-init, added missing init [PCL-3368]

cbf533e

mvieth reviewed Aug 5, 2022

View reviewed changes

recognition/src/linemod.cpp Outdated Show resolved Hide resolved

More reversions and corrections per review [PCL-3368]

f52b182

mvieth reviewed Aug 7, 2022

View reviewed changes

io/src/ply_io.cpp Outdated Show resolved Hide resolved

common/include/pcl/conversions.h Outdated Show resolved Hide resolved

io/src/hdl_grabber.cpp Show resolved Hide resolved

A few more reversions per review [PCL-3368]

d2766fd

mvieth approved these changes Aug 9, 2022

View reviewed changes

mvieth added changelog: enhancement Meta-information for changelog generation and removed needs: more work Specify why not closed/merged yet labels Aug 16, 2022

mvieth merged commit fa3da0b into PointCloudLibrary:master Aug 16, 2022

gnawme mentioned this pull request Sep 18, 2022

Modernized memset via uniform init or modern constructs #5436

Merged

	double central_point[3]{};
	std::array<double, 3> central_point{};

	double neigh_point[3]{};
	std::array<double, 3> neigh_point{};

	unsigned char map[255]{};
	std::array<unsigned char, 255> map{};

	double* pixel = new double [4]{0.0};
	std::array<double, 4> pixel{};

	const float correct_rift_feature_values[32] =
	std::array<float, 32> correct_rift_feature_values{};

Changed memcpy/memset for non-POD objects [PCL-3368] #4299

Changed memcpy/memset for non-POD objects [PCL-3368] #4299

Conversation

gnawme commented Jul 30, 2020 • edited Loading

kunaltyagi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi commented Aug 2, 2020

gnawme commented Aug 2, 2020

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunaltyagi left a comment

Choose a reason for hiding this comment

SunBlack left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvieth commented Jul 26, 2022

mvieth commented Jul 28, 2022

gnawme commented Jul 28, 2022

mvieth left a comment

Choose a reason for hiding this comment

mvieth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvieth left a comment

Choose a reason for hiding this comment

gnawme commented Aug 5, 2022

mvieth left a comment

Choose a reason for hiding this comment

gnawme commented Jul 30, 2020 •

edited

Loading