elixir-sqlite / exqlite

An SQLite3 driver for Elixir
https://hexdocs.pm/exqlite
MIT License
217 stars 48 forks source link

Adds Sqlite3.release/2 for prepared statements #155

Closed warmwaffles closed 3 years ago

warmwaffles commented 3 years ago

This should in theory fix #154

warmwaffles commented 3 years ago

Well that was weird, compilation segfaulted

warmwaffles commented 3 years ago

I wonder if the segfault is from enif_release_resource(). When I run mix test on my local machine I do not get the segfault. I'll check if I can reproduce in a Docker container.

warmwaffles commented 3 years ago

For anyone following along, I haven't forgotten about this. Just been busy and haven't been able to get back to this.

warmwaffles commented 3 years ago

@kevinlang I figured out where the seg fault was coming from. I over released a resource. So once you call enif_make_resource that gives you an opaque pointer to fiddle with and then call enif_release_resource after to decrement the reference counter and hand it back to erlang to manage.

If you already released the resource and then later try to double release it like this

static ERL_NIF_TERM
exqlite_release(ErlNifEnv* env, int argc, const ERL_NIF_TERM argv[])
{
    assert(env);

    statement_t* statement = NULL;
    connection_t* conn     = NULL;

    if (argc != 2) {
        return enif_make_badarg(env);
    }

    if (!enif_get_resource(env, argv[0], connection_type, (void**)&conn)) {
        return make_error_tuple(env, "invalid_connection");
    }

    if (!enif_get_resource(env, argv[1], statement_type, (void**)&statement)) {
        return make_error_tuple(env, "invalid_statement");
    }

    if (statement->statement) {
        sqlite3_finalize(statement->statement);
        statement->statement = NULL;
    }
    //
    // KABOOM
    //
    enif_release_resource(statement);

    return make_atom(env, "ok");
}

Previously when the statement was initialized and ready to go we immediately called release resource once we had the opaque handle to return.

    rc = sqlite3_prepare_v3(conn->db, (char*)bin.data, bin.size, 0, &statement->statement, NULL);
    if (rc != SQLITE_OK) {
        enif_release_resource(statement);
        return make_sqlite3_error_tuple(env, rc, conn->db);
    }

    result = enif_make_resource(env, statement);
    enif_release_resource(statement);
warmwaffles commented 3 years ago

I still don't know of a good way to test for memory leaks.

One thing I think we probably need to explore is a process terminating unexpectedly, and seeing if that leaks memory.

kevinlang commented 3 years ago

Looks good! I also cannot think of a good way to test for memory leaks.

warmwaffles commented 3 years ago

The only thing I could think of is making a C test harness and just having it run valgrind checking for leaks, but that's pandora's box.