This is Jeremy pretending to be Volker, 'cos the

link from Seattle is having problems.
I've added 3 things here to work on the fcntl spin
problem.
1). Check *all* tdb return codes... :-).
2). If we're asking ourselves to break an oplock, and we can't
find a fsp pointer that matches the entry, this is a *logic bug*
and we should abort and panic so someone with gdb can pick up
the pieces.
3). After we've broken an oplock, ensure that the entry itself
has been removed, and if not remove it ourselves. This should
not be neccessary in a correctly working environmen,t, but will
provide an added layer of robustness in error situations.
4). I hate german keyboards :-) :-).
Jeremy.
(This used to be commit 1c94fa80646f9e31377fbb41332fe4780f550cab)

1 files changed, 136 insertions, 64 deletions

diff --git a/source3/locking/locking.c b/source3/locking/locking.c
index ee3ce7c7d4..4b037c0f3d 100644
--- a/source3/locking/locking.c
+++ b/source3/locking/locking.c
@@ -239,6 +239,7 @@ static int delete_fn(TDB_CONTEXT *ttdb, TDB_DATA kbuf, TDB_DATA dbuf, void *stat
 	int i, del_count=0;
 	pid_t mypid = sys_getpid();
 	BOOL check_self = *(BOOL *)state;
+	int ret = 0;
 
 	tdb_chainlock(tdb, kbuf);
 
@@ -270,13 +271,16 @@ static int delete_fn(TDB_CONTEXT *ttdb, TDB_DATA kbuf, TDB_DATA dbuf, void *stat
 	dbuf.dsize -= del_count * sizeof(*shares);
 
 	/* store it back in the database */
-	if (data->num_share_mode_entries == 0)
-		tdb_delete(ttdb, kbuf);
-	else
-		tdb_store(ttdb, kbuf, dbuf, TDB_REPLACE);
+	if (data->num_share_mode_entries == 0) {
+		if (tdb_delete(ttdb, kbuf) == -1)
+			ret = -1;
+	} else {
+		if (tdb_store(ttdb, kbuf, dbuf, TDB_REPLACE) == -1)
+			ret = -1;
+	}
 
 	tdb_chainunlock(tdb, kbuf);
-	return 0;
+	return ret;
 }
 
 /****************************************************************************
@@ -340,8 +344,9 @@ BOOL locking_end(void)
 }
 
 /*******************************************************************
- form a static locking key for a dev/inode pair 
+ Form a static locking key for a dev/inode pair.
 ******************************************************************/
+
 static TDB_DATA locking_key(SMB_DEV_T dev, SMB_INO_T inode)
 {
 	static struct locking_key key;
@@ -354,6 +359,7 @@ static TDB_DATA locking_key(SMB_DEV_T dev, SMB_INO_T inode)
 	kbuf.dsize = sizeof(key);
 	return kbuf;
 }
+
 static TDB_DATA locking_key_fsp(files_struct *fsp)
 {
 	return locking_key(fsp->dev, fsp->inode);
@@ -377,10 +383,10 @@ void unlock_share_entry(connection_struct *conn,
 	tdb_chainunlock(tdb, locking_key(dev, inode));
 }
 
-
 /*******************************************************************
  Lock a hash bucket entry. use a fsp for convenience
 ******************************************************************/
+
 BOOL lock_share_entry_fsp(files_struct *fsp)
 {
 	return tdb_chainlock(tdb, locking_key(fsp->dev, fsp->inode)) == 0;
@@ -389,6 +395,7 @@ BOOL lock_share_entry_fsp(files_struct *fsp)
 /*******************************************************************
  Unlock a hash bucket entry.
 ******************************************************************/
+
 void unlock_share_entry_fsp(files_struct *fsp)
 {
 	tdb_chainunlock(tdb, locking_key(fsp->dev, fsp->inode));
@@ -397,6 +404,7 @@ void unlock_share_entry_fsp(files_struct *fsp)
 /*******************************************************************
  Get all share mode entries for a dev/inode pair.
 ********************************************************************/
+
 int get_share_modes(connection_struct *conn, 
 		    SMB_DEV_T dev, SMB_INO_T inode, 
 		    share_mode_entry **shares)
@@ -408,7 +416,8 @@ int get_share_modes(connection_struct *conn,
 	*shares = NULL;
 
 	dbuf = tdb_fetch(tdb, locking_key(dev, inode));
-	if (!dbuf.dptr) return 0;
+	if (!dbuf.dptr)
+		return 0;
 
 	data = (struct locking_data *)dbuf.dptr;
 	ret = data->num_share_mode_entries;
@@ -416,41 +425,76 @@ int get_share_modes(connection_struct *conn,
 		*shares = (share_mode_entry *)memdup(dbuf.dptr + sizeof(*data), ret * sizeof(**shares));
 	free(dbuf.dptr);
 
-	if (! *shares) return 0;
+	if (! *shares)
+		return 0;
 
 	return ret;
 }
 
 /*******************************************************************
- Del the share mode of a file for this process. Return the number
- of entries left, and a memdup'ed copy of the entry deleted.
+ Fill a share mode entry.
 ********************************************************************/
 
-size_t del_share_mode(files_struct *fsp, share_mode_entry **ppse)
+static void fill_share_mode(char *p, files_struct *fsp, uint16 port, uint16 op_type)
+{
+	share_mode_entry *e = (share_mode_entry *)p;
+	memset(e, '\0', sizeof(share_mode_entry));
+	e->pid = sys_getpid();
+	e->share_mode = fsp->share_mode;
+	e->op_port = port;
+	e->op_type = op_type;
+	memcpy((char *)&e->time, (char *)&fsp->open_time, sizeof(struct timeval));
+}
+
+/*******************************************************************
+ Check if two share mode entries are identical, ignoring oplock 
+ and port info. 
+********************************************************************/
+
+static BOOL share_modes_identical( share_mode_entry *e1, share_mode_entry *e2)
+{
+	return (e1->pid == e2->pid &&
+	  e1->share_mode == e2->share_mode &&
+	  e1->time.tv_sec == e2->time.tv_sec &&
+	  e1->time.tv_usec == e2->time.tv_usec );
+}
+
+/*******************************************************************
+ Delete a specific share mode. Return the number
+ of entries left, and a memdup'ed copy of the entry deleted (if required).
+ Ignore if no entry deleted.
+********************************************************************/
+
+ssize_t del_share_entry( SMB_DEV_T dev, SMB_INO_T inode,
+			share_mode_entry *entry, share_mode_entry **ppse)
 {
 	TDB_DATA dbuf;
 	struct locking_data *data;
 	int i, del_count=0;
 	share_mode_entry *shares;
-	pid_t pid = sys_getpid();
-	size_t count;
+	ssize_t count;
 
-	*ppse = NULL;
+	if (ppse)
+		*ppse = NULL;
 
 	/* read in the existing share modes */
-	dbuf = tdb_fetch(tdb, locking_key_fsp(fsp));
-	if (!dbuf.dptr) return 0;
+	dbuf = tdb_fetch(tdb, locking_key(dev, inode));
+	if (!dbuf.dptr)
+		return -1;
 
 	data = (struct locking_data *)dbuf.dptr;
 	shares = (share_mode_entry *)(dbuf.dptr + sizeof(*data));
 
-	/* find any with our pid and delete it by overwriting with the rest of the data 
-	   from the record */
+	/*
+	 * Find any with this pid and delete it
+	 * by overwriting with the rest of the data 
+	 * from the record.
+	 */
+
 	for (i=0;i<data->num_share_mode_entries;) {
-		if (shares[i].pid == pid &&
-		    shares[i].time.tv_sec == fsp->open_time.tv_sec &&
-		    shares[i].time.tv_usec == fsp->open_time.tv_usec ) {
-			*ppse = memdup(&shares[i], sizeof(*shares));
+		if (share_modes_identical(&shares[i], entry)) {
+			if (ppse)
+				*ppse = memdup(&shares[i], sizeof(*shares));
 			data->num_share_mode_entries--;
 			memmove(&shares[i], &shares[i+1], 
 				dbuf.dsize - (sizeof(*data) + (i+1)*sizeof(*shares)));
@@ -460,44 +504,53 @@ size_t del_share_mode(files_struct *fsp, share_mode_entry **ppse)
 		}
 	}
 
-	/* the record has shrunk a bit */
-	dbuf.dsize -= del_count * sizeof(*shares);
+	if (del_count) {
+		/* the record may have shrunk a bit */
+		dbuf.dsize -= del_count * sizeof(*shares);
 
-	count = data->num_share_mode_entries;
+		count = (ssize_t)data->num_share_mode_entries;
 
-	/* store it back in the database */
-	if (data->num_share_mode_entries == 0) {
-		tdb_delete(tdb, locking_key_fsp(fsp));
-	} else {
-		tdb_store(tdb, locking_key_fsp(fsp), dbuf, TDB_REPLACE);
+		/* store it back in the database */
+		if (data->num_share_mode_entries == 0) {
+			if (tdb_delete(tdb, locking_key(dev, inode)) == -1)
+				count = -1;
+		} else {
+			if (tdb_store(tdb, locking_key(dev, inode), dbuf, TDB_REPLACE) == -1)
+				count = -1;
+		}
 	}
-
 	free(dbuf.dptr);
 	return count;
 }
 
 /*******************************************************************
-fill a share mode entry
+ Del the share mode of a file for this process. Return the number
+ of entries left, and a memdup'ed copy of the entry deleted.
 ********************************************************************/
-static void fill_share_mode(char *p, files_struct *fsp, uint16 port, uint16 op_type)
+
+ssize_t del_share_mode(files_struct *fsp, share_mode_entry **ppse)
 {
-	share_mode_entry *e = (share_mode_entry *)p;
-	e->pid = sys_getpid();
-	e->share_mode = fsp->share_mode;
-	e->op_port = port;
-	e->op_type = op_type;
-	memcpy((char *)&e->time, (char *)&fsp->open_time, sizeof(struct timeval));
+	share_mode_entry entry;
+
+	/*
+	 * Fake up a share_mode_entry for comparisons.
+	 */
+
+	fill_share_mode((char *)&entry, fsp, 0, 0);
+	return del_share_entry(fsp->dev, fsp->inode, &entry, ppse);
 }
 
 /*******************************************************************
  Set the share mode of a file. Return False on fail, True on success.
 ********************************************************************/
+
 BOOL set_share_mode(files_struct *fsp, uint16 port, uint16 op_type)
 {
 	TDB_DATA dbuf;
 	struct locking_data *data;
 	char *p=NULL;
 	int size;
+	BOOL ret = True;
 		
 	/* read in the existing share modes if any */
 	dbuf = tdb_fetch(tdb, locking_key_fsp(fsp));
@@ -511,15 +564,18 @@ BOOL set_share_mode(files_struct *fsp, uint16 port, uint16 op_type)
 
 		size = sizeof(*data) + sizeof(share_mode_entry) + strlen(fname) + 1;
 		p = (char *)malloc(size);
+		if (!p)
+			return False;
 		data = (struct locking_data *)p;
 		data->num_share_mode_entries = 1;
 		pstrcpy(p + sizeof(*data) + sizeof(share_mode_entry), fname);
 		fill_share_mode(p + sizeof(*data), fsp, port, op_type);
 		dbuf.dptr = p;
 		dbuf.dsize = size;
-		tdb_store(tdb, locking_key_fsp(fsp), dbuf, TDB_REPLACE);
+		if (tdb_store(tdb, locking_key_fsp(fsp), dbuf, TDB_REPLACE) == -1)
+			ret = False;
 		free(p);
-		return True;
+		return ret;
 	}
 
 	/* we're adding to an existing entry - this is a bit fiddly */
@@ -528,6 +584,8 @@ BOOL set_share_mode(files_struct *fsp, uint16 port, uint16 op_type)
 	data->num_share_mode_entries++;
 	size = dbuf.dsize + sizeof(share_mode_entry);
 	p = malloc(size);
+	if (!p)
+		return False;
 	memcpy(p, dbuf.dptr, sizeof(*data));
 	fill_share_mode(p + sizeof(*data), fsp, port, op_type);
 	memcpy(p + sizeof(*data) + sizeof(share_mode_entry), dbuf.dptr + sizeof(*data),
@@ -535,15 +593,16 @@ BOOL set_share_mode(files_struct *fsp, uint16 port, uint16 op_type)
 	free(dbuf.dptr);
 	dbuf.dptr = p;
 	dbuf.dsize = size;
-	tdb_store(tdb, locking_key_fsp(fsp), dbuf, TDB_REPLACE);
+	if (tdb_store(tdb, locking_key_fsp(fsp), dbuf, TDB_REPLACE) == -1)
+		ret = False;
 	free(p);
-	return True;
+	return ret;
 }
 
-
 /*******************************************************************
-a generic in-place modification call for share mode entries
+ A generic in-place modification call for share mode entries.
 ********************************************************************/
+
 static BOOL mod_share_mode(files_struct *fsp,
 			   void (*mod_fn)(share_mode_entry *, SMB_DEV_T, SMB_INO_T, void *),
 			   void *param)
@@ -551,34 +610,39 @@ static BOOL mod_share_mode(files_struct *fsp,
 	TDB_DATA dbuf;
 	struct locking_data *data;
 	int i;
-	share_mode_entry *shares;
-	pid_t pid = sys_getpid();
-	int need_store=0;
+	share_mode_entry *shares, entry;
+	BOOL need_store=False;
 
 	/* read in the existing share modes */
 	dbuf = tdb_fetch(tdb, locking_key_fsp(fsp));
-	if (!dbuf.dptr) return False;
+	if (!dbuf.dptr)
+		return False;
 
 	data = (struct locking_data *)dbuf.dptr;
 	shares = (share_mode_entry *)(dbuf.dptr + sizeof(*data));
 
+	/*
+	 * Fake up a share_mode_entry for comparisons.
+	 */
+
+	fill_share_mode((char *)&entry, fsp, 0, 0);
+	
 	/* find any with our pid and call the supplied function */
 	for (i=0;i<data->num_share_mode_entries;i++) {
-		if (pid == shares[i].pid && 
-		    shares[i].share_mode == fsp->share_mode &&
-		    shares[i].time.tv_sec == fsp->open_time.tv_sec &&
-		    shares[i].time.tv_usec == fsp->open_time.tv_usec ) {
+		if (share_modes_identical(&entry, &shares[i])) {
 			mod_fn(&shares[i], fsp->dev, fsp->inode, param);
-			need_store=1;
+			need_store=True;
 		}
 	}
 
 	/* if the mod fn was called then store it back */
 	if (need_store) {
 		if (data->num_share_mode_entries == 0) {
-			tdb_delete(tdb, locking_key_fsp(fsp));
+			if (tdb_delete(tdb, locking_key_fsp(fsp)) == -1)
+				need_store = False;
 		} else {
-			tdb_store(tdb, locking_key_fsp(fsp), dbuf, TDB_REPLACE);
+			if (tdb_store(tdb, locking_key_fsp(fsp), dbuf, TDB_REPLACE) == -1)
+				need_store = False;
 		}
 	}
 
@@ -586,11 +650,11 @@ static BOOL mod_share_mode(files_struct *fsp,
 	return need_store;
 }
 
-
 /*******************************************************************
  Static function that actually does the work for the generic function
  below.
 ********************************************************************/
+
 static void remove_share_oplock_fn(share_mode_entry *entry, SMB_DEV_T dev, SMB_INO_T inode, 
                                    void *param)
 {
@@ -604,6 +668,7 @@ static void remove_share_oplock_fn(share_mode_entry *entry, SMB_DEV_T dev, SMB_I
 /*******************************************************************
  Remove an oplock port and mode entry from a share mode.
 ********************************************************************/
+
 BOOL remove_share_oplock(files_struct *fsp)
 {
 	return mod_share_mode(fsp, remove_share_oplock_fn, NULL);
@@ -613,6 +678,7 @@ BOOL remove_share_oplock(files_struct *fsp)
  Static function that actually does the work for the generic function
  below.
 ********************************************************************/
+
 static void downgrade_share_oplock_fn(share_mode_entry *entry, SMB_DEV_T dev, SMB_INO_T inode, 
                                    void *param)
 {
@@ -624,6 +690,7 @@ static void downgrade_share_oplock_fn(share_mode_entry *entry, SMB_DEV_T dev, SM
 /*******************************************************************
  Downgrade a oplock type from exclusive to level II.
 ********************************************************************/
+
 BOOL downgrade_share_oplock(files_struct *fsp)
 {
 	return mod_share_mode(fsp, downgrade_share_oplock_fn, NULL);
@@ -643,7 +710,8 @@ BOOL modify_delete_flag( SMB_DEV_T dev, SMB_INO_T inode, BOOL delete_on_close)
 
 	/* read in the existing share modes */
 	dbuf = tdb_fetch(tdb, locking_key(dev, inode));
-	if (!dbuf.dptr) return False;
+	if (!dbuf.dptr)
+		return False;
 
 	data = (struct locking_data *)dbuf.dptr;
 	shares = (share_mode_entry *)(dbuf.dptr + sizeof(*data));
@@ -657,19 +725,21 @@ BOOL modify_delete_flag( SMB_DEV_T dev, SMB_INO_T inode, BOOL delete_on_close)
 
 	/* store it back */
 	if (data->num_share_mode_entries) {
-		if (tdb_store(tdb, locking_key(dev,inode), dbuf, TDB_REPLACE)==-1)
+		if (tdb_store(tdb, locking_key(dev,inode), dbuf, TDB_REPLACE)==-1) {
+			free(dbuf.dptr);
 			return False;
+		}
 	}
 
 	free(dbuf.dptr);
 	return True;
 }
 
-
 /****************************************************************************
-traverse the whole database with this function, calling traverse_callback
-on each share mode
+ Traverse the whole database with this function, calling traverse_callback
+ on each share mode
 ****************************************************************************/
+
 static int traverse_fn(TDB_CONTEXT *the_tdb, TDB_DATA kbuf, TDB_DATA dbuf, 
                        void* state)
 {
@@ -694,8 +764,10 @@ static int traverse_fn(TDB_CONTEXT *the_tdb, TDB_DATA kbuf, TDB_DATA dbuf,
  Call the specified function on each entry under management by the
  share mode system.
 ********************************************************************/
+
 int share_mode_forall(SHAREMODE_FN(fn))
 {
-	if (!tdb) return 0;
+	if (!tdb)
+		return 0;
 	return tdb_traverse(tdb, traverse_fn, (void*)fn);
 }