Should it be reentrant and not recursive?

[X-Ryl669]

I think recursiveness is not an issue but reentrancy is. I think you meant the latter in the README.

[Lectem]

I have to admit I'm not a native english speaker, so I am not sure about the best term to use here.

An example of the issue:

• collection.add() (for example std::vector::push_back)
• copy constructor of the added object (which references the collection)
• collection.get() (for example std::vector::size )

The example above is indeed not exactly a recursion (as in calling the same function twice) but attempting to modify the data while doing operations on it (which is also close to the definition of a race condition, but it's way too often used to describe race conditions in a concurrent context, and I'd like to disambiguate the issue).

I don't mind changing the term if you think it is more accurate !

[X-Ryl669]

I've hard time figuring out why recursion would be a problematic. Unlike reentrancy.

For example, let's say you have a code like this:

template <class T>
void dumpContainer(T & container)
{
    for (auto i = 0; i < container.size(); ++i)
    {
         std::cout << container[i] << std::endl;
    }
}

This is perfectly fine, except if you get a signal (from the kernel) and you have this signal handler:

void sig_handler(...)
{ 
    // Exiting program
    globalContainer.clear();
}

If the globalContainer is being dump while the signal happens, then you have a reentrancy issue, since the vector's size is modified in the signal handler (in clear method) so the local variable i is now trashed.

This can't happen in a recursive method, if I understand correctly, since the control is transferred when leaving the function. Let's take an similar example:

template <class T, class U>
void savePrimes(T & container, U lastKnown)
{
    for (auto i = 0; i < 10; ++i)
    {
         if (isPrime(i + lastKnown)) container.push_back(i + lastKnown);
    }
    size_t s = container.size();
    if (lastKnown < 100) savePrimes(container, lastKnown + 10);
    
    for (auto i = 0; i < s; i++)
        std::cout << container[i] << std::endl;
}

In this example, I've tried to write the dumbest prime searching function possible with an obvious bug. I'm reading the size before modifying the container and using the stale value for dumping it. This is a logical error, but it'll go unnoticed since the container is growing. It would crash if the container was shrinking in savePrimes. This isn't a recursive issue, nor a reentrancy issue here, since it's impossible for the control flow to change the size variable, it's a plain old logical error.

Do you mean that BAG will detect this case? Or, will it detect the former case with the signal handler?

[Lectem]

So the recursion I'm talking about it not from the point of view of the user code (which is totally fine) but from the point of view of the container itself.

Or, will it detect the former case with the signal handler?

It could detect some cases with the signal handler if the signal interrupts one of the container methods. It's actually exactly the same case as if the objects constructor/destructor were triggering something related to the container itself.

Here is one case I've seen in production, which is very similar to what might find using signals except here it is with callbacks. Let's modify the following example a bit (and I'll update it if you think it's explains things better)

BadAccessGuards/examples/GuardedVectorExample.cpp

Lines 11 to 28 in fa51c46

	struct RecursiveBehaviour {
	ExampleGuardedVector<RecursiveBehaviour>& vectorRef;
	int& valueRef;
	RecursiveBehaviour(ExampleGuardedVector<RecursiveBehaviour>& v, int& value)
	: vectorRef(v)
	, valueRef(value)
	{
	}
	~RecursiveBehaviour()
	{
	// This is something that sometimes happen when you have a lot of callbacks...
	// Things will try to use the container they are being removed from,
	// which is sometimes fine, but most often dangerous.
	// Here we're doing a read, but it could be something adding an element !
	valueRef += vectorRef.size();
	}
	};

To the following

std::vector<std::function<void()>> delegates;

struct RecursiveBehaviour {
    RecursiveBehaviour()
    {
        // You are actually inside the `ExampleGuardedVector::emplace_back()`!
        for (const auto& delegate : delegates)
        {
            delegate();
        }
    }
};

void TriggerRecursionDetection()
{
    ExampleGuardedVector<RecursiveBehaviour> vector;
    // Somewhere you setup a delegate function clearing the vector
    delegates.push_back([&]() {
        vector.clear();
        });

    // Then you add some item that triggers delegates (could be a signal, event, etc...)
    // The issue is that while modifying the vector itself (you're building the item inside it!), you're also asking to clear it (due to the delegate).
    vector.emplace_back();
}

Callstack:

As you can see this is almost always undesirable, those kind of operations should not be nested (or it is at the very least dangerous).