Sector Map: Auto Route button crashes game when no hyperdrive fitted

[craigo-]

Observed behaviour

With a ship that is not fitted with a hyperdrive, pressing the "Auto Route" button on the sector map results in a game crash:

Expected behaviour

That the game doesn't crash 😄

Perhaps for ships without a hyperspace drive, pressing the "Auto Route" button could generate an informational alert (e.g. "No hyperdrive"), or perhaps simply do nothing at all.

Steps to reproduce

1. Start at Barnard's Star (Xylophis, no hyperdrive)
2. Switch to Sector Map
3. Select any system distant from Barnard's Star
4. Click "Auto Route"

NB: when doing the above with a hyperdrive-equipped ship, the game does not crash.

My pioneer version (and OS):

Pioneer 20190203, and compiled from master (commit 4bc31eef4700b343256cb1dae604622680c534d8)

Windows 10 Enterprise 1809 x64

[mike-f1]

The bug raise in https://github.com/pioneerspacesim/pioneer/blob/4bc31eef4700b343256cb1dae604622680c534d8/src/SectorView.cpp#L595

when a GetMaximumRange is searched in an empty table, as the previous line is getting an engine which is not present.

In particular, when PushValueToStack (in LuaTable.h, line 311) calls lua_gettable, it (eventually) fail.

A fast workaround is to check in Lua if there's an engine, but a better solution may be to put a general mechanism to check if there's a value that should be returned.

What's your thoughts?

Ping @Web-eWorks , @ecraven and @laarmen

[impaktor]

I assume C++ side shouldn't fail with a crash, but fail with grace, like a ballerina falling on her ass.

[laarmen]

My first thought is "why is the hyperdrive an empty table and not nil" (from your description of the incident). If that's the case, that's IMO a bug. If not, it means we have a ScopedTable that's not backed by a proper Lua table. The original author of said class probably couldn't be bothered to imagine that somebody would be stupid enough to do that. As it turns out, since both author and user are the same person, me, they/he/I was wrong.

The ScopedTable assumption that the table is valid is IMHO a good thing to keep in the general case, as the instances are intended to be short-lived, and in most cases the dev actually knows there's a table. However, a method to actually check that it is the case, that would be called explicitly, would be a great addition.

On Thu, Aug 22, 2019 at 1:47 PM Karl F notifications@github.com wrote:

I assume C++ side shouldn't fail with a crash, but fail with grace, like a ballerina falling on her ass.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pioneerspacesim/pioneer/issues/4667?email_source=notifications&email_token=AAEQ4NPF6YGQRRDD3UN7Y63QFZ4GFA5CNFSM4IMNFIF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD442E6Y#issuecomment-523870843, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEQ4NIOXKUFVFD7DT2FPPDQFZ4GFANCNFSM4IMNFIFQ .

[laarmen]

(I'm not claiming authorship of the autorouting code, just the bit fetching the hyperspace range)

On Thu, Aug 22, 2019 at 2:14 PM Simon Chopin chopin.simon@gmail.com wrote:

My first thought is "why is the hyperdrive an empty table and not nil" (from your description of the incident). If that's the case, that's IMO a bug. If not, it means we have a ScopedTable that's not backed by a proper Lua table. The original author of said class probably couldn't be bothered to imagine that somebody would be stupid enough to do that. As it turns out, since both author and user are the same person, me, they/he/I was wrong.

The ScopedTable assumption that the table is valid is IMHO a good thing to keep in the general case, as the instances are intended to be short-lived, and in most cases the dev actually knows there's a table. However, a method to actually check that it is the case, that would be called explicitly, would be a great addition.

On Thu, Aug 22, 2019 at 1:47 PM Karl F notifications@github.com wrote:

I assume C++ side shouldn't fail with a crash, but fail with grace, like a ballerina falling on her ass.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pioneerspacesim/pioneer/issues/4667?email_source=notifications&email_token=AAEQ4NPF6YGQRRDD3UN7Y63QFZ4GFA5CNFSM4IMNFIF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD442E6Y#issuecomment-523870843, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEQ4NIOXKUFVFD7DT2FPPDQFZ4GFANCNFSM4IMNFIFQ .

[mike-f1]

If that's the case, that's IMO a bug.

:+1:

However, a method to actually check that it is the case, that would be called explicitly, would be a great addition.

This line: https://github.com/pioneerspacesim/pioneer/blob/4bc31eef4700b343256cb1dae604622680c534d8/src/SectorView.cpp#L593

is initializing a ScopedTable from a LuaRef, which already has a IsValid() method. However, that method seems not working: splitting that line in

    LuaRef lua_ref = LuaObject<Player>::CallMethod<LuaRef>(Pi::player, "GetEquip", "engine", 1);
    if (!lua_ref.IsValid()) return;

    const ScopedTable hyperdrive = ScopedTable(lua_ref);

doesn't prevent the error as a LUA_NOREF is -2 whilst lua_ref.m_id is -1 (here the link)

And here I start having troubles understanding why the above happens and what's the logic behind :worried:

...If I may suggest something, all the above could be helped with a check on lines:

https://github.com/pioneerspacesim/pioneer/blob/4bc31eef4700b343256cb1dae604622680c534d8/src/LuaTable.h#L310-L316

which print if there's no value for the given key improving (or I hope so) debugging...

[laarmen]

I can think of a number of ways to do fix all this. First off, we should be able to directly return a ScopedTable from CallMethod by using move semantics to avoid the destructions triggered by the returns. That would mean getting rid of the LuaRef, whose use is way overkill here. However, this means that we still need a way to report back to the caller that there is no hyperdrive. Is there an Option type in C++ these days ?

On Thu, Aug 22, 2019 at 3:08 PM mike-f1 notifications@github.com wrote:

If that's the case, that's IMO a bug.

👍

However, a method to actually check that it is the case, that would be called explicitly, would be a great addition.

This line:

https://github.com/pioneerspacesim/pioneer/blob/4bc31eef4700b343256cb1dae604622680c534d8/src/SectorView.cpp#L593

is initializing a ScopedTable from a LuaRef, which already has a IsValid() method. However, that method seems not working: splitting that line in

LuaRef lua_ref = LuaObject::CallMethod(Pi::player, "GetEquip", "engine", 1);

if (!lua_ref.IsValid()) return;

const ScopedTable hyperdrive = ScopedTable(lua_ref);

doesn't prevent the error as a LUA_NOREF is -2 whilst lua_ref.m_id is -1 (here the link https://github.com/pioneerspacesim/pioneer/blob/4bc31eef4700b343256cb1dae604622680c534d8/src/LuaRef.h#L28 )

And here I start having troubles understanding why the above happens and what's the logic behind 😟

...If I may suggest something, all the above could be helped with a check on lines:

https://github.com/pioneerspacesim/pioneer/blob/4bc31eef4700b343256cb1dae604622680c534d8/src/LuaTable.h#L310-L316

which print if there's no value for the given key improving (or I hope so) debugging...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pioneerspacesim/pioneer/issues/4667?email_source=notifications&email_token=AAEQ4NIDTPLHOLTL7OD5QRDQF2FTVA5CNFSM4IMNFIF2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD45A2PQ#issuecomment-523898174, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEQ4NOKVQEEIT3N5LEJAZ3QF2FTVANCNFSM4IMNFIFQ .

[Web-eWorks]

@laarmen maybe I'm reading this wrong, but I don't see a way to directly return a ScopedTable from LuaObject<T>::CallMethod() - not only does CallMethod delete the return value from the stack, but there's no pi_lua_generic_pull overload for ScopedTable or LuaTable for that matter. How are you thinking of rectifying this?

[mike-f1]

Is there an Option type in C++ these days ?

https://en.cppreference.com/w/cpp/utility/optional ...but it needs C++17, which is quite an(other) overkill...

My question is when an "empty object" wrapped in a LuaRef should be considered valid? ...Which is similar to your thought:

"why is the hyperdrive an empty table and not nil"

...because if there's no need for an empty object/table then that is a design (and logic) flaws...

@Web-eWorks , you should consider that that method is working if an hyperdrive is present: indeed it calls this function in order to retrieve a "valid" LuaRef...

Thus all this issue is about "how to handle error when C++ needs Lua", and "how to crash in your feet" (= giving right hints to devs and useful messages)

[laarmen]

I might have been talking out of my arse the entire conversation here, since I only had access to the code via Github, but yeah I foresaw this answer :-)

In this specific case, we could insert a copy of the table under all the return values in the _generic_pull() for ScopedTable. Sure, it would break the tacit invariant that we leave the stack the way we found it. I think ScopedTable wouldn't mind such a solution, I dunno about LuaObject though.

On Thu, Aug 22, 2019 at 6:52 PM Webster Sheets notifications@github.com wrote:

@laarmen maybe I'm reading this wrong, but I don't see a way to directly return a ScopedTable from LuaObject::CallMethod() - not only does CallMethod delete the return value from the stack, but there's no pi_lua_generic_pull overload for ScopedTable or LuaTable for that matter. How are you thinking of rectifying this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[Gliese852]

So, maybe we just won't call this function if we don't have a drive and check it in lua-side? Anyway, we will not be able to correctly calculate the optimal route, not knowing for which drive we are calculating.