Issue670

Title Explosive LuaCode
Priority minor Status open
Assigned To Keywords
Linked issues Watchers fluzz

Submitted on 2013-05-08 02h01 by WeatherGod, last changed by flaviojs.

Files
File name Uploaded Type Edit Remove
Test.sav.gz flaviojs, 2018-04-17.04:46:22 application/gzip
Test.shp flaviojs, 2018-04-17.11:54:22 application/x-qgis
fix-issue-670.patch flaviojs, 2018-04-18.19:51:11 text/x-patch
print_lua_stack.c flaviojs, 2018-04-17.11:52:38 text/x-csrc
Messages
Author: WeatherGod Date: 2013-05-08   02h01
While playing, I found the following in my terminal:

FreedroidRPG 0.15-529-g93706c1 encountered a problem in function:  run_lua 
Error running Lua code: attempt to yield from outside a coroutine.
Erroneous LuaCode={
2        update_quest(_"An Explosive Situation", _"Not only is the town safe,
but apparently so is Ewald's 296 droid. Oh joy.")       
}

Note that the quest still closed, so this is a very minor issue.
Author: jesusalva Date: 2014-01-06   20h04
Right now, if it try update a closed quest, it should print to the terminal, ir
red, that the dialog tried to update a closed quest and continue the execution,
I think.
Author: matthiaskrgr Date: 2014-01-07   07h16
Jesuslava, that behaviour is intended (check map/script_helpers.lua).
It should be unrelated to the behaviour stated in the ticket.

Infrared, you made the quest didn't you (or was it Miles? I don't remember exactly)?
I'll assign this to, if you are unrelated to this, you can unassign again.
Author: flaviojs Date: 2018-04-17   04h44
My issue #913 is a duplicate of this issue, but with version 0.16-410-g7d8504e,
so I'll continue here.

I looked at 'lua_yieldk@lua/ldo.c' and without really understanding the
internals... my best guess is that the error is triggered when a coroutine
exists but is not currently running.

The error should come from the 'luaL_dostring' inside 'run_lua@src/lua.c',
called from 'complete_mission@src/mission.c',
called from '_complete_quest@src/lua/luaFD_tux.c',
called from 'end_quest@lua_modules/script_helpers.lua',
called from 'node30@data/storyline/act1/dialogs/Ewalds_296.lua'.

To know more, pretty_print_lua_error would need to print info about the stack
contents (of all coroutines?).
No idea how to do that... so I'll submit a save file right before the error (you
only need to speak with the droid).
Author: jesusalva Date: 2018-04-17   11h34
Problematic code removed in commit 94141d3d63370ad068c92f1c135ccf40900294bf

Issue may be no longer relevant.
Savefiles not verified (missing .shp file as well), thus, exact reason for bug 
is unknown.

This is the reason why I am not yet closing this issue.
Author: flaviojs Date: 2018-04-17   11h52
Making it print the stack was easier than expected, will submit the function as
attachment (each unique 'lua_State *L' is a different thread).
I called the function in 'run_lua' at the start, on error, and at the end.
This was the result of a trial run (load Test save file and talk to droid):

(... the Test save file was loaded ...)
START run_lua:
thread address 0x5622368
stack size 0
END run_lua:
thread address 0x5622368
stack size 0
(... dialog reached the Tux:end_quest call ...)
START run_lua:
thread address 0x5622368
stack size 1
[-1] thread:
 thread address 0x18481d8
 stack size 3
 [-1] string "I found Ewald's 296 alive and well in Ewald's bar! I was sure it
was destroyed in that explosion I heard, but it's safe and sound. Well, I guess
all's well that ends well."
 [-2] string "An Explosive Situation"
 [-3] userdata 0x1af9118
ERROR run_lua:
thread address 0x5622368
stack size 2
[-1] string "attempt to yield from outside a coroutine"
[-2] thread:
 thread address 0x18481d8
 stack size 3
 [-1] string "I found Ewald's 296 alive and well in Ewald's bar! I was sure it
was destroyed in that explosion I heard, but it's safe and sound. Well, I guess
all's well that ends well."
 [-2] string "An Explosive Situation"
 [-3] userdata 0x1af9118
(... error message from pretty_print_lua_error goes here ...)
END run_lua:
thread address 0x5622368
stack size 0


The requirements to trigger this appear to be:
1) use 'update_quest' inside the 'completion_code' of the quest being updated;
2) end the quest when the dialog is open.

It appears the code after the error was executed properly.
Author: flaviojs Date: 2018-04-17   11h54
Didn't realize the shp file was needed too, submited.
Author: flaviojs Date: 2018-04-18   16h36
The thread in the stack is the coroutine created inside 'prepare_lua_coroutine'
with the arguments: LUA_DIALOG "FDdialog" "run_node" "d" ...

A call to 'resume_lua_coroutine' happens after each line of the node code, maybe
to return from C code?

After adding "while (lua_isthread(L, -1)) L = lua_tothread(L, -1);" to 'run_lua'
the error changes to "attempt to yield across a C-call boundary".
Author: flaviojs Date: 2018-04-18   17h04
Replacing the contents of 'run_lua' with:

	struct lua_coroutine *co = load_lua_coroutine(target, code);
	while (!resume_lua_coroutine(co)) ;
	lua_pop(get_lua_state(target), 1);
	free(co);
	return 0;

solves the issue, but the return value is lost so it's not a solution.
Author: flaviojs Date: 2018-04-18   20h17
Submitted a fix that catches yields and simulates the behavior of luaL_dostring
by copying over the remaining data.

If the code yields then 'run_lua' will return LUA_YIELD, allowing the caller to
decide what comes next.
Currently the return value is always ignored, so a yield will be equivalent to a
return.

Note that I'm unsure if this is a proper fix because I don't know:
1) if the code is meant to interact with the target state or the coroutine that
is currently running;
2) what should happen to the yielded/returned data. Note that the data might be
"polluting" the stack of the target and be a cause of problems somewhere else.
Author: jesusalva Date: 2018-04-19   04h17
IIRC that bug was a code malpractice, 
there is a hard limitation that we can 
only stack two lua contexts or something 

Fluzz is better to explain it
Anyway, issue anymore present in next 
release.
Author: flaviojs Date: 2018-04-26   14h07
From what I can tell from the lua source, it allows a stack of 200 C-calls
unless compiled with a different limit.
The only catch is that any custom C-call must handle yields by itself (any
assumption on lua's part would limit what can be implemented).

With commit 94141d3d63370ad068c92f1c135ccf40900294bf this message won't appear
again, but a similar one is guaranteed to appear again until yields are handled
by all lua C-functions that execute other lua code.

My patch only results in yields being treated as returns.
I'm not sure if it's a proper fix because I can't grasp the intended use of lua,
can't even understand why there are two lua states.
History
Date User Action Args
2018-04-26 14:07:12flaviojssetmessages: + msg3580
2018-04-19 04:17:13jesusalvasetnosy: + fluzz, - infrared
messages: + msg3573
assignedto: infrared ->
2018-04-18 20:17:38flaviojssetmessages: + msg3572
2018-04-18 19:51:11flaviojssetfiles: + fix-issue-670.patch
2018-04-18 17:04:44flaviojssetmessages: + msg3571
2018-04-18 16:36:07flaviojssetmessages: + msg3570
2018-04-17 11:54:46flaviojssetmessages: + msg3569
2018-04-17 11:54:22flaviojssetfiles: + Test.shp
2018-04-17 11:52:38flaviojssetfiles: + print_lua_stack.c
2018-04-17 11:52:10flaviojssetmessages: + msg3568
2018-04-17 11:34:33jesusalvasetmessages: + msg3565
2018-04-17 04:46:22flaviojssetfiles: + Test.sav.gz
2018-04-17 04:44:43flaviojssetmessages: + msg3563
2018-04-16 21:28:45flaviojslinkissue913 linked
2014-01-07 07:16:08matthiaskrgrsetassignedto: infrared
nosy: + infrared
2014-01-07 07:16:02matthiaskrgrsetmessages: + msg2668
2014-01-06 20:04:04jesusalvasetmessages: + msg2648
2013-05-08 02:01:07WeatherGodcreate