Potential leak in conn_execParams

[daurnimator]

https://github.com/arcapos/luapgsql/blob/328f6cb89b588775be8a598bb86040fd2fa0a4ac/luapgsql.c#L529 luaL_getmetatable can throw which result in frees being missed.

Fix should be to hoist all the freeing above the luaL_getmetatable call.

Same issue in conn_prepare, conn_execPrepared

[mbalmer]

conn_execParams could look like this with the helper function gcmalloc() and gcfree():

static int
conn_execParams(lua_State *L)
{
    PGresult **res;
    int n, nParams, sqlParams, count;

    nParams = lua_gettop(L) - 2;    /* subtract connection and command */
    if (nParams < 0)
        nParams = 0;

    for (n = 0, sqlParams = 0; n < nParams; n++) {
        get_sql_params(L, 3 + n, sqlParams, NULL, NULL, NULL, NULL,
            &count);
        sqlParams += count;
    }
    res = lua_newuserdata(L, sizeof(PGresult *));
    if (sqlParams) {
        Oid **paramTypes;
        char ***paramValues;
        int **paramLengths, **paramFormats;

        paramTypes = (Oid **)gcmalloc(L, sizeof(Oid *));
        paramValues = (char ***)gcmalloc(L, sizeof(char **));
        paramLengths = (int **)gcmalloc(L, sizeof(int *));
        paramFormats = (int **)gcmalloc(L, sizeof(int *));

        *paramTypes = calloc(sqlParams, sizeof(Oid));
        *paramValues = calloc(sqlParams, sizeof(char *));
        *paramLengths = calloc(sqlParams, sizeof(int));
        *paramFormats = calloc(sqlParams, sizeof(int));

        if (*paramTypes == NULL || *paramValues == NULL
            || *paramLengths == NULL || *paramFormats == NULL)
                return luaL_error(L, "out of memory");

        for (n = 0, sqlParams = 0; n < nParams; n++) {
            if (get_sql_params(L, 3 + n, sqlParams, *paramTypes,
                *paramValues, *paramLengths, *paramFormats, &count))
                    return luaL_error(L, "out of memory");
            sqlParams += count;
        }
        *res = PQexecParams(pgsql_conn(L, 1),
            luaL_checkstring(L, 2), sqlParams, *paramTypes,
            (const char * const*)*paramValues, *paramLengths,
            *paramFormats, 0);
        for (n = 0; n < sqlParams; n++)
            free((void *)*paramValues[n]);
        gcfree((void **)paramTypes);
        gcfree((void **)paramValues);
        gcfree((void **)paramLengths);
        gcfree((void **)paramFormats);
    } else
        *res = PQexecParams(pgsql_conn(L, 1), luaL_checkstring(L, 2),
            0, NULL, NULL, NULL, NULL, 0);
    luaL_getmetatable(L, RES_METATABLE);
    lua_setmetatable(L, -2);
    return 1;
}

So all the memory dance is only done if we have parameters.

[mbalmer]

It can be extended, in this version the helper functions behave more like malloc(), calloc() etc:

/*
 * Garbage collected memory
 */
static void **
gcalloc(lua_State *L)
{
    void **p;
    p = lua_newuserdata(L, sizeof(void *));
    *p = NULL;
    luaL_setmetatable(L, GCMEM_METATABLE);
    return p;
}

static void **
gcmalloc(lua_State *L, size_t size)
{
    void **p;
    p = lua_newuserdata(L, sizeof(void *));
    *p = malloc(size);
    luaL_setmetatable(L, GCMEM_METATABLE);
    return p;
}

static void **
gccalloc(lua_State *L, size_t nmemb, size_t size)
{
    void **p;
    p = lua_newuserdata(L, sizeof(void *));
    *p = calloc(nmemb, size);
    luaL_setmetatable(L, GCMEM_METATABLE);
    return p;
}

/* Memory can be free'ed immediately or left to the garbage collector */
static void
gcfree(void **p)
{
    free(*p);
    *p = NULL;
}

static int
gcmem_clear(lua_State *L)
{
    void **p = luaL_checkudata(L, 1, GCMEM_METATABLE);
    PQfreemem(*p);
    *p = NULL;
    return 0;
}

If a pointer is allocated by a third party library, we use gcalloc() as follows:

static int
pgsql_encryptPassword(lua_State *L)
{
    void **pw;

    pw = gcalloc(L);
    *pw = PQencryptPassword(luaL_checkstring(L, 1), luaL_checkstring(L, 2));
    lua_pushstring(L, *pw);
    gcfree(pw);
    return 1;
}

We allocate the memory ourselves, we can use gcmalloc() or gccalloc() which similar to their libc counterparts, but return a pointer to a pointer:

static int
conn_execParams(lua_State *L)
{
    PGresult **res;
    int n, nParams, sqlParams, count;

    nParams = lua_gettop(L) - 2;    /* subtract connection and command */
    if (nParams < 0)
        nParams = 0;

    for (n = 0, sqlParams = 0; n < nParams; n++) {
        get_sql_params(L, 3 + n, sqlParams, NULL, NULL, NULL, NULL,
            &count);
        sqlParams += count;
    }
    res = lua_newuserdata(L, sizeof(PGresult *));
    if (sqlParams) {
        Oid **paramTypes;
        char ***paramValues;
        int **paramLengths, **paramFormats;

        paramTypes = (Oid **)gccalloc(L, sqlParams, sizeof(Oid));
        paramValues = (char ***)gccalloc(L, sqlParams, sizeof(char *));
        paramLengths = (int **)gccalloc(L, sqlParams, sizeof(int));
        paramFormats = (int **)gccalloc(L, sqlParams, sizeof(int));

        if (*paramTypes == NULL || *paramValues == NULL
            || *paramLengths == NULL || *paramFormats == NULL)
                return luaL_error(L, "out of memory");

        for (n = 0, sqlParams = 0; n < nParams; n++) {
            if (get_sql_params(L, 3 + n, sqlParams, *paramTypes,
                *paramValues, *paramLengths, *paramFormats, &count))
                    return luaL_error(L, "out of memory");
            sqlParams += count;
        }
        *res = PQexecParams(pgsql_conn(L, 1),
            luaL_checkstring(L, 2), sqlParams, *paramTypes,
            (const char * const*)*paramValues, *paramLengths,
            *paramFormats, 0);
        for (n = 0; n < sqlParams; n++)
            free((void *)*paramValues[n]);
        gcfree((void **)paramTypes);
        gcfree((void **)paramValues);
        gcfree((void **)paramLengths);
        gcfree((void **)paramFormats);
    } else
        *res = PQexecParams(pgsql_conn(L, 1), luaL_checkstring(L, 2),
            0, NULL, NULL, NULL, NULL, 0);
    luaL_getmetatable(L, RES_METATABLE);
    lua_setmetatable(L, -2);
    return 1;
}

[mbalmer]

And if gcmalloc()and 'gccalloc()check the memory allocation,conn_execParams()` gets even simpler:

static void **
static void **
gcmalloc(lua_State *L, size_t size)
{
    void **p;
    p = lua_newuserdata(L, sizeof(void *));
    if ((*p = (void *)malloc(size)) == NULL)
        luaL_error(L, "out of memory");
    luaL_setmetatable(L, GCMEM_METATABLE);
    return p;
}

static void **
gccalloc(lua_State *L, size_t nmemb, size_t size)
{
    void **p;
    p = lua_newuserdata(L, sizeof(void *));
    if ((*p = (void *)calloc(nmemb, size)) == NULL)
        luaL_error(L, "out of memory");
    luaL_setmetatable(L, GCMEM_METATABLE);
    return p;
}

static int
conn_execParams(lua_State *L)
{
    PGresult **res;
    int n, nParams, sqlParams, count;

    nParams = lua_gettop(L) - 2;    /* subtract connection and command */
    if (nParams < 0)
        nParams = 0;

    for (n = 0, sqlParams = 0; n < nParams; n++) {
        get_sql_params(L, 3 + n, sqlParams, NULL, NULL, NULL, NULL,
            &count);
        sqlParams += count;
    }
    res = lua_newuserdata(L, sizeof(PGresult *));
    if (sqlParams) {
        Oid **paramTypes;
        char ***paramValues;
        int **paramLengths, **paramFormats;

        paramTypes = (Oid **)gccalloc(L, sqlParams, sizeof(Oid));
        paramValues = (char ***)gccalloc(L, sqlParams, sizeof(char *));
        paramLengths = (int **)gccalloc(L, sqlParams, sizeof(int));
        paramFormats = (int **)gccalloc(L, sqlParams, sizeof(int));

        for (n = 0, sqlParams = 0; n < nParams; n++) {
            if (get_sql_params(L, 3 + n, sqlParams, *paramTypes,
                *paramValues, *paramLengths, *paramFormats, &count))
                    return luaL_error(L, "out of memory");
            sqlParams += count;
        }
        *res = PQexecParams(pgsql_conn(L, 1),
            luaL_checkstring(L, 2), sqlParams, *paramTypes,
            (const char * const*)*paramValues, *paramLengths,
            *paramFormats, 0);
        for (n = 0; n < sqlParams; n++)
            free((void *)*paramValues[n]);
        gcfree((void **)paramTypes);
        gcfree((void **)paramValues);
        gcfree((void **)paramLengths);
        gcfree((void **)paramFormats);
    } else
        *res = PQexecParams(pgsql_conn(L, 1), luaL_checkstring(L, 2),
            0, NULL, NULL, NULL, NULL, 0);
    luaL_getmetatable(L, RES_METATABLE);
    lua_setmetatable(L, -2);
    return 1;
}

[daurnimator]

You don't need the gccalloc. You can just use a user data with no metatable instead of the calloc in the first place.

[mbalmer]

actually the result must be created last, or we end up with a gcmem object on the stack, so it should be like this (tested):

static int
conn_execParams(lua_State *L)
{
    PGresult **ures, *res;
    int n, nParams, sqlParams, count;

    nParams = lua_gettop(L) - 2;    /* subtract connection and command */
    if (nParams < 0)
        nParams = 0;

    for (n = 0, sqlParams = 0; n < nParams; n++) {
        get_sql_params(L, 3 + n, sqlParams, NULL, NULL, NULL, NULL,
            &count);
        sqlParams += count;
    }

    if (sqlParams) {
        Oid **paramTypes;
        char ***paramValues;
        int **paramLengths, **paramFormats;

        paramTypes = (Oid **)gccalloc(L, sqlParams, sizeof(Oid));
        paramValues = (char ***)gccalloc(L, sqlParams, sizeof(char *));
        paramLengths = (int **)gccalloc(L, sqlParams, sizeof(int));
        paramFormats = (int **)gccalloc(L, sqlParams, sizeof(int));

        for (n = 0, sqlParams = 0; n < nParams; n++) {
            if (get_sql_params(L, 3 + n, sqlParams, *paramTypes,
                *paramValues, *paramLengths, *paramFormats, &count))
                    return luaL_error(L, "out of memory");
            sqlParams += count;
        }
        res = PQexecParams(pgsql_conn(L, 1),
            luaL_checkstring(L, 2), sqlParams, *paramTypes,
            (const char * const*)*paramValues, *paramLengths,
            *paramFormats, 0);
        for (n = 0; n < sqlParams; n++)
            free((void *)*paramValues[n]);
        gcfree((void **)paramTypes);
        gcfree((void **)paramValues);
        gcfree((void **)paramLengths);
        gcfree((void **)paramFormats);
    } else
        res = PQexecParams(pgsql_conn(L, 1), luaL_checkstring(L, 2),
            0, NULL, NULL, NULL, NULL, 0);

    ures = lua_newuserdata(L, sizeof(PGresult *));
    *ures = res;
    luaL_getmetatable(L, RES_METATABLE);
    lua_setmetatable(L, -2);

    return 1;
}

The gccalloc() is there to separate size and nmemb, to avoid overflow (just like calloc). calling lua_newuserdate(L, size * nmemb) can lead problems. But the gcmalloc() is not needed, indeed.

[mbalmer]

Actually the maximum number of allowed parameters is 65535, so it should not overflow here, we can check that limit and switch to lua_newuserdata() (and get rid of gccalloc() indeed).

[mbalmer]

This has now been changed to use lua_newuserdata() instead of calloc(). So we leave the cleaning up to Lua's garbage collector.