feat: add map/unordered_map bounds checking #3671

arng40 · 2025-05-16T15:15:16Z

This PR adds further bounds checking in GEOS.

std::map/unordered_map wrapper that has bounds checking for ::operator
disallow use of map/unordered_map::operator[] to perform insertion.
the new wrappers are preprocessor guarded with aliases s.t. they alias to the std::container in the non-bounds checking case.

…oveMapBoundsChecking

arng40 · 2025-06-03T08:30:18Z

I think that the compil issue might be related to inheriting constructors

…oveMapBoundsChecking

MelReyCG

Is it normal that there still are some direct usage of std::vector & std::map? (unitTests/, event/, fileIO/...)

MelReyCG · 2025-06-05T09:20:40Z

I think we will need one more PR for std::array<>.
@arng40 @rrsettgast

MelReyCG · 2025-06-05T09:13:15Z

src/coreComponents/dataRepository/xmlWrapper.cpp

@@ -22,6 +22,7 @@
 #include "xmlWrapper.hpp"

 #include "common/format/StringUtilities.hpp"
+#include "common/StdContainerWrappers.hpp"


why adding this header here?

MelReyCG · 2025-06-05T09:14:53Z

src/coreComponents/constitutive/unitTests/testMultiFluidCO2Brine.cpp

+  string_array & phaseNames = group.getReference< string_array >( MultiFluidBase::viewKeyStruct::phaseNamesString() );
  phaseNames = {"gas", "liquid"};

-  auto & compNames = group.getReference< string_array >( MultiFluidBase::viewKeyStruct::componentNamesString() );
+  string_array & compNames = group.getReference< string_array >( MultiFluidBase::viewKeyStruct::componentNamesString() );


Now that we found the compilation issue, I think you can revert that.

MelReyCG · 2025-06-05T09:15:53Z

src/coreComponents/constitutive/unitTests/CMakeLists.txt

     testMultiFluidBlackOil.cpp
-     testMultiFluidCO2Brine.cpp
     testMultiFluidTwoPhaseCompositionalMultiphase.cpp
+     testMultiFluidCO2Brine.cpp
     testMultiFluidDeadOil.cpp


MelReyCG · 2025-06-05T11:52:01Z

src/coreComponents/common/StdContainerWrappers.hpp

   * Access element at index with bounds checking if USE_STD_CONTAINER_BOUNDS_CHECKING is true.
   * Otherwise, uses operator[] for unchecked access.


Suggested change

* Access element at index with bounds checking if USE_STD_CONTAINER_BOUNDS_CHECKING is true.

* Otherwise, uses operator[] for unchecked access.

* Access element at index with bounds checking if USE_BOUNDS_CHECKING is true.

* Otherwise, uses operator[] for unchecked access.

* @throws std::out_of_range if index is out of bounds.

(and same below)

MelReyCG · 2025-06-05T13:41:28Z

src/coreComponents/common/StdContainerWrappers.hpp

+template< typename TKEY, typename TVAL, typename SORTED >
+class mapBase
+{};
+
+/// @cond DO_NOT_DOCUMENT
+template< typename TKEY, typename TVAL >
+class mapBase< TKEY, TVAL, std::integral_constant< bool, true > > : public stdMap< TKEY, TVAL >
+{
+public:
+  using stdMap< TKEY, TVAL >::stdMap;
+};
+
+template< typename TKEY, typename TVAL >
+class mapBase< TKEY, TVAL, std::integral_constant< bool, false > > : public stdUnorderedMap< TKEY, TVAL >
+{
+  using stdUnorderedMap< TKEY, TVAL >::stdUnorderedMap;
+};


I think we could simplify this part:

about the mapBase naming, there is no real "base" concept here, nor need for a class here,

why not simplifying the std::integral_constant to a simple bool?

a pattern like the std::*_t would be more standard.

I would suggest:

namespace internal { template< typename TKEY, typename TVAL, bool SORTED > struct stdMapTypeDefinition {}; template<typename TKEY, typename TVAL> struct stdMapTypeDefinition< TKEY, TVAL,true > { using Type = stdMap< TKEY, TVAL >; }; template<typename TKEY, typename TVAL> struct stdMapTypeDefinition< TKEY, TVAL,false > { using Type = stdUnorderedMap< TKEY, TVAL >; }; } // namespace internal /** * @brief templated type for ordered and unordered maps. * @tparam TKEY key type * @tparam TVAL value type * @tparam SORTED a bool indicating whether map is ordered */ template< typename TKEY, typename TVAL, bool SORTED > using stdMapType = internal::stdMapTypeDefinition<TKEY,TVAL,SORTED>::Type;

They would be a lot of changements in BufferOps.hpp imo

I renamed mapBase to mapType
I can't change typename SORTED to bool SORTED, I tried and it required a lot of work for this PR and maybe when we will jump into c++20 for concepts

MelReyCG · 2025-06-05T13:44:16Z

src/coreComponents/common/StdContainerWrappers.hpp

+template< typename MapType,
+          bool USE_BOUNDS_CHECKING = false
+          >


Suggested change

template< typename MapType,

bool USE_BOUNDS_CHECKING = false

>

template< typename MapType,

bool USE_BOUNDS_CHECKING = false >

MelReyCG · 2025-06-05T13:47:08Z

src/coreComponents/common/StdContainerWrappers.hpp

+  // Override operator[]
+  MappedType & operator[]( KeyType const & key )
+  {
+    if constexpr (USE_BOUNDS_CHECKING)
+    {
+      return this->at( key );  // Throws std::out_of_range if key is missing
+    }
+    else
+    {
+      return Base::operator[]( key );  // Inserts default-constructed value if missing
+    }
+  }
+
+  MappedType const & operator[]( KeyType const & key ) const
+  {
+    if constexpr (USE_BOUNDS_CHECKING)
+    {
+      return this->at( key );
+    }
+    else
+    {
+      return Base::operator[]( key );
+    }
+  }


Where the behaviour of these operators is tested in fonction of all possible USE_BOUNDS_CHECKING values? (same remark for stdVector, except that it cannot be tested against USE_BOUNDS_CHECKING=false)
If they are not tested, we can add them in testDataTypes.cpp.

add a test in testDataTypes.cpp

…oveMapBoundsChecking

rrsettgast · 2025-06-17T17:21:25Z

@arng40 Do you think it would be good idea to introduce the aliases (i.e. stdUnorderedMap, stdMap) in a separate PR which just aliases to std::unordered_map, and std::map so that we can review the actual changes independently from this swap...which should result in no behavioral changes with develop?

arng40 · 2025-06-18T08:39:43Z

@arng40 Do you think it would be good idea to introduce the aliases (i.e. stdUnorderedMap, stdMap) in a separate PR which just aliases to std::unordered_map, and std::map so that we can review the actual changes independently from this swap...which should result in no behavioral changes with develop?

@rrsettgast
I don't see any problem in splitting into 2 PR. I wond if in the 2nd PR we shouldn't use map and unordered_map as alias (stdMap cannot be converted to a map) to avoid confusion for when we use stdMap or map.

arng40 added 2 commits May 16, 2025 16:39

std::map -> stdMap

6fa6616

std::unordered_map -> stdUnorderedMap

4bfd8e4

arng40 self-assigned this May 16, 2025

arng40 added the type: feature New feature or request label May 16, 2025

arng40 changed the title ~~Improve map/unordered_map bound checking~~ Improve map/unordered_map bounds checking May 16, 2025

arng40 added 4 commits May 19, 2025 14:25

missing stdVector

41ccba0

remove unused code & move mapBase declaration

7d7ec24

use GEOS_USE_BOUNDS_CHECK as macro

3a73e09

doxygen and simplification

f36cc17

arng40 changed the title ~~Improve map/unordered_map bounds checking~~ feat: add map/unordered_map bounds checking May 19, 2025

arng40 added ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: no rebaseline Does not require rebaseline labels May 19, 2025

arng40 added 3 commits May 19, 2025 15:27

xsd

6f01722

Merge remote-tracking branch 'origin/develop' into feature/dudes/impr…

109e9d5

…oveMapBoundsChecking

uncrustify + xsd

7aeff12

arng40 marked this pull request as ready for review May 19, 2025 13:48

arng40 requested review from rrsettgast, CusiniM, corbett5, wrtobin, MelReyCG, castelletto1, jhuang2601, Guotong-Ren, paveltomin, tjb-ltk, dkachuma, untereiner and victorapm as code owners May 19, 2025 13:48

arng40 and others added 2 commits May 27, 2025 11:16

missing stdVector after merge

9d1b483

uncrustify

378b60a

arng40 added 7 commits June 3, 2025 14:06

explicitly declare constructors

7a70f7b

fix unused var

e65cdd5

fix error in template naming

b181d9d

fix constructor overloads & renaming

5fc335f

Merge remote-tracking branch 'origin/develop' into feature/dudes/impr…

d3d9fb9

…oveMapBoundsChecking

malformed doxy

2b508c7

comment on constructors bug

869e6dd

MelReyCG reviewed Jun 5, 2025

View reviewed changes

MelReyCG requested changes Jun 5, 2025

View reviewed changes

arng40 added 4 commits June 10, 2025 15:10

first review pass

4b207ca

Merge remote-tracking branch 'origin/develop' into feature/dudes/impr…

4e4b317

…oveMapBoundsChecking

[not working] - attempt to simplify sorted type

331f819

revert attempt for simplifing map type and replace some map with stdMap

67d86c6

arng40 requested review from frankfeifan and cssherman as code owners June 13, 2025 08:54

add boundscCheckinng test

698db4c

arng40 requested a review from MelReyCG June 13, 2025 09:25

arng40 added 6 commits June 13, 2025 15:18

mark a unused

685243b

attempt to remove ci rror with pragma

cfbd819

2nd test

29c1bdd

ezmove UB in test

7f6c426

atttempt to remove last ci errordue to oor

fbe3cb2

Merge remote-tracking branch 'origin/develop' into feature/dudes/impr…

7b50ec2

…oveMapBoundsChecking

try to remove last bugs

5acd636

		* Access element at index with bounds checking if USE_STD_CONTAINER_BOUNDS_CHECKING is true.
		* Otherwise, uses operator[] for unchecked access.

feat: add map/unordered_map bounds checking #3671

Are you sure you want to change the base?

feat: add map/unordered_map bounds checking #3671

Conversation

arng40 commented May 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arng40 commented Jun 3, 2025

Uh oh!

MelReyCG left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MelReyCG commented Jun 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arng40 Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rrsettgast commented Jun 17, 2025

Uh oh!

arng40 commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

arng40 commented May 16, 2025 •

edited

Loading

MelReyCG left a comment •

edited

Loading

arng40 Jun 13, 2025 •

edited

Loading

arng40 commented Jun 18, 2025 •

edited

Loading