Callbacks need to be longjmp safe

[daurnimator]

Callback functions (on_connect, on_message, etc.) are called from non-lua code; which I cannot see as documented to be longjmp safe.

Lua C api functions will longjmp out on failure to the last pcall. To fix, your lua function invocations should use lua_pcall.

Additionally, other functions such as lua_pushstring can longjmp out on memory allocation failure. https://github.com/flukso/lua-mosquitto/blob/aa2e0b723bf46139f11ede33b876e7f6411f1382/lua-mosquitto.c#L555

These should also be called from inside a pcall (push a pointer on the stack instead and then inside the pcall use lua_pushstring).

[daurnimator]

Callback functions (on_connect, on_message, etc.) are called from non-lua code; which I cannot see as documented to be longjmp safe.

I had a look into the libmosquitto code base, and it's evident that longjmping out is unsafe, as the cleanup code here: https://github.com/eclipse/mosquitto/blob/8025f5a29b78551e1d5e9ea13ae9dacabb6830da/lib/net_mosq.c#L1142-L1147 would get skipped (amoung other issues).

The issue in coming up with a fix is that libmosquitto doesn't provide any way to report errors from callbacks; and will immediately process the next packet.

I think this means that errors in a user's lua callback also have to be silent (as we have nowhere to report them to). Sending PR with this fix.

[karlp]

ok, I'm of the opinion now, as I was back then, that user callbacks should not be pcalled, because, as you mention yourself, this means that errors in user code are silently discarded. See https://github.com/flukso/lua-mosquitto/commit/57634bc57b8ab6c3d425b68cc6f2e690f34661af

I strongly feel this is the domain of the user to handle properly.

[daurnimator]

They must be pcalled, to do otherwise would break libmosquitto in a way that the only solution is to abort() the process.

The open question is: what to do when pcall indicates an error: my current PR does nothing (throwing the error away). Instead you may want to get it back to the caller (though libmosquitto doesn't make it easy).

[karlp]

how does this break:

function realhandler(mid, topic, payload, qos, retain)
   blah
end
mqtt:set_callback(mosq.ON_MESSAGE, function(mid, topic, payload, qos, retain)
   local ok, err = pcall(realhandler, mid, topic, payload, qos, retain)
 end)

Because you want to do an awful lot of extra C code just to throw away errors. The code above makes it absolutely user choice how to handle the errors, and correctly does the pcall.

Alternatively, you could try and just reverting https://github.com/flukso/lua-mosquitto/commit/57634bc57b8ab6c3d425b68cc6f2e690f34661af and adding an api call that provides some sort of pcall handler for the case when it fails, but that sounds like a complicated user API for no gain.

[daurnimator]

Because when you call lua_pushstring (to push e.g. payload) when memory constrained, lua may longjmp out due to memory allocation failure. libmosquitto does not handle such a thing safely (and is not documented to).