search

good-place / tahani

Janet wrapper of the LevelDB C API
5 stars 0 forks source link

Memory leak on panic. #5

Closed andrewchambers closed 4 years ago

andrewchambers commented 4 years ago

Yo, saw this lib, there is a mistake here:

static void paniconerr(char *err) {
    if (err != NULL) {
        janet_panic(err);
    }
    free_err(err);
}

You are only freeing the error if it is NULL, which won't do anything. Otherwise you are leaking the error message if you were indeed meant to free it. (I didn't read the leveldb docs)

another bit of feedback, The null assignment here doesn't do anything.


static void free_err(char *err) {
    leveldb_free(err);
    err = NULL;
}
andrewchambers commented 4 years ago

Also:

  Db *db = initdb(name, conn, options);
  paniconerr(err);

This leaks db's, you should not panic before cleaning up the error in some way, or assigning it to an abstract value that will have it's GC called.

pepe commented 4 years ago

Thanks a lot. You can be sure, that my C-fu is minimal, mostly copied from janet-lang/sqlite3 and some LevelDB C examples :-). Will fix!

pepe commented 4 years ago

The first part I hope fixed. Now to the second one. Am I leaking DB, even if it is GCed?

andrewchambers commented 4 years ago

oh no you are right, the second one is my mistake lol.