Re: svn commit: r1912607 - in /apr/apr/trunk: ./ .github/workflows/ build/ dbm/ include/ include/private/ test/

On Fri, Sep 29, 2023 at 3:39 PM <[email protected]> wrote:
>
> Author: jorton
> Date: Fri Sep 29 13:39:11 2023
> New Revision: 1912607
[]
> --- apr/apr/trunk/dbm/apr_dbm_lmdb.c (added)
> +++ apr/apr/trunk/dbm/apr_dbm_lmdb.c Fri Sep 29 13:39:11 2023
[]
> +static apr_status_t vt_lmdb_open(apr_dbm_t **pdb, const char *pathname,
> +                                 apr_int32_t mode, apr_fileperms_t perm,
> +                                 apr_pool_t *pool)
> +{
[]
> +    {
> +        int dberr;
> +        file.txn = NULL;
> +        file.cursor = NULL;
> +
> +        if ((dberr = mdb_env_create(&file.env)) == 0) {
> +            //XXX: properly set db size
> +            if ((dberr = mdb_env_set_mapsize(file.env, UINT32_MAX)) == 0){
> +                if ((dberr = mdb_env_open(file.env, pathname, dbmode | DEFAULT_ENV_FLAGS, apr_posix_perms2mode(perm))) == 0) {
> +                    if ((dberr = mdb_txn_begin(file.env, NULL, dbmode, &file.txn)) == 0){
> +                        if ((dberr = mdb_dbi_open(file.txn, NULL, dbi_open_flags, &file.dbi)) != 0){
> +                            /* close the env handler */
> +                            mdb_env_close(file.env);
> +                        }
> +                    }
> +                }
> +            }
> +        }
> +
> +        if (truncate){
> +            if ((dberr = mdb_drop(file.txn, file.dbi, 0)) != 0){
> +                mdb_env_close(file.env);
> +            }
> +        }
> +
> +        if (dberr != 0)
> +            return db2s(dberr);
> +    }

This seems to potentially double-close or leak "file.env" on some
errors, maybe write it like the below instead?

    dberr = mdb_env_create(&file.env);
    if (dberr != 0) {
        return db2s(dberr);
    }
    dberr = ...;
    if (dberr == 0) {
        dberr = ...;
    }
    dberr = ...;
    if (dberr == 0) {
        dberr = ...;
    }
    ...
    if (dberr == 0) {
        dberr = mdb_dbi_open(file.txn, NULL, dbi_open_flags, &file.dbi);
        if (dberr == 0 && truncate) {
            dberr = mdb_drop(file.txn, file.dbi, 0);
            if (dberr != 0) {
                mdb_dbi_close(file.env, file.dbi);
            }
        }
    }
    if (dberr != 0) {
        mdb_env_close(file.env);
        return db2s(dberr);
    }

> +
> +    /* we have an open database... return it */
> +    *pdb = apr_pcalloc(pool, sizeof(**pdb));
> +    (*pdb)->pool = pool;
> +    (*pdb)->type = &apr_dbm_type_lmdb;
> +    (*pdb)->file = apr_pmemdup(pool, &file, sizeof(file));
> +
> +    /* ### register a cleanup to close the DBM? */
> +
> +    return APR_SUCCESS;
> +}
> +
> +static void vt_lmdb_close(apr_dbm_t *dbm)
> +{
> +    real_file_t *f = dbm->file;
> +
> +    if (f->cursor){
> +        mdb_cursor_close(f->cursor);
> +        f->cursor = NULL;
> +    }
> +
> +    if (f->txn){
> +       mdb_txn_commit(f->txn);
> +       f->txn = NULL;
> +    }

Shouldn't we abort the transaction here if there was no explicit
commit (store/del) previously?
In any case, the docs[1] for commit/abort seem to say that both
invalidate the ->cursor, so maybe we want to write this like:

   if (f->txn){
       mdb_txn_{commit,abort}(f->txn);
       f->txn = NULL;
       f->cursor = NULL;
   }
   if (f->cursor){
       mdb_cursor_close(f->cursor);
       f->cursor = NULL;
   }

?
[1] http://www.lmdb.tech/doc/group__mdb.html#ga846fbd6f46105617ac9f4d76476f6597

> +
> +    mdb_dbi_close(f->env, f->dbi);
> +    mdb_env_close(f->env);
> +
> +    f->env = NULL;
> +    f->dbi = 0;
> +}
[]
> +static apr_status_t vt_lmdb_store(apr_dbm_t *dbm, apr_datum_t key,
> +                                  apr_datum_t value)
> +{
> +    real_file_t *f = dbm->file;
> +    int rv;
> +    MDB_val ckey = { 0 };
> +    MDB_val cvalue = { 0 };
> +
> +    ckey.mv_data = key.dptr;
> +    ckey.mv_size = key.dsize;
> +
> +    cvalue.mv_data = value.dptr;
> +    cvalue.mv_size = value.dsize;
> +
> +    if ((rv = mdb_put(f->txn, f->dbi, &ckey, &cvalue, 0)) == 0) {
> +        /* commit transaction */
> +        if (((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS)
> +            && ((rv = mdb_txn_begin(f->env, NULL, 0, &f->txn)) == MDB_SUCCESS)) {
> +            f->cursor = NULL;
> +        }

AIUI, f->cursor should be reset whether or not mdb_txn_begin()
succeeds here, and f->txn if any of _commit() or _begin() fails?

> +    }
> +
> +    /* store any error info into DBM, and return a status code. */
> +    return set_error(dbm, rv);
> +}
> +
> +static apr_status_t vt_lmdb_del(apr_dbm_t *dbm, apr_datum_t key)
> +{
> +    real_file_t *f = dbm->file;
> +    int rv;
> +    MDB_val ckey = { 0 };
> +
> +    ckey.mv_data = key.dptr;
> +    ckey.mv_size = key.dsize;
> +
> +    if ((rv = mdb_del(f->txn, f->dbi, &ckey, NULL)) == 0) {
> +        /* commit transaction */
> +        if (((rv = mdb_txn_commit(f->txn)) == MDB_SUCCESS)
> +            && ((rv = mdb_txn_begin(f->env, NULL, 0, &f->txn)) == MDB_SUCCESS)) {
> +            f->cursor = NULL;
> +        }

Likewise for f->cursor and f->txn resetting.

> +    }
> +
> +    /* store any error info into DBM, and return a status code. */
> +    return set_error(dbm, rv);
> +}
[]
> +static apr_status_t vt_lmdb_firstkey(apr_dbm_t *dbm, apr_datum_t * pkey)
> +{
> +    real_file_t *f = dbm->file;
> +    MDB_val first, data;
> +    int dberr;
> +
> +    if ((dberr = mdb_cursor_open(f->txn, f->dbi, &f->cursor)) == 0) {
> +        dberr = mdb_cursor_get(f->cursor, &first, &data, MDB_FIRST);
> +        if (dberr == MDB_NOTFOUND) {
> +            memset(&first, 0, sizeof(first));
> +            mdb_cursor_close(f->cursor);
> +            f->cursor = NULL;
> +            dberr = 0;
> +        }
> +    }

Clear/memset "first" if mdb_cursor_open() fails too?

> +
> +    pkey->dptr = first.mv_data;
> +    pkey->dsize = first.mv_size;
> +
> +    /* store any error info into DBM, and return a status code. */
> +    return set_error(dbm, dberr);
> +}
> +
> +static apr_status_t vt_lmdb_nextkey(apr_dbm_t *dbm, apr_datum_t * pkey)
> +{
> +    real_file_t *f = dbm->file;
> +    MDB_val ckey, data;
> +    int dberr;
> +
> +    ckey.mv_data = pkey->dptr;
> +    ckey.mv_size = pkey->dsize;
> +
> +    if (f->cursor == NULL){
> +        return APR_EINVAL;
> +    }
> +
> +    dberr = mdb_cursor_get(f->cursor, &ckey, &data, MDB_NEXT);
> +    if (dberr == MDB_NOTFOUND) {
> +        mdb_cursor_close(f->cursor);
> +        f->cursor = NULL;
> +        dberr = 0;
> +        ckey.mv_data = NULL;
> +        ckey.mv_size = 0;
> +    }
> +
> +    pkey->dptr = ckey.mv_data;
> +    pkey->dsize = ckey.mv_size;
> +
> +    /* store any error info into DBM, and return a status code. */
> +    return set_error(dbm, dberr);
> +}


Regards;
Yann.