From 3c40b8829417e15d5b62226a43778b1dbec8a16c Mon Sep 17 00:00:00 2001
From: Art Cancro <ajc@citadel.org>
Date: Sat, 19 Aug 2023 17:42:31 -0900
Subject: [PATCH] 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.
---
 .../server/backends/berkeley_db/berkeley_db.c | 40 +++++++------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/citadel/server/backends/berkeley_db/berkeley_db.c b/citadel/server/backends/berkeley_db/berkeley_db.c
index 62b8738f6..54d8dfecb 100644
--- 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);
-- 
2.39.2