BillboardLine::addPoint() does not throw an exception when exceeding max_points_per_line limit

[zhihaoshang]

Operating System:

Linux shangzh-VMware-Virtual-Platform 6.11.0-19-generic #19~24.04.1-Ubuntu SMP PREEMPT_DYNAMIC Mon Feb 17 11:51:52 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

ROS version or commit hash:

ros2 jazzy

RMW implementation (if applicable):

No response

RMW Configuration (if applicable):

No response

Client library (if applicable):

rviz

'ros2 doctor --report' output

ros2 doc --report

   NETWORK CONFIGURATION
inet         : 127.0.0.1
inet4        : ['127.0.0.1']
inet6        : ['::1']
netmask      : 255.0.0.0
device       : lo
flags        : 73<RUNNING,LOOPBACK,UP>
mtu          : 65536
inet         : 192.168.148.137
inet4        : ['192.168.148.137']
ether        : 00:0c:29:be:c8:19
inet6        : ['fe80::20c:29ff:febe:c819%ens33']
netmask      : 255.255.255.0
device       : ens33
flags        : 4163<MULTICAST,RUNNING,BROADCAST,UP>
mtu          : 1500
broadcast    : 192.168.148.255

   PLATFORM INFORMATION
system           : Linux
platform info    : Linux-6.11.0-19-generic-x86_64-with-glibc2.39
release          : 6.11.0-19-generic
processor        : x86_64

   QOS COMPATIBILITY LIST
compatibility status    : No publisher/subscriber pairs found

   RMW MIDDLEWARE
middleware name    : rmw_fastrtps_cpp

   TOPIC LIST
topic               : none
publisher count     : 0
subscriber count    : 0

Steps to reproduce issue

Environment

OS Version: Ubuntu 24.04
rviz version: ros2 jazzy
Compiler name and version number: Ubuntu clang version 18.1.3
Source or binary build?
source build
build options: --mixin asan-gcc

Description

In the implementation of rviz_rendering::BillboardLine, the addPoint() method does not properly handle cases where the number of added points exceeds the limit set by setMaxPointsPerLine(). The current implementation only uses assert() to check for boundary violations, which is not effective in Release builds, as assertions are typically disabled.

As a result, in production builds, the function allows adding more points than allowed, potentially leading to undefined behavior. For example, this has resulted in an AddressSanitizer out-of-memory (OOM) error during testing when a large number of points were unintentionally added.

Test Case

#include <gtest/gtest.h>
#include <gmock/gmock.h>
#include <memory>
#include <string>
#include <vector>
#include <OgreBillboardChain.h>
#include <OgreRoot.h>
#include "../ogre_testing_environment.hpp"
#include "rviz_rendering/objects/billboard_line.hpp"

using namespace ::testing;  // NOLINT

class BillboardLineTestFixture : public ::testing::Test
{
protected:
  void SetUp()
  {
    testing_environment_ = std::make_shared<rviz_rendering::OgreTestingEnvironment>();
    testing_environment_->setUpOgreTestEnvironment();
  }
  std::shared_ptr<rviz_rendering::OgreTestingEnvironment> testing_environment_;
};

static std::vector<Ogre::Vector3> squareCenteredAtZero
{
  Ogre::Vector3(1, 1, 0),
  Ogre::Vector3(1, -1, 0),
  Ogre::Vector3(-1, 1, 0),
  Ogre::Vector3(-1, -1, 0)
};

std::unique_ptr<rviz_rendering::BillboardLine> oneCellGrid()
{
  auto grid_cell = std::make_unique<rviz_rendering::BillboardLine>(
    Ogre::Root::getSingletonPtr()->createSceneManager());
  std::vector<Ogre::Vector3> pts = squareCenteredAtZero;
  grid_cell->setMaxPointsPerLine(2);
  grid_cell->setNumLines(2 * 2);
  grid_cell->addPoint(pts[0]);
  grid_cell->addPoint(pts[1]);
  grid_cell->finishLine();
  grid_cell->addPoint(pts[2]);
  grid_cell->addPoint(pts[3]);
  grid_cell->finishLine();
  grid_cell->addPoint(pts[0]);
  grid_cell->addPoint(pts[2]);
  grid_cell->finishLine();
  grid_cell->addPoint(pts[1]);
  grid_cell->addPoint(pts[3]);
  return grid_cell;
}

std::unique_ptr<rviz_rendering::BillboardLine> twoLines()
{
  auto two_lines = std::make_unique<rviz_rendering::BillboardLine>(
    Ogre::Root::getSingletonPtr()->createSceneManager());
  std::vector<Ogre::Vector3> pts = squareCenteredAtZero;
  two_lines->setMaxPointsPerLine(2);
  two_lines->setNumLines(2);
  two_lines->addPoint(pts[0]);
  two_lines->addPoint(pts[1]);
  two_lines->finishLine();
  two_lines->addPoint(pts[2]);
  two_lines->addPoint(pts[3]);
  return two_lines;
}

TEST_F(BillboardLineTestFixture, addPoint_checks_for_max_points_per_line_boundary) {
  rviz_rendering::BillboardLine grid_cell(Ogre::Root::getSingletonPtr()->createSceneManager());
  grid_cell.setMaxPointsPerLine(2);
  grid_cell.addPoint(Ogre::Vector3(0, 0, 0));
  grid_cell.addPoint(Ogre::Vector3(1, 1, 0));
  EXPECT_THROW(grid_cell.addPoint(Ogre::Vector3(2, 2, 0)), std::out_of_range);
}

Output

Running main() from gmock_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from BillboardLineTestFixture
[ RUN      ] BillboardLineTestFixture.addPoint_checks_for_max_points_per_line_boundary
[rviz_rendering:debug] Available Renderers(1): OpenGL Rendering Subsystem, at /home/shangzh/ros2_jazzy/src/ros2/rviz/rviz_rendering/src/rviz_rendering/render_system.cpp:289
[rviz_rendering:info] Stereo is NOT SUPPORTED, at /home/shangzh/ros2_jazzy/src/ros2/rviz/rviz_rendering/src/rviz_rendering/render_system.cpp:531
[rviz_rendering:info] OpenGl version: 4.3 (GLSL 4.3), at /home/shangzh/ros2_jazzy/src/ros2/rviz/rviz_rendering/src/rviz_rendering/render_system.cpp:272
/home/shangzh/ros2_jazzy/src/ros2/rviz/rviz_rendering/test/rviz_rendering/objects/billboard_line_test.cpp:73: Failure
Expected: grid_cell.addPoint(Ogre::Vector3(2, 2, 0)) throws an exception of type std::out_of_range.
  Actual: it throws nothing.

[  FAILED  ] BillboardLineTestFixture.addPoint_checks_for_max_points_per_line_boundary (1887 ms)
[----------] 1 test from BillboardLineTestFixture (1887 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (1887 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] BillboardLineTestFixture.addPoint_checks_for_max_points_per_line_boundary

 1 FAILED TEST

Expected behavior

When the number of added points exceeds the max_points_per_line limit, the addPoint() method should throw a std::out_of_range (or similar) exception to clearly indicate a boundary violation. This ensures proper usage of the API and helps maintain system stability at runtime.

Actual behavior

The current implementation relies solely on assert() for boundary checking. In Release mode, these assertions are removed by the compiler, so the check is bypassed, allowing unchecked addition of elements. This can lead to memory overuse, crashes, or undefined behavior.

This behavior also causes the unit test addPoint_checks_for_max_points_per_line_boundary to fail, as the test expects a std::out_of_range exception, but none is thrown.

Additional information

Suggested Fix

if (chain_containers_[current_chain_container_]->getNumChainElements(current_line_ % chains_per_container_) >= max_points_per_line_) {
  throw std::out_of_range("Exceeded max_points_per_line limit.");
}