poga / actix-lua

Safe Lua Scripting Environment for Actix
MIT License
121 stars 13 forks source link

Typed functions panic on wrong argument types #22

Closed Arnaz87 closed 5 years ago

Arnaz87 commented 5 years ago

When lua calls rust defined functions with wrong argument types it panics the thread, it doesn't propagate the error for lua or rust to handle it, it straight up panics.

It may happen with any error returned by rust code called from lua handler code, but I don't know, I just saw it happen with wrong typed arguments.

naturallymitchell commented 5 years ago

(not meant to speak to this issue really, but static analysis of lua code is possible and potentially useful for other environmental programmability improvements)

Arnaz87 commented 5 years ago

Relevant lines in the stack backtrace

  17: <actix_lua::message::LuaMessage as rlua::value::FromLua<'lua>>::from_lua
             at .../.cargo/registry/src/github.com-1ecc6299db9ec823/actix-lua-0.5.1/src/message.rs:120
  18: actix_lua::actor::invoke::{{closure}}
             at .../.cargo/registry/src/github.com-1ecc6299db9ec823/actix-lua-0.5.1/src/actor.rs:224
  19: rlua::lua::Lua::scope
             at .../.cargo/registry/src/github.com-1ecc6299db9ec823/rlua-0.15.3/src/lua.rs:369
  20: actix_lua::actor::invoke
             at .../.cargo/registry/src/github.com-1ecc6299db9ec823/actix-lua-0.5.1/src/actor.rs:145
  21: <actix_lua::actor::LuaActor as actix::handler::Handler<actix_lua::message::LuaMessage>>::handle
             at .../.cargo/registry/src/github.com-1ecc6299db9ec823/actix-lua-0.5.1/src/actor.rs:286
poga commented 5 years ago

can you provide a minimum reproducible example?

Arnaz87 commented 5 years ago

https://gist.github.com/Arnaz87/550bee2ec9f59aae05952cceb8192878

It doesn't panic...

Arnaz87 commented 5 years ago

In the original code, this is the error message, I forgot to say it

thread 'arbiter:5037cb99-0989-4fb1-b855-945ba10943dc:actor' panicked at 'Lua error: FromLuaConversionError { from: "nil", to: "String", message: Some("expected string or number") }', .../.cargo/registry/src/github.com-1ecc6299db9ec823/actix-lua-0.5.1/src/message.rs:123:17

naturallymitchell commented 5 years ago

@Arnaz87 in your test crate, which function expects a typed input, and where does it get called with a different type? does it produce any error?

Arnaz87 commented 5 years ago

This commit has an instance of it happening in the code i'm working on, uuid.check, defined here, expects a string and if I pass a nil the thread panics.

Arnaz87 commented 5 years ago

I modified the gist to explain what is happening and what should happen, it's in main.lua

naturallymitchell commented 5 years ago

A bit more context:

In Lighttouch, built on Torchbear (our actix-lua implmentation), uuid.check broke with a nil input in a url checking condition. In https://github.com/foundpatterns/lighttouch-html-interface/commit/90ca60e617b8915a55f9815e8cd90a5b62d6ba34, Arnaud added a line to prevent passing nil as a parameter, because uuid.check expects a string, and if it's passed something else it panics.

These projects still need documentation, so I'm more than happy to answer any questions.

poga commented 5 years ago

It's kinda hard for me to understand what happened since I can't reproduce the panic with your example. :p

naturallymitchell commented 5 years ago

Arnaud struggled the whole day to distill it to a working test. I told him, then we need to at least give you something you can reproduce; that way we can either come back to it later with more info, or so that with it you can catch up to speed with the code built on top of yours and help. That's the best we have so far.

Could I interest you in a crash course in Lighttouch? I promise you'll enjoy it, or your money back! ;)

poga commented 5 years ago

Since I can't reproduce the panic with neither pure rlua nor actix-lua. Here's my suggestion to check:

  1. You forget to wrap a function call with pcall
  2. You're using pcall as pcall(function(args)). It should be pcall(function, args).
Arnaz87 commented 5 years ago

Here is a minimum reproducible example:

https://github.com/Arnaz87/Actix-lua-test

It copies the structure of the project with the bug. The error trigger is in main.lua.

poga commented 5 years ago

https://github.com/Arnaz87/Actix-lua-test/blob/master/main.lua#L11 You need to wrap the call to myfn with pcall.

Change it to local ok, ret = print(pcall(myfn,nil)) and the panic is gone.

*Edit: should be local ok, ret = pcall(myfn, nil).

Arnaz87 commented 5 years ago

No, it's already wrapped in an xpcall, change that line for error("My error") and see the error handler is triggered. The xpcall is Here.

When the error is from lua it works fine, but when it's from a rust function it skips the protected environment and straight up panics.

poga commented 5 years ago

At https://github.com/Arnaz87/Actix-lua-test/blob/master/handler.lua#L13. When the error happened in rust, rlua emit a rlua:Error. It got caught by your xpcall wrapper. Then, you store the rlua:Error to res and return it to rust.

actix-lua need to convert the rlua:Error to LuaMessage but it can't. Hence it panic.

error("My error") returns a string "My error" to pcall wrappers. Errors returned by rust is a rlua::Error. actix-lua knows how to deal with returned string but not rlua::Error.

Arnaz87 commented 5 years ago

Yes, you are right, this was the problem. I made a rust function that converts an error to a string. Thanks for helping me solve it.

It would be nice if rlua did that itself though.

Arnaz87 commented 5 years ago

Actually tostring(msg) does work on lua errors, that's the better solution.

poga commented 5 years ago

🎉