Segfault when using methods after :finish

[daurnimator]

I just realised that after calling :finish, *conn is NULL. This isn't checked before calling any of the libpq functions.

e.g. this segfaults:

pg = require"pgsql"; 
conn = pg.connectdb("dbname=test")
conn:finish()
conn:isnonblocking()

I propose the creation of a helper function that validates a connection object (untested):

static PGconn*
pgsql_checkconn(lua_State *L, int n) {
    PGconn** conn = luaL_checkudata(L, n, CONN_METATABLE);
    luaL_argcheck(L, NULL != *conn, n, "connection pointer is NULL");
    return *conn;
}

[mbalmer]

You must not use the connection object after finish() has been called on it. That is documented: "The connection object must not be used again after finish has been called." (the same phrase is found in the libpq() documentation, btw).

It is set to NULL so that when the userdata object is later garbage collected, PQfinish() is not called a second time (if :finish() had not been called, *conn is not NULL and the garbage collection function calls PQfinish()).

That said, I don't think it is necessary to add a check for *conn not being NULL in every call.

[daurnimator]

IMO absolutely no lua function (excluding debug library and ffi) should ever cause lua to segfault.

[mbalmer]

I agree that segfaulting in a library is not really very user friendly ;)

[mbalmer]

I just committed a version with a function pgsql_conn() that very similar to the one you suggested. If that works for you as well, I will tag it as 1.4.3 and start addressing the API refinement issues (i.e. returning nil where appropriate etc.).

So far I see no regression on our systems.

[daurnimator]

I just committed a version with a function pgsql_conn() that very similar to the one you suggested. If that works for you as well, I will tag it as 1.4.3 and start addressing the API refinement issues (i.e. returning nil where appropriate etc.).

So far I see no regression on our systems.

Looks reasonable to me. I won't be able to run a full test until I'm back at work on Monday.

But don't let my testing block you fixing other things :)

[daurnimator]

Seems to be fixed :D Closed with f512765bbb3aaf78a056e183cfcc9cc353235bfc

[daurnimator]

btw, could you add some method that return whether the connection has been :finish()'d yet?

[mbalmer]

Am 25.02.15 um 19:02 schrieb daurnimator:

btw, could you add some method that return whether the connection has been |:finish()|'d yet?

conn:status() could return nil in this case, what do you think?

[daurnimator]

conn:status() could return nil in this case, what do you think?

mmmm, doesn't feel right. In C PQstatus(NULL) an error message like "connection object is NULL". returning nil in lua is a bit too far from the C behaviour IMO (at least for a library like luapgsql that follows the C api closely).

[mbalmer]

Am 25.02.15 um 19:14 schrieb daurnimator:

conn:status() could return nil in this case, what do you think?

mmmm, doesn't feel right. In C |PQstatus(NULL)| an error message like "connection object is NULL". returning nil in lua is a bit too far from the C behaviour IMO (at least for a library like luapgsql that follows the C api closely).

Well, you get a Lua error anyways when using a finished() connection, so you can pcall() and be safe. And a connection does not get finished() just so, it must be the program that finishes it.

[daurnimator]

you can pcall() and be safe.

Fair enough.

[mbalmer]

Am 25.02.15 um 19:14 schrieb daurnimator:

conn:status() could return nil in this case, what do you think?

mmmm, doesn't feel right.

I tend to disagree slightly. returning nil would clearly indicate that there is no status at all. Unlinke in C, where passing NULL to PQstatus() yields an error, we are here calling :status() on a valid Lua-PostgreSQL-connection-object. Just the underlying connection is gone.

but we can also rely on the fact that any function will now create a Lua error and that pcall can catch that.