[FFmpeg-devel] [PATCH 2/8] ffserver: Implement lua config file reader

[Stephan Holljes]

On Mon, May 21, 2018 at 3:04 PM, James Darnley <james.darnley at gmail.com> wrote:
> On 2018-05-20 20:53, Stephan Holljes wrote:
>> +#include <lua5.3/lua.h>
>> +#include <lua5.3/lauxlib.h>
>> +#include <lua5.3/lualib.h>
>
> That's not portable.  Lua headers are not in a subdirectory.

Yes, artifact from early testing, changed and tested, pkg-config adds
the appropriate include directories.

>
>> +int configs_read(struct HTTPDConfig **configs, const char *filename)
>> +{
>> +    int ret = 0;
>> +    int nb_configs = 0;
>> +    int nb_streams = 0;
>> +    int nb_formats = 0;
>> +    int i;
>> +    int index = 0;
>> +    const char *key, *error;
>> +    struct HTTPDConfig *parsed_configs = NULL;
>> +    struct HTTPDConfig *config;
>> +    struct StreamConfig *stream;
>> +    lua_State *L = luaL_newstate();
>> +    ret = luaL_loadfile(L, filename);
>> +    if (ret != 0) {
>> +        fprintf(stderr, "Unable to open config file: %s\n", lua_tostring(L, -1));
>> +        lua_close(L);
>> +        return -1;
>> +    }
>> +
>> +    ret = lua_pcall(L, 0, 0, 0);
>> +
>> +    if (ret != 0) {
>> +        fprintf(stderr, "Unable to read config file: %s\n", lua_tostring(L, -1));
>> +        lua_close(L);
>> +        return -1;
>> +    }
>> +    lua_getglobal(L, "settings");
>> +    if (lua_type(L, -1) != LUA_TTABLE) {
>> +        lua_pushstring(L, "Error \"settings\" is not a table");
>> +        goto fail;
>> +    }
>> +    lua_pushnil(L);
>> +
>> +    // iterate servers
>> +    while (lua_next(L, -2) != 0) {
>> +        nb_configs++;
>> +        parsed_configs = av_realloc(parsed_configs, nb_configs * sizeof(struct HTTPDConfig));
>> +        config = &parsed_configs[nb_configs - 1];
>> +        config->server_name = NULL;
>> +        config->bind_address = NULL;
>> +        config->port = 0;
>> +        config->accept_timeout = 1000;
>> +        config->streams = NULL;
>> +        config->nb_streams = 0;
>> +        if (lua_type(L, -2) != LUA_TSTRING) {
>> +            lua_pushstring(L, "Error server name is not a string.");
>> +            goto fail;
>> +        }
>> +        if (lua_type(L, -1) != LUA_TTABLE) {
>> +            lua_pushstring(L, "Error server settings is not a table.");
>> +            goto fail;
>> +        }
>> +        config->server_name = av_strdup(lua_tostring(L, -2));
>> +        lua_pushnil(L);
>> +        // iterate server properties
>> +        nb_streams = 0;
>> +        while(lua_next(L, -2) != 0) {
>> +            if (lua_type(L, -2) != LUA_TSTRING) {
>> +                lua_pushstring(L, "Error server property is not a string.");
>> +                goto fail;
>> +            }
>> +            key = lua_tostring(L, -2);
>> +            if (!strncmp("bind_address", key, 12)) {
>> +                config->bind_address = av_strdup(lua_tostring(L, -1));
>> +            } else if (!strncmp("port", key, 4)) {
>> +                config->port = (int) lua_tonumber(L, -1);
>> +            } else {
>> +                // keys that are not "bind_address" or "port" are streams
>> +                if (lua_type(L, -1) != LUA_TTABLE) {
>> +                    lua_pushstring(L, "Error Stream configuration is not a table.");
>> +                    goto fail;
>> +                }
>> +
>> +                nb_streams++;
>> +                config->streams = av_realloc(config->streams, nb_streams * sizeof(struct StreamConfig));
>> +                stream = &config->streams[nb_streams - 1];
>> +                stream->input_uri = NULL;
>> +                stream->formats = NULL;
>> +                stream->stream_name = av_strdup(lua_tostring(L, -2));
>> +                lua_pushnil(L);
>> +                while(lua_next(L, -2) != 0) {
>> +                    if (lua_type(L, -2) != LUA_TSTRING) {
>> +                        lua_pushstring(L, "Error stream property is not a string.");
>> +                        goto fail;
>> +                    }
>> +                    key = lua_tostring(L, -2);
>> +                    if (!strncmp("input", key, 5)) {
>> +                        stream->input_uri = av_strdup(lua_tostring(L, -1));
>> +                    } else if (!strncmp("formats", key, 7)) {
>> +                        index = 1;
>> +                        nb_formats = 0;
>> +                        lua_pushnumber(L, index);
>> +                        while(1) {
>> +                            lua_gettable(L, -2);
>> +                            if (lua_isnil(L, -1))
>> +                                break;
>> +                            if (lua_type(L, -1) != LUA_TSTRING) {
>> +                                lua_pushstring(L, "Error format name is not a string.");
>> +                                goto fail;
>> +                            }
>> +                            stream->formats = av_realloc(stream->formats,
>> +                                                         (nb_formats + 1) * sizeof(enum StreamFormat));
>> +                            key = lua_tostring(L, -1);
>> +                            if (!strncmp("mkv", key, 3)) {
>> +                                stream->formats[nb_formats++] = FMT_MATROSKA;
>> +                            } else {
>> +                                fprintf(stderr, "Warning unknown format (%s) in stream format configuration.\n",
>> +                                                                                                           key);
>> +                                av_realloc(stream->formats, nb_formats * sizeof(enum StreamFormat));
>> +                            }
>> +                            stream->nb_formats = nb_formats;
>> +                            lua_pop(L, 1);
>> +                            lua_pushnumber(L, ++index);
>> +                        }
>> +                        lua_pop(L, 1);
>> +
>> +                    } else {
>> +                        fprintf(stderr, "Warning unknown key (%s) in stream configuration.\n", key);
>> +                    }
>> +                    lua_pop(L, 1);
>> +                }
>> +            }
>> +            lua_pop(L, 1);
>> +        }
>> +        config->nb_streams = nb_streams;
>> +        lua_pop(L, 1);
>> +     }
>> +
>> +    lua_close(L);
>> +    *configs = parsed_configs;
>> +    return nb_configs;
>> +
>> +fail:
>> +    error = lua_tostring(L, -1);
>> +    fprintf(stderr, "%s\n", error);
>> +    lua_close(L);
>> +    for (i = 0; i < nb_configs; i++)
>> +        config_free(&parsed_configs[i]);
>> +    av_free(parsed_configs);
>> +    return -1;
>> +}
>
> Do you know what Lua does if any of these function calls raises an
> error?  I see you catching the error when reading the config file but
> all other use of Lua is not in a protected environment.

I've been wondering about this, but didn't look into it too deeply.

>
> Lua 5.3 manual, section 4.6:
>> If an error happens outside any protected environment, Lua calls a panic function (see lua_atpanic) and then calls abort, thus exiting the host application.
>
> Even worse: if someone sets the global variable "settings" to something
> that isn't a table then lua_next will immediately segfault and you will
> get no diagnostics.  I was surprised by this because I thought it would
> raise an error.

This should actually not happen since it is tested against LUA_TTABLE
before starting to read it. (All other variables that are extracted
from the stack are checked for the corresponding type as well.)

>
> Other than these significant issues it looks like a reasonable use of
> the Lua API.
>
> My suggestions:
> 1 - Move all the Lua use into a lua_Cfunction and pcall it.
> 2 - Use that to raise a Lua error when checking types.

Thanks, this sounds very reasonable and I have already started reading
the documentation on how to make use of that.

I have also locally fixed the other things addressed (AVERROR usage
and alloc and thread creation checks), but I have yet to test it works
properly.

Thanks again!
Stephan