Re: [Flightgear-devel] [Flightgear-commitlogs] [SimGear] branch next updated: props: Over-protect nodeOriginsLock

[James H. <ja...@al...>]

On 18 June 2023 14:57:29 BST, Julian Smith <ju...@op...> wrote:
>On Sun, 18 Jun 2023 09:54:14 +0100
>James Hogan via Flightgear-devel
><fli...@li...> wrote:
>> It seems that on certain Mac platforms, the lock is no longer usable
>> after it has been destroyed resulting in exceptions when used by
>> destructing property nodes, presumably like the one Fahim Dalvi
>> quoted yesterday:
>> 
>> libc++abi: terminating with uncaught exception of type
>> std::__1::system_error: mutex lock failed: Invalid argument
>> 
>> So yeh its a destructor ordering thing, and it'd be better if the
>> properties were cleaned up first. Given that its not a problem on
>> Linux or I think Windows I figured its best to just make it work in
>> that case.
>
>Thanks for this.
>
>I've managed to provoke the problem on Linux by creating a global
>SGPropertyNode_ptr with `SGPropertyNode_ptr g_test1(new
>SGPropertyNode);` in simgear/simgear/props/props.cxx itself, before
>(i.e. above in the source code) the `NecroLock nodeOriginsLock;` line.
>
>Globals are destroyed in the reverse order in which they were
>constructed, so this guarantees that g_test1's destructor will be
>called /after/ nodeOriginsLock's destructor, so the former's call to
>SGSourceLocation() will try to use a destructed NecroLock.
>
>This reproducer only works reliably because everything is happening in
>the same compilation unit. Order of initialisation between compilation
>units is undefined, so i suspect it's just luck that things are
>currently working on Linux and Windows.

To clarify, adding debug logging shows that the use after destruction of the mutex does seem to happen on linux too, even in fgfs (not just the unit tests), but only on mac has that caused a problem. I'm guessing other platforms leave destructed mutex memory in a state still usable as a lock.

>> > [If the problem is that properties are being manipulated after the
>> > static global `std::shared_mutex nodeOriginsLock` is being
>> > destructed by the C++ runtime, and for some reason we cannot avoid
>> > this happening, then maybe we could put nodeOriginsLock on the heap
>> > and never delete it, e.g. with `static std::shared_mutex&
>> > nodeOriginsLock = *new std::shared_mutex`?]  
>> 
>> Sure, that does seem better. I'll give that a try and report back
>> here with a patch for a kind Mac user to test.
>
>A possibly more elegant solution might be to make all SGPropertyNode
>constructors call a new dummy function in NecroLock(), which will ensure
>that the global nodeOriginsLock is created before all SGPropertyNode's.
>So nodeOriginsLock will be correspondingly destroyed /after/ all
>SGPropertyNode's.
>
>Arguably sticking nodeOriginsLock on the heap is a simpler solution
>though...?

Yeh, I think I prefer putting it on the heap. More readable/understandable I think.

thanks
JamesH