Undefined usage of std::vector in PointColDetector

[JonRurka]

While testing and researching isolated sections of the codebase, I have discovered the std::vector usage in PointColDetector is undefined, as per the std::vector C++ documentation, and can cause access violations depending on the compiler's implementation of std::vector.

The values of "m_pointid_list" and "m_ref_list" are initially set using the operator [], but this usage is officially considered undefined, according to the "Notes" on cppreference.com:

https://en.cppreference.com/w/cpp/container/vector/operator_at

Each element should first be appended to the vector with std::vector::push_back(), even when vector::resize() was set to the needed size (as it's not just a normal array, there are other internal mechanisms that rely on push_back, depending on the implementation).

The issue occurs at https://github.com/RigsOfRods/rigs-of-rods/blob/master/source/main/physics/collision/PointColDetector.cpp#L105

I will be keeping my eye out for more instances of undefined vector appending.

[ohlidalp]

Thank you for reporting. This is positively a bug; somebody had set bad vector sizes. There should be an assert() checking the vector size to make it obvious that we don't intend to insert new elements.

I looked to the history trying to find some suspicous changes, but I got nothing - there had been signifficant code changes in the past, but in all cases the m_pointid_list was both resized and accessed in sync with number of contacter nodes.

I also looked at your modified version which you posted on Discord chat: https://github.com/JonRurka/SoftBodyLib/commit/75bd32d5b36a5f37f70e7ceb77b1011d57ab7614 but I see no signifficant difference, except you use naked new[] rather than vector::resize(). I'm, however, positive that using operator[] to read/write elements created by vector::resize() is OK - otherwise what would resize() be even for?

At the moment I don't know that code well enough to inspect it, I have to study it first. Can you tell me under what scenario did you observe the out-of-range access?

[JonRurka]

I had recently changed my implementation to use array pointer with new[] to allocate PointColDetector arrays. When I first brought up this issue, this was the state of my implementation: https://github.com/JonRurka/SoftBodyLib/blob/077476294960bdd485386e5a87a3068255e72d87/source/library/physics/collision/PointColDetector.cpp#L96

In regards to: "I'm, however, positive that using operator[] to read/write elements created by vector::resize() is OK - otherwise what would resize() be even for?"

I would say it's similar (at least in theory) to how List works in C#, where elements still need to be added with .Add() before being accessed despite giving the List constructor an initial size. Resize may allocate the memory, but how the internal logic works for operator[] can be different for different compilers. The official documentation of "operator_at" states:

_"Note: Unlike std::map::operator[], this operator never inserts a new element into the container. Accessing a nonexistent element through this operator is undefined behavior."_

As Standard libraries are not all programmed the same internally (depending on the environment), the behavior is considered undefined because different compilation environments treat this differently. In the case of the compiler used in my project, it does not support this operation.

Using operator[] this way seems to work in the RoR environment currently, however I'd remain conscious of the non-standard use of vector additions using that operator in case it causes problems in the future.