better check of called function's return

tdbtorture say it's ok
(This used to be commit af0fa4cf7c229fb908063bfcc3cbb214dae5ed0e)

2 files changed, 127 insertions, 59 deletions

diff --git a/source3/tdb/tdb.c b/source3/tdb/tdb.c
index e5f1b0a19b..0a847ed690 100644
--- a/source3/tdb/tdb.c
+++ b/source3/tdb/tdb.c
@@ -84,16 +84,20 @@ TDB_DATA tdb_null;
 /* all contexts, to ensure no double-opens (fcntl locks don't nest!) */
 static TDB_CONTEXT *tdbs = NULL;
 
-static void tdb_munmap(TDB_CONTEXT *tdb)
+static int tdb_munmap(TDB_CONTEXT *tdb)
 {
 	if (tdb->flags & TDB_INTERNAL)
-		return;
+		return 0;
 
 #ifdef HAVE_MMAP
-	if (tdb->map_ptr)
-		munmap(tdb->map_ptr, tdb->map_size);
+	if (tdb->map_ptr) {
+		int ret = munmap(tdb->map_ptr, tdb->map_size);
+		if (ret != 0)
+			return ret;
+	}
 #endif
 	tdb->map_ptr = NULL;
+	return 0;
 }
 
 static void tdb_mmap(TDB_CONTEXT *tdb)
@@ -222,25 +226,41 @@ static int tdb_lock(TDB_CONTEXT *tdb, int list, int ltype)
 }
 
 /* unlock the database: returns void because it's too late for errors. */
-static void tdb_unlock(TDB_CONTEXT *tdb, int list, int ltype)
+	/* changed to return int it may be interesting to know there
+	   has been an error  --simo */
+static int tdb_unlock(TDB_CONTEXT *tdb, int list, int ltype)
 {
+	int ret = -1;
+
 	if (tdb->flags & TDB_NOLOCK)
-		return;
+		return 0;
 
 	/* Sanity checks */
-	if (list < -1 || list >= (int)tdb->header.hash_size)
-		return;
-	if (tdb->locked[list+1].count==0)
-		return;
+	if (list < -1 || list >= (int)tdb->header.hash_size) {
+		TDB_LOG((tdb, 0, "tdb_unlock: list %d invalid (%d)\n", list, tdb->header.hash_size));
+		return ret;
+	}
+
+	if (tdb->locked[list+1].count==0) {
+		TDB_LOG((tdb, 0, "tdb_unlock: count is 0\n"));
+		return ret;
+	}
 
 	if (tdb->locked[list+1].count == 1) {
 		/* Down to last nested lock: unlock underneath */
-		if (!tdb->read_only && tdb->header.rwlocks)
-			tdb_spinunlock(tdb, list, ltype);
-		else
-			tdb_brlock(tdb, FREELIST_TOP+4*list, F_UNLCK, F_SETLKW, 0);
+		if (!tdb->read_only && tdb->header.rwlocks) {
+			ret = tdb_spinunlock(tdb, list, ltype);
+		} else {
+			ret = tdb_brlock(tdb, FREELIST_TOP+4*list, F_UNLCK, F_SETLKW, 0);
+		}
+	} else {
+		ret = 0;
 	}
 	tdb->locked[list+1].count--;
+
+	if (ret)
+		TDB_LOG((tdb, 0,"tdb_unlock: An error occurred unlocking!\n")); 
+	return ret;
 }
 
 /* This is based on the hash algorithm from gdbm */
@@ -434,18 +454,17 @@ static tdb_off tdb_dump_record(TDB_CONTEXT *tdb, tdb_off offset)
 	return rec.next;
 }
 
-static void tdb_dump_chain(TDB_CONTEXT *tdb, int i)
+static int tdb_dump_chain(TDB_CONTEXT *tdb, int i)
 {
 	tdb_off rec_ptr, top;
 
 	top = TDB_HASH_TOP(i);
 
-	tdb_lock(tdb, i, F_WRLCK);
+	if (tdb_lock(tdb, i, F_WRLCK) != 0)
+		return -1;
 
-	if (ofs_read(tdb, top, &rec_ptr) == -1) {
-		tdb_unlock(tdb, i, F_WRLCK);
-		return;
-	}
+	if (ofs_read(tdb, top, &rec_ptr) == -1)
+		return tdb_unlock(tdb, i, F_WRLCK);
 
 	if (rec_ptr)
 		printf("hash=%d\n", i);
@@ -453,7 +472,8 @@ static void tdb_dump_chain(TDB_CONTEXT *tdb, int i)
 	while (rec_ptr) {
 		rec_ptr = tdb_dump_record(tdb, rec_ptr);
 	}
-	tdb_unlock(tdb, i, F_WRLCK);
+
+	return tdb_unlock(tdb, i, F_WRLCK);
 }
 
 void tdb_dump_all(TDB_CONTEXT *tdb)
@@ -466,30 +486,32 @@ void tdb_dump_all(TDB_CONTEXT *tdb)
 	tdb_dump_chain(tdb, -1);
 }
 
-void tdb_printfreelist(TDB_CONTEXT *tdb)
+int tdb_printfreelist(TDB_CONTEXT *tdb)
 {
+	int ret;
 	long total_free = 0;
 	tdb_off offset, rec_ptr;
 	struct list_struct rec;
 
-	tdb_lock(tdb, -1, F_WRLCK);
+	if ((ret = tdb_lock(tdb, -1, F_WRLCK)) != 0)
+		return ret;
 
 	offset = FREELIST_TOP;
 
 	/* read in the freelist top */
 	if (ofs_read(tdb, offset, &rec_ptr) == -1) {
-		return;
+		return 0;
 	}
 
 	printf("freelist top=[0x%08x]\n", rec_ptr );
 	while (rec_ptr) {
 		if (tdb_read(tdb, rec_ptr, (char *)&rec, sizeof(rec), DOCONV()) == -1) {
-			return;
+			return -1;
 		}
 
 		if (rec.magic != TDB_FREE_MAGIC) {
 			printf("bad magic 0x%08x in free list\n", rec.magic);
-			return;
+			return -1;
 		}
 
 		printf("entry offset=[0x%08x], rec.rec_len = [0x%08x (%d)]\n", rec.next, rec.rec_len, rec.rec_len );
@@ -501,7 +523,7 @@ void tdb_printfreelist(TDB_CONTEXT *tdb)
 	printf("total rec_len = [0x%08x (%d)]\n", (int)total_free, 
                (int)total_free);
 
-	tdb_unlock(tdb, -1, F_WRLCK);
+	return tdb_unlock(tdb, -1, F_WRLCK);
 }
 
 /* Remove an element from the freelist.  Must have alloc lock. */
@@ -534,7 +556,10 @@ static int tdb_free(TDB_CONTEXT *tdb, tdb_off offset, struct list_struct *rec)
 		return -1;
 
 	/* set an initial tailer, so if we fail we don't leave a bogus record */
-	update_tailer(tdb, offset, rec);
+	if (update_tailer(tdb, offset, rec) != 0) {
+		TDB_LOG((tdb, 0, "tdb_free: upfate_tailer failed!\n"));
+		goto fail;
+	}
 
 	/* Look right first (I'm an Australian, dammit) */
 	right = offset + sizeof(*rec) + rec->rec_len;
@@ -1077,7 +1102,8 @@ static int do_delete(TDB_CONTEXT *tdb, tdb_off rec_ptr, struct list_struct*rec)
 		rec->magic = TDB_DEAD_MAGIC;
 		return rec_write(tdb, rec_ptr, rec);
 	}
-	write_unlock_record(tdb, rec_ptr);
+	if (write_unlock_record(tdb, rec_ptr) != 0)
+		return -1;
 
 	/* find previous record in hash chain */
 	if (ofs_read(tdb, TDB_HASH_TOP(rec->full_hash), &i) == -1)
@@ -1120,7 +1146,8 @@ static int tdb_next_lock(TDB_CONTEXT *tdb, struct tdb_traverse_lock *tlock,
 				goto fail;
 		} else {
 			/* Otherwise unlock the previous record. */
-			unlock_record(tdb, tlock->off);
+			if (unlock_record(tdb, tlock->off) != 0)
+				goto fail;
 		}
 
 		if (want_next) {
@@ -1137,13 +1164,15 @@ static int tdb_next_lock(TDB_CONTEXT *tdb, struct tdb_traverse_lock *tlock,
 				goto fail;
 			if (!TDB_DEAD(rec)) {
 				/* Woohoo: we found one! */
-				lock_record(tdb, tlock->off);
+				if (lock_record(tdb, tlock->off) != 0)
+					goto fail;
 				return tlock->off;
 			}
 			/* Try to clean dead ones from old traverses */
 			current = tlock->off;
 			tlock->off = rec->next;
-			do_delete(tdb, current, rec);
+			if (do_delete(tdb, current, rec) != 0)
+				goto fail;
 		}
 		tdb_unlock(tdb, tlock->hash, F_WRLCK);
 		want_next = 0;
@@ -1153,7 +1182,8 @@ static int tdb_next_lock(TDB_CONTEXT *tdb, struct tdb_traverse_lock *tlock,
 
  fail:
 	tlock->off = 0;
-	tdb_unlock(tdb, tlock->hash, F_WRLCK);
+	if (tdb_unlock(tdb, tlock->hash, F_WRLCK) != 0)
+		TDB_LOG((tdb, 0, "tdb_next_lock: On error unlock failed!\n"));
 	return -1;
 }
 
@@ -1184,26 +1214,36 @@ int tdb_traverse(TDB_CONTEXT *tdb, tdb_traverse_func fn, void *state)
 		key.dptr = tdb_alloc_read(tdb, tl.off + sizeof(rec), 
 					  rec.key_len + rec.data_len);
 		if (!key.dptr) {
-			tdb_unlock(tdb, tl.hash, F_WRLCK);
-			unlock_record(tdb, tl.off);
-			tdb->travlocks.next = tl.next;
-			return -1;
+			ret = -1;
+			if (tdb_unlock(tdb, tl.hash, F_WRLCK) != 0)
+				goto out;
+			if (unlock_record(tdb, tl.off) != 0)
+				TDB_LOG((tdb, 0, "tdb_traverse: key.dptr == NULL and unlock_record failed!\n"));
+			goto out;
 		}
 		key.dsize = rec.key_len;
 		dbuf.dptr = key.dptr + rec.key_len;
 		dbuf.dsize = rec.data_len;
 
 		/* Drop chain lock, call out */
-		tdb_unlock(tdb, tl.hash, F_WRLCK);
+		if (tdb_unlock(tdb, tl.hash, F_WRLCK) != 0) {
+			ret = -1;
+			goto out;
+		}
 		if (fn && fn(tdb, key, dbuf, state)) {
 			/* They want us to terminate traversal */
-			unlock_record(tdb, tl.off);
+			ret = count;
+			if (unlock_record(tdb, tl.off) != 0) {
+				TDB_LOG((tdb, 0, "tdb_traverse: unlock_record failed!\n"));;
+				ret = -1;
+			}
 			tdb->travlocks.next = tl.next;
 			SAFE_FREE(key.dptr);
 			return count;
 		}
 		SAFE_FREE(key.dptr);
 	}
+out:
 	tdb->travlocks.next = tl.next;
 	if (ret < 0)
 		return -1;
@@ -1218,7 +1258,8 @@ TDB_DATA tdb_firstkey(TDB_CONTEXT *tdb)
 	struct list_struct rec;
 
 	/* release any old lock */
-	unlock_record(tdb, tdb->travlocks.off);
+	if (unlock_record(tdb, tdb->travlocks.off) != 0)
+		return tdb_null;
 	tdb->travlocks.off = tdb->travlocks.hash = 0;
 
 	if (tdb_next_lock(tdb, &tdb->travlocks, &rec) <= 0)
@@ -1226,7 +1267,8 @@ TDB_DATA tdb_firstkey(TDB_CONTEXT *tdb)
 	/* now read the key */
 	key.dsize = rec.key_len;
 	key.dptr =tdb_alloc_read(tdb,tdb->travlocks.off+sizeof(rec),key.dsize);
-	tdb_unlock(tdb, BUCKET(tdb->travlocks.hash), F_WRLCK);
+	if (tdb_unlock(tdb, BUCKET(tdb->travlocks.hash), F_WRLCK) != 0)
+		TDB_LOG((tdb, 0, "tdb_firstkey: error occurred while tdb_unlocking!\n"));
 	return key;
 }
 
@@ -1247,8 +1289,10 @@ TDB_DATA tdb_nextkey(TDB_CONTEXT *tdb, TDB_DATA oldkey)
 					    rec.key_len))
 		    || memcmp(k, oldkey.dptr, oldkey.dsize) != 0) {
 			/* No, it wasn't: unlock it and start from scratch */
-			unlock_record(tdb, tdb->travlocks.off);
-			tdb_unlock(tdb, tdb->travlocks.hash, F_WRLCK);
+			if (unlock_record(tdb, tdb->travlocks.off) != 0)
+				return tdb_null;
+			if (tdb_unlock(tdb, tdb->travlocks.hash, F_WRLCK) != 0)
+				return tdb_null;
 			tdb->travlocks.off = 0;
 		}
 
@@ -1261,7 +1305,10 @@ TDB_DATA tdb_nextkey(TDB_CONTEXT *tdb, TDB_DATA oldkey)
 		if (!tdb->travlocks.off)
 			return tdb_null;
 		tdb->travlocks.hash = BUCKET(rec.full_hash);
-		lock_record(tdb, tdb->travlocks.off);
+		if (lock_record(tdb, tdb->travlocks.off) != 0) {
+			TDB_LOG((tdb, 0, "tdb_nextkey: lock_record failed (%s)!\n", strerror(errno)));
+			return tdb_null;
+		}
 	}
 	oldhash = tdb->travlocks.hash;
 
@@ -1272,10 +1319,12 @@ TDB_DATA tdb_nextkey(TDB_CONTEXT *tdb, TDB_DATA oldkey)
 		key.dptr = tdb_alloc_read(tdb, tdb->travlocks.off+sizeof(rec),
 					  key.dsize);
 		/* Unlock the chain of this new record */
-		tdb_unlock(tdb, tdb->travlocks.hash, F_WRLCK);
+		if (tdb_unlock(tdb, tdb->travlocks.hash, F_WRLCK) != 0)
+			TDB_LOG((tdb, 0, "tdb_nextkey: WARNING tdb_unlock failed!\n"));
 	}
 	/* Unlock the chain of old record */
-	tdb_unlock(tdb, BUCKET(oldhash), F_WRLCK);
+	if (tdb_unlock(tdb, BUCKET(oldhash), F_WRLCK) != 0)
+		TDB_LOG((tdb, 0, "tdb_nextkey: WARNING tdb_unlock failed!\n"));
 	return key;
 }
 
@@ -1289,7 +1338,8 @@ int tdb_delete(TDB_CONTEXT *tdb, TDB_DATA key)
 	if (!(rec_ptr = tdb_find_lock(tdb, key, F_WRLCK, &rec)))
 		return -1;
 	ret = do_delete(tdb, rec_ptr, &rec);
-	tdb_unlock(tdb, BUCKET(rec.full_hash), F_WRLCK);
+	if (tdb_unlock(tdb, BUCKET(rec.full_hash), F_WRLCK) != 0)
+		TDB_LOG((tdb, 0, "tdb_delete: WARNING tdb_unlock failed!\n"));
 	return ret;
 }
 
@@ -1365,14 +1415,16 @@ int tdb_store(TDB_CONTEXT *tdb, TDB_DATA key, TDB_DATA dbuf, int flag)
 	if (rec_write(tdb, rec_ptr, &rec) == -1
 	    || tdb_write(tdb, rec_ptr+sizeof(rec), p, key.dsize+dbuf.dsize)==-1
 	    || ofs_write(tdb, TDB_HASH_TOP(hash), &rec_ptr) == -1) {
-	fail:
 		/* Need to tdb_unallocate() here */
-		ret = -1;
+		goto fail;
 	}
  out:
 	SAFE_FREE(p); 
 	tdb_unlock(tdb, BUCKET(hash), F_WRLCK);
 	return ret;
+fail:
+	ret = -1;
+	goto out;
 }
 
 static int tdb_already_open(dev_t device,
@@ -1447,7 +1499,10 @@ TDB_CONTEXT *tdb_open_ex(const char *name, int hash_size, int tdb_flags,
 	if (tdb->flags & TDB_INTERNAL) {
 		tdb->flags |= (TDB_NOLOCK | TDB_NOMMAP);
 		tdb->flags &= ~TDB_CLEAR_IF_FIRST;
-		tdb_new_database(tdb, hash_size);
+		if (tdb_new_database(tdb, hash_size) != 0) {
+			TDB_LOG((tdb, 0, "tdb_open_ex: tdb_new_database failed!"));
+			goto fail;
+		}
 		goto internal;
 	}
 
@@ -1524,7 +1579,11 @@ TDB_CONTEXT *tdb_open_ex(const char *name, int hash_size, int tdb_flags,
 	tdb_mmap(tdb);
 	if (locked) {
 		if (!tdb->read_only)
-			tdb_clear_spinlocks(tdb);
+			if (tdb_clear_spinlocks(tdb) != 0) {
+				TDB_LOG((tdb, 0, "tdb_open_ex: "
+				"failed to clear spinlock\n"));
+				goto fail;
+			}
 		if (tdb_brlock(tdb, ACTIVE_LOCK, F_UNLCK, F_SETLK, 0) == -1) {
 			TDB_LOG((tdb, 0, "tdb_open_ex: "
 				 "failed to take ACTIVE_LOCK on %s: %s\n",
@@ -1560,7 +1619,8 @@ TDB_CONTEXT *tdb_open_ex(const char *name, int hash_size, int tdb_flags,
 	}
 	SAFE_FREE(tdb->name);
 	if (tdb->fd != -1)
-		close(tdb->fd);
+		if (close(tdb->fd) != 0)
+			TDB_LOG((tdb, 5, "tdb_open_ex: failed to close tdb->fd on error!\n"));
 	SAFE_FREE(tdb->locked);
 	SAFE_FREE(tdb);
 	errno = save_errno;
@@ -1681,9 +1741,10 @@ int tdb_chainlock(TDB_CONTEXT *tdb, TDB_DATA key)
 {
 	return tdb_lock(tdb, BUCKET(tdb_hash(&key)), F_WRLCK);
 }
-void tdb_chainunlock(TDB_CONTEXT *tdb, TDB_DATA key)
+
+int tdb_chainunlock(TDB_CONTEXT *tdb, TDB_DATA key)
 {
-	tdb_unlock(tdb, BUCKET(tdb_hash(&key)), F_WRLCK);
+	return tdb_unlock(tdb, BUCKET(tdb_hash(&key)), F_WRLCK);
 }
 
 
@@ -1700,14 +1761,21 @@ int tdb_reopen(TDB_CONTEXT *tdb)
 {
 	struct stat st;
 
-	tdb_munmap(tdb);
-	close(tdb->fd);
+	if (tdb_munmap(tdb) != 0) {
+		TDB_LOG((tdb, 0, "tdb_reopen: munmap failed (%s)\n", strerror(errno)));
+		goto fail;
+	}
+	if (close(tdb->fd) != 0)
+		TDB_LOG((tdb, 0, "tdb_reopen: WARNING closing tdb->fd failed!\n"));
 	tdb->fd = open(tdb->name, tdb->open_flags & ~(O_CREAT|O_TRUNC), 0);
 	if (tdb->fd == -1) {
 		TDB_LOG((tdb, 0, "tdb_reopen: open failed (%s)\n", strerror(errno)));
 		goto fail;
 	}
-	fstat(tdb->fd, &st);
+	if (fstat(tdb->fd, &st) != 0) {
+		TDB_LOG((tdb, 0, "tdb_reopen: fstat failed (%s)\n", strerror(errno)));
+		goto fail;
+	}
 	if (st.st_ino != tdb->inode || st.st_dev != tdb->device) {
 		TDB_LOG((tdb, 0, "tdb_reopen: file dev/inode has changed!\n"));
 		goto fail;
diff --git a/source3/tdb/tdb.h b/source3/tdb/tdb.h
index 523c617bdc..54cde10d95 100644
--- a/source3/tdb/tdb.h
+++ b/source3/tdb/tdb.h
@@ -126,11 +126,11 @@ void tdb_unlockall(TDB_CONTEXT *tdb);
 
 /* Low level locking functions: use with care */
 int tdb_chainlock(TDB_CONTEXT *tdb, TDB_DATA key);
-void tdb_chainunlock(TDB_CONTEXT *tdb, TDB_DATA key);
+int tdb_chainunlock(TDB_CONTEXT *tdb, TDB_DATA key);
 
 /* Debug functions. Not used in production. */
 void tdb_dump_all(TDB_CONTEXT *tdb);
-void tdb_printfreelist(TDB_CONTEXT *tdb);
+int tdb_printfreelist(TDB_CONTEXT *tdb);
 
 extern TDB_DATA tdb_null;