Potential leak in encryptPassword

[daurnimator]

lua_pushstring can throw, which will result in PQfreemem(encrypted); never getting called. https://github.com/arcapos/luapgsql/blob/328f6cb89b588775be8a598bb86040fd2fa0a4ac/luapgsql.c#L136

Same issue in conn_escapeString, conn_escapeLiteral, conn_escapeIdentifier, conn_escapeBytea, conn_unescapeBytea, conn_getCopyData

Similar issue in conn_cancel

[mbalmer]

What extactly do you mean by "can throw"? What will happen in such a case (which, btw, is not documented), will it call err() or exit()? Or longjmp() (where to?)

[daurnimator]

What extactly do you mean by "can throw"? What will happen in such a case (which, btw, is not documented), will it call err() or exit()? Or longjmp() (where to?)

"can throw" in the lua C API means "can longjmp out"

It is documented in the lua manual: lua_pushstring is annotated with 'm', which (as mentioned in section 4.8) means: "the function may raise memory errors".

---

One solution is to set up garbage collection on the to-be-freed string:

#define FREEMEM__GC_MT "freemem_gc"

static int freemem__gc(lua_State *L) {
    void *mem = luaL_checkudata(L, 1, FREEMEM__GC_MT);
    if (*mem != NULL) {
        PQfreemem(*mem);
        *mem = NULL;
    }
    return 0;
}

static int
pgsql_encryptPassword(lua_State *L)
{
    /* argument check *first* so that invalid input is caught */
    const char* passwd = luaL_checkstring(L, 1);
    const char* user = luaL_checkstring(L, 2);
    /* temporary userdata to make sure `encrypted` doesn't leak */
    char** encrypted = lua_newuserdata(L, sizeof(char*));
    *encrypted = NULL; /* make sure it's initialised to 0 */
    luaL_setmetatable(L, FREEMEM__GC_MT);
    /* make the call */
    *encrypted = PQencryptPassword(passwd, user);
    if (*encrypted != NULL) {
        lua_pushstring(L, *encrypted);
        /* (optional): cleanup as early as possible */
        PQfreemem(*encrypted);
        *encrypted = NULL; /* don't forget to invalidate the userdata for collection */
    } else
        lua_pushnil(L);
    return 1;
}

/* In initialisation set up FREEMEM__GC_MT */
    luaL_newmetatable(L, FREEMEM__GC_MT);
    lua_pushcfunction(L, freemem__gc);
    lua_setfield(L, -2, "__gc");

[mbalmer]

Not sure if it is really worth going that far for every small allocation. Especially since the default behaviour of Lua is to terminate the calling program in case of an error anyways.

[daurnimator]

Especially since the default behaviour of Lua is to terminate the calling program in case of an error anyways.

Not it's not... It goes up to the nearest pcall (or coroutine.resume).

[mbalmer]

I still think this is cluttering the code for no real gain. Do such memory errors happen in real live or are we trying to fix a hypothetical problem here?

[daurnimator]

I still think this is cluttering the code for no real gain. Do such memory errors happen in real live or are we trying to fix a hypothetical problem here?

It's as likely as malloc failing. i.e. it depends on your operating system, how it's configured and your libc. When I have a memory leak in my lua programs I often have few options other than to go through all the C libraries we use to look for culprits. In this case: I found several in luapgsql.

[mbalmer]

I could think of a set of small, reusable functions, this adds a little overhead, but the code remains readable:

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

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

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

Then later use them like you suggested:

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

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

Note that the call to gcfree() is purely optional, but it's probably a good thing to immediately free memory if we can.

[siffiejoe]

I have used a similar set of helper functions, and I think that not handling memory allocation errors is definitely a code-smell -- especially since the Lua team goes to great lengths to ensure that Lua itself does the right thing when malloc fails. This effort is wasted if some extension module author chooses not to bother.

[mbalmer]

So you suggest to try to replace all occurences of malloc etc. by lua_newuserdata, to make every allocation, and be it small, garbage collectable ?

[siffiejoe]

Yes. The only exception I can think of is as part of another finalizable userdata.

You can add stack-based allocations for small memory chunks as an optimization later if necessary (which I doubt will be the case):

char buffer[ REASONABLE_SIZE ];
char* ptr = buffer;
if( n > sizeof( buffer ) )
  ptr = lua_newuserdata( L, n );
/* use ptr */

(This is pretty much equivalent to using the luaL_Buffer API as Tomas suggested on lua-l.)

[mbalmer]

This issue has now been fixed. It uses the mechanism daurnimator suggested (factored out in some small functions, so it can be easily reused(.