cdb_fetch()/cdb_next_item() return immutable data

************************************************************************

Before now, the Berkeley DB back end was supplied the DB_DBT_MALLOC flag
and it became the caller's responsibility to handle any memory allocated
and returned by cdb_fetch() and cdb_next_item().  These heuristics are
not expected to be compatible with new back ends currently in planning.

cdb_fetch() and cdb_next_item() now return data that is to be considered
immutable (don't write to it or you'll segfault).  The caller must still
call cdb_free() to free the `struct cdbdata` itself, but the pointers
inside it point to buffers that the back end will reuse.

After a call to cdb_fetch() or cdb_next_item(), the buffers are
guaranteed to be readable until the next call to one of those functions
on the same table.

diff --git a/citadel/server/backends/berkeley_db/berkeley_db.c b/citadel/server/backends/berkeley_db/berkeley_db.c
index 62b8738f639c49a918c968183425e8af331a14c0..54d8dfecb123e7baa619b20ab14ace67c3133fad 100644 (file)
--- a/citadel/server/backends/berkeley_db/berkeley_db.c
+++ b/citadel/server/backends/berkeley_db/berkeley_db.c
@@ -34,6 +34,8 @@ static DB_ENV *bdb_env;               // The DB environment (global)
 struct bdb_tsd {
        DB_TXN *tid;            // Transaction handle
        DBC *cursors[MAXCDB];   // Cursors, for traversals...
+       DBT dbkey[MAXCDB];
+       DBT dbdata[MAXCDB];
 };
 
 
@@ -527,7 +529,7 @@ struct cdbdata *bdb_fetch(int cdb, const void *key, int keylen) {
        }
 
        struct cdbdata *tempcdb;
-       DBT dkey, dret;
+       DBT dkey;
        int ret;
 
        memset(&dkey, 0, sizeof(DBT));
@@ -535,18 +537,16 @@ struct cdbdata *bdb_fetch(int cdb, const void *key, int keylen) {
        dkey.data = (void *) key;
 
        if (TSD->tid != NULL) {
-               memset(&dret, 0, sizeof(DBT));
-               dret.flags = DB_DBT_MALLOC;
-               ret = bdb_table[cdb]->get(bdb_table[cdb], TSD->tid, &dkey, &dret, 0);
+               TSD->dbdata[cdb].flags = DB_DBT_REALLOC;
+               ret = bdb_table[cdb]->get(bdb_table[cdb], TSD->tid, &dkey, &TSD->dbdata[cdb], 0);
        }
        else {
                DBC *curs;
 
                do {
-                       memset(&dret, 0, sizeof(DBT));
-                       dret.flags = DB_DBT_MALLOC;
+                       TSD->dbdata[cdb].flags = DB_DBT_REALLOC;
                        curs = bdb_localcursor(cdb);
-                       ret = curs->c_get(curs, &dkey, &dret, DB_SET);
+                       ret = curs->c_get(curs, &dkey, &TSD->dbdata[cdb], DB_SET);
                        bdb_cclose(curs);
                } while (ret == DB_LOCK_DEADLOCK);
        }
@@ -566,8 +566,8 @@ struct cdbdata *bdb_fetch(int cdb, const void *key, int keylen) {
                bdb_abort();
        }
        else {
-               tempcdb->len = dret.size;
-               tempcdb->ptr = dret.data;
+               tempcdb->len = TSD->dbdata[cdb].size;
+               tempcdb->ptr = TSD->dbdata[cdb].data;
                bdb_decompress_if_necessary(tempcdb);
                return (tempcdb);
        }
@@ -575,15 +575,7 @@ struct cdbdata *bdb_fetch(int cdb, const void *key, int keylen) {
 
 
 // Free a cdbdata item.
-//
-// Note that we only free the 'ptr' portion if it is not NULL.  This allows
-// other code to assume ownership of that memory simply by storing the
-// pointer elsewhere and then setting 'ptr' to NULL.  bdb_free() will then
-// avoid freeing it.
 void bdb_free(struct cdbdata *cdb) {
-       if (cdb->ptr) {
-               free(cdb->ptr);
-       }
        free(cdb);
 }
 
@@ -620,16 +612,14 @@ void bdb_rewind(int cdb) {
 // Fetch the next item in a sequential search.  Returns a pointer to a 
 // cdbdata structure, or NULL if we've hit the end.
 struct cdbdata *bdb_next_item(int cdb) {
-       DBT key, data;
        struct cdbdata *cdbret;
        int ret = 0;
 
-       // Initialize the key/data pair so the flags aren't set.
-       memset(&key, 0, sizeof(key));
-       memset(&data, 0, sizeof(data));
-       data.flags = DB_DBT_MALLOC;
+       // reuse memory from the previous call.
+       TSD->dbkey[cdb].flags = DB_DBT_MALLOC;
+       TSD->dbdata[cdb].flags = DB_DBT_MALLOC;
 
-       ret = TSD->cursors[cdb]->c_get(TSD->cursors[cdb], &key, &data, DB_NEXT);
+       ret = TSD->cursors[cdb]->c_get(TSD->cursors[cdb], &TSD->dbkey[cdb], &TSD->dbdata[cdb], DB_NEXT);
 
        if (ret) {
                if (ret != DB_NOTFOUND) {
@@ -641,8 +631,8 @@ struct cdbdata *bdb_next_item(int cdb) {
        }
 
        cdbret = (struct cdbdata *) malloc(sizeof(struct cdbdata));
-       cdbret->len = data.size;
-       cdbret->ptr = data.data;
+       cdbret->len = TSD->dbdata[cdb].size;
+       cdbret->ptr = TSD->dbdata[cdb].data;
        bdb_decompress_if_necessary(cdbret);
 
        return (cdbret);