PointCloudLibrary/pcl

Kinfu MarchingCubes writes unbounded data to hardcode sized buffer

Open

#719 opened on May 31, 2014

View on GitHub
 (12 comments) (0 reactions) (1 assignee)C++ (9,023 stars) (4,506 forks)batch import
help wantedkind: bugmodule: gpuskill: cudastatus: stale

Description

pcl::gpu::MarchingCubes::run:

  if (triangles_buffer.empty())
    triangles_buffer.create(DEFAULT_TRIANGLES_BUFFER_SIZE);

DEFAULT_TRIANGLES_BUFFER_SIZE = 2 * 1000 * 1000 * POINTS_PER_TRIANGLE

marching_cubes.cu/store_point():

triangles_buffer[index] = ...; // index is dependent on how many voxels we have

I suggest changing DEFAULT_TRIANGLES_BUFFER_SIZE to WHY_WOULD_ANYONE_EVER_HAVE_MORE_TRIANGLES, that will make the out of bounds crash on the GPU much easier to find.

I also like how it starts with 2 * - that looks almost like somebody thought "hmm for some reason the code works if I make this bigger".

Contributor guide

Tech stack
cpp
Domain
backendperformance
Issue type
bug
DifficultyEstimated implementation difficulty for a new contributor, from 1 for very small changes to 5 for expert-level work.
3
Estimated timeA rough time range for an experienced contributor to investigate, implement, test, and prepare a pull request.
half day
Activity statusHow available the issue appears right now: fresh, active, stale, blocked, or waiting on maintainer input.
blocked
ClarityHow clearly the issue explains the expected change, acceptance criteria, and next step.
clear
Prerequisites
C++CUDA
Newbie friendlinessA 1-100 score estimating how approachable this issue is for first-time contributors.
25
Research direction
Investigate the MarchingCubes algorithm in PCL's GPU module, specifically the `store point` function in `marching cubes.cu`. Understand how the triangle index is computed and compare it to the fixed buffer size defined by `DEFAULT TRIANGLES BUFFER SIZE`. Check the issue comments for any proposed fixes or discussions. Propose a solution to either dynamically allocate the buffer or compute the correct size based on input parameters.