#2858 ILS search with navinfo() causes 2020.4 to crash

[Ivan Strebkov]

Recently I noticed that some search attempts with nasal function navinfo() (and also findNavaidsByID() and possibly some else under certain conditions) were crashing the 2020.4 without any remarks in log, even at bulk level, while 2020.3.19 did absolutely fine.

The file with nasal console code to reproduce the issue is attached, start position should not matter but tested in UERR and LOWI airports.

So far I managed to spot the issue only with ILS search, but I'm not familiar with FG codebase, so I can't guarantee that there no other cases causing navinfo(), airportinfo(), findNavaidByID() or other functions working with navaid or airport ghosts to crash the 2020.4.

I tested it myself on Windows 10 and, unfortunately, I don't have GDB to debug this properly, but I will also attach a MacOS crash reporter backtrace (which is possibly similar to GDB output).

[James Turner]

It's something to do with a custom scenery path, try with only the base scenery configured?

---

[Ivan Strebkov]

Tried right now and without any custom scenery the issue is still present on 2020.4

[Ivan Strebkov]

Should've clarify this better: macos crash report was provided not by me but by Fahim on FG discord server, so it's possible that there are also some issues with his FG config.
But that's the best what I have.

[Gijs]

I can replicate the issue by just calling this:

findNavaidsByID("SG");

>   [Inline Frame] fgfs.exe!std::_Char_traits<char,int>::copy(char * const) Line 64 C++
    [Inline Frame] fgfs.exe!std::string::_Construct(const char * const) Line 2690   C++
    fgfs.exe!std::string::basic_string<char,std::char_traits<char>,std::allocator<char>>(const std::string & _Right) Line 2499  C++
    fgfs.exe!FGAirport::sceneryPath() Line 98   C++
    fgfs.exe!XMLLoader::findAirportData(const std::string & aICAO, const std::string & aFileName, SGPath & aPath) Line 119  C++
    fgfs.exe!FGAirport::validateILSData() Line 805  C++
    fgfs.exe!flightgear::NavDataCache::loadById(__int64 rowid) Line 2033    C++
    fgfs.exe!flightgear::NavDataCache::NavDataCachePrivate::findAllByString(const std::string & s, const std::string & column, FGPositioned::Filter * filter, bool exact) Line 871  C++
    fgfs.exe!flightgear::NavDataCache::findAllWithIdent(const std::string & s, FGPositioned::Filter * filter, bool exact) Line 2243 C++
    fgfs.exe!FGPositioned::findAllWithIdent(const std::string & aIdent, FGPositioned::Filter * aFilter, bool aExact) Line 333   C++
    fgfs.exe!FGNavList::findByIdentAndFreq(const std::string & ident, const double freq, FGNavList::TypeFilter * filter) Line 273   C++
    fgfs.exe!FGNavList::findByIdentAndFreq(const SGGeod & position, const std::string & ident, const double freq, FGNavList::TypeFilter * filter) Line 294  C++
    fgfs.exe!f_findNavaidsByIdent(Context * c, naRef me, int argc, naRef * args) Line 1467  C++

This is without custom scenery. Just TerraSync.

@frougon, I wonder if this may have something to do with your commit [351ca1]? For some reason the sceneryPath seems to be gibberish (I once got Path "▼ instead of Path "" when printing it).

But, interestingly it doesn't always crash with "SG". Sometimes it doesn't. Sometimes it crashes with "SB" or "SA", sometimes it doesn't.

The whole findNavaidsByID function seems a bit fishy though, as the results of findNavaidsByID() seem to consistently include duplicates (but only for some of the items!) on the first call in a FlightGear session...

For example, first call of findNavaidsByID("SA");

MIELEC NDB
MIELEC NDB
SANTANDER NDB
SANTANDER NDB
EFSA 12 GS
EFSA 12 ILS-cat-I
EFSA 12 ILS-cat-I
SORREISA NDB
SORREISA NDB
SKAGI NDB
SKAGI NDB
SARATOV NDB
SARATOV NDB
ELISTA NDB
ELISTA NDB
NAMANGAN NDB
NAMANGAN NDB
SANAA NDB
SANAA NDB
SAURIMO NDB
SAURIMO NDB
MAVIS NDB
MAVIS NDB
BLUIE NDB
BLUIE NDB
SAMBAVA NDB
SAMBAVA NDB
SASA 02 ILS-cat-I
SASA 02 GS
SOROAKO NDB
SOROAKO NDB
STANLEY NDB
STANLEY NDB
SOLA NDB
SOLA NDB

Second and consecutive calls of findNavaidsByID("SA");

MIELEC NDB
SANTANDER NDB
EFSA 12 GS
EFSA 12 ILS-cat-I
SORREISA NDB
SKAGI NDB
SARATOV NDB
ELISTA NDB
NAMANGAN NDB
SANAA NDB
SAURIMO NDB
MAVIS NDB
BLUIE NDB
SAMBAVA NDB
SASA 02 ILS-cat-I
SASA 02 GS
SOROAKO NDB
STANLEY NDB
SOLA NDB

@jmturner or @frougon: any ideas on how to further debug this?

[James Turner]

I'm going to debug it 'soon', don't worry about it (got some other changes in the same area I need to finish, first)

[Florent Rougon]

Thanks @gijsrooy for the nice recipe to reproduce the bug.

AFAICS, this is not related to my commit 351ca111.

While debugging this crash using what Gijs proposed, I found that NavDataCachePrivate::findAllByString() was running the following query:

SELECT rowid FROM positioned WHERE ident="LFLS" AND type>=1 AND type <=3;

However, the result was consistently 325005 which does not correspond to ident="LFLS":

sqlite> SELECT * FROM positioned WHERE rowid=325005;
type  ident  name        airport  lon         lat          elev_m    octree_node  cart_x            cart_y            cart_z 
----  -----  ----------  -------  ----------  -----------  --------  -----------  ----------------  ----------------  ----------------
14    SG     ENSG 24 GS  19170    7.13940833  61.15748611  496.5192  9040576690   3061090.47654637  383417.255814895  5564264.22822344
s

This happens because NavDataCachePrivate::findAllByString() is used as if it were reentrant, whereas it is not.

The actual query is: SELECT rowid FROM positioned WHERE ident=?1 AND type>=?2 AND type <=?3 which is cached in the findByStringDict std::map. Using Gijs' recipe, this query is executed with "SG" substituted for ?1 and before the reset() is done for the prepared SQL statement, the same query using the same prepared SQL statement is executed with "LFLS" substituted for ?1 (this is because of the reentrant call originating from NavDataCache::loadById() in NavDataCachePrivate::findAllByString()). However, since reset() hasn't been called yet for the first query, stepSelect() returns results for ident="SG" (actually, there is even another query in between, for ident="ENSG").

Anyway, as NavDataCachePrivate::findAllByString() is written, it needs to be reentrant which, unless I'm mistaken, makes it impossible to cache the prepared SQL statements as attempted with findByStringDict (these statements retain execution state until reset() is called for them).

The attached patch removes this caching and appears to fix the crash triggered by Gijs' recipe.

(This might also fix what @merspieler reported on flightgear-devel about some airports not being found in their custom WS3 scenery using updated WS3 apt.dat.)

[James Turner]

I'd prefer to avoid the re-entrancy rather than pessimise the performance, though: the reentrancy was not really intended here, just na effect of the 'loadILSData' call.

[James Turner]

From a bit more reading: I think we can just change findAllByString to build up a vector<positionedid>, reset the query and then call loadById on each, which will avoid the re-entrancy.</positionedid>

[Florent Rougon]

@jmturner No more time today but I believe you are quite right. Besides, I retract that the problem would not be related to my commit 351ca111—it did introduce reentrancy because of:

bool XMLLoader::findAirportData(const std::string& aICAO, 
    const std::string& aFileName, SGPath& aPath)
{
  FGAirportRef airport = FGAirport::findByIdent(aICAO);

Anyway, to bed now. :)

Regards

[Florent Rougon]

New patch attached. Lightly tested but seems to work for “Gijs' crash recipe.” :-)

[merspieler]

Tried both patches, the problem persists.
Thanks for mentioning us tho

[Florent Rougon]

@merspieler To be clear, you are not talking about the problem mentioned in this report, right? If this is indeed a different issue, please give a foolproof recipe to reproduce it (would be better in a separate report). I may be able to debug it, though I can't promise this will happen in a short time frame.

[merspieler]

Yes, it's about the issue from the mailing list you've mentioned.
I'll open a new ticket.

[Gijs]

Hi Florent, fix_findAllByString_v2.patch works fine here! No more duplicate entries on the first call, and no crashes so far. Great job!

[James Turner]

Yep that path looks reaosnable to me as well, Florent or Gojs you can apply, I am making some other NavCache changes right now so don't want to switch branches

[Florent Rougon]

@jmturner Thanks for your confirmation! I've pushed the fix with minor editing changes [fbbc14] :

• placement of opening braces;
• id (resp. ids) replaced with guid (resp. guids) for consistency with other NavDataCache code.

Regards