From feef6f74e701ea68540ca306628888c1cf4f01bd Mon Sep 17 00:00:00 2001
From: =?utf8?q?J=C3=A9r=C3=A9mie=20Galarneau?=
 <jeremie.galarneau@efficios.com>
Date: Wed, 7 Jun 2023 16:28:27 -0400
Subject: [PATCH] Fix: coverity warns of uncaught exception
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit

Coverity warns that some container operations used by
random_access_container_wrapper can throw even though methods are marked
as noexcept.

  CID 1512893 (#1-2 of 2): Uncaught exception (UNCAUGHT_EXCEPT)
  exn_spec_violation: An exception of type lttng::invalid_argument_error is thrown but the exception specification noexcept doesn't allow it to be thrown. This will result in a call to terminate().

The noexcept specifier is remvoved from operator* and end() of
random_access_container_wrapper's iterator implementation.

To make this a bit clearer, a bounds check is performed in operator[]
directly which will make errors easier to catch.

Reported-by: Coverity Scan
Signed-off-by: Jérémie Galarneau <jeremie.galarneau@efficios.com>
Change-Id: I31d51e8709d33b3c80d64c8c05a23e519e3a93e7
---
 src/common/container-wrapper.hpp | 34 ++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/src/common/container-wrapper.hpp b/src/common/container-wrapper.hpp
index bccb23dda..5ee20dfea 100644
--- a/src/common/container-wrapper.hpp
+++ b/src/common/container-wrapper.hpp
@@ -8,6 +8,8 @@
 #ifndef LTTNG_CONTAINER_WRAPPER_H
 #define LTTNG_CONTAINER_WRAPPER_H
 
+#include <common/exception.hpp>
+#include <common/format.hpp>
 #include <common/macros.hpp>
 
 #include <cstddef>
@@ -71,7 +73,7 @@ class random_access_container_wrapper {
 		typename std::conditional<std::is_pointer<IteratorElementType>::value,
 					  IteratorElementType,
 					  IteratorElementType&>::type
-		operator*() const noexcept
+		operator*() const
 		{
 			return _container[_index];
 		}
@@ -95,7 +97,7 @@ public:
 		return iterator(*this);
 	}
 
-	iterator end() noexcept
+	iterator end()
 	{
 		return iterator(*this, ContainerOperations::size(_container));
 	}
@@ -105,7 +107,7 @@ public:
 		return const_iterator(*this);
 	}
 
-	const_iterator end() const noexcept
+	const_iterator end() const
 	{
 		return const_iterator(*this, ContainerOperations::size(_container));
 	}
@@ -118,8 +120,22 @@ public:
 	typename std::conditional<std::is_pointer<ElementType>::value, ElementType, ElementType&>::type
 	operator[](std::size_t index)
 	{
-		LTTNG_ASSERT(index < ContainerOperations::size(_container));
-		return ContainerOperations::get(_container, index);
+		/*
+		 * To share code between the const and mutable versions of this operator, 'this'
+		 * is casted to a const reference. A const_cast then ensures that a mutable
+		 * reference (or pointer) is returned.
+		 *
+		 * We typically avoid const_cast, but this is safe: if the user is calling the
+		 * mutable version of this operator, it had a mutable object anyhow.
+		 *
+		 * For more information, see Item 3 of Effective C++.
+		 */
+		const auto& const_this = static_cast<const decltype(*this)&>(*this);
+
+		/* NOLINTNEXTLINE(cppcoreguidelines-pro-type-const-cast) */
+		return const_cast<typename std::conditional<std::is_pointer<ElementType>::value,
+							    ElementType,
+							    ElementType&>::type>(const_this[index]);
 	}
 
 	typename std::conditional<std::is_pointer<ElementType>::value,
@@ -127,7 +143,13 @@ public:
 				  const ElementType&>::type
 	operator[](std::size_t index) const
 	{
-		LTTNG_ASSERT(index < ContainerOperations::size(_container));
+		if (index >= ContainerOperations::size(_container)) {
+			LTTNG_THROW_INVALID_ARGUMENT_ERROR(fmt::format(
+				"Out of bound access through random_access_container_wrapper: index={}, size={}",
+				index,
+				size()));
+		}
+
 		return ContainerOperations::get(_container, index);
 	}
 
-- 
2.34.1