Re: [Flightgear-devel] Segmentation fault acquiring SGPropertyLock

[Julian S. <ju...@op...>]

On Tue, 18 Jul 2023 11:18:22 +0200
Florent Rougon <f.r...@fr...> wrote:

> Hi,
> 
> Julian Smith <ju...@op...> wrote:
> 
> > This diff seems to fix things for me:  
> 
> Many thanks, same here!
> 
> > The problem was that tiedProperties.Tie() was calling
> > FGProperties::setFreeze(true) before it returned, but we were only setting
> > _simFreezeNode *after* tiedProperties.Tie() had returned.  
> 
> Indeed, TiedPropertyList::Tie() calls SGPropertyNode::tie()
> (simgear/simgear/props/props.cxx:4217) which, when the “useDefault” arg
> is true which is the case here, does setValue(exclusive, old_val) which,
> in the case at hand, calls the setter method FGProperties::setFreeze().
> As you explained, this executes _simFreezeNode->fireValueChanged() but
> _simFreezeNode hasn't been set at this point yet.
> 
> > tiedProperties.Tie() simply returns the node that it was given as its
> > first arg, so the diff simply sets _simFreezeNode to the value it would
> > have taken later, before calling tiedProperties.Tie().  
> 
> > Please note that it's entirely possible that i've missed something
> > subtle here. I haven't considered what's going on in any great detail,
> > and i've never used tied properties myself.  
> 
> I've read the code of SGRawValueMethods, TiedPropertyList and
> SGPropertyNode::tie() to understand this but that's about what I know on
> the subject matter — and nothing about the freeze process per se. Your
> analysis looks good to me and your patch fixes the crash I reported
> yesterday.
> 
> Maybe James will want to double-check, but since he seems to be busy
> with RL, given the non-invasiveness of your patch and our understanding
> of this issue, I believe it's pretty safe to commit.

Great, good to know you think it's ok. I've just pushed the fix.

> 
> Thanks & regards
> 
> P.S.: your patch (the attachment) had all explanations plus your
>       signature inside (yeah, I know the patch tool tolerates
>       “trailing, uh... garbage”). :)

I reckon that might be your email client? - i didn't attach the patch,
just included it inline.

Thanks,

- Jules

-- 
http://op59.net