[SCM] Samba Shared Repository - branch master updated - release-4-0-0alpha7-1429-ge46a88c

Jeremy Allison jra at samba.org
Mon May 4 15:33:39 GMT 2009


The branch, master has been updated
       via  e46a88ce35e1aba9d9a344773bc97a9f3f2bd616 (commit)
      from  d8de7e3193143ec50d86adc704123ca240a8f549 (commit)

http://gitweb.samba.org/?p=samba.git;a=shortlog;h=master


- Log -----------------------------------------------------------------
commit e46a88ce35e1aba9d9a344773bc97a9f3f2bd616
Author: Jeremy Allison <jra at samba.org>
Date:   Mon May 4 08:31:40 2009 -0700

    Fix bug #6315 smbd crashes doing vfs_full_audit on IPC$ close event.
    The underlying problem
    is that once SMBulogoff is called, all server_info contexts associated with the
    vuid should become invalid, even if that's the context being currently used by
    the connection struct (tid). When the SMBtdis comes in it doesn't need a valid
    vuid value, but the code called inside vfs_full_audit always assumes that there
    is one (and hence a valid conn->server_info pointer) available.
    
    This is actually a bug inside the vfs_full_audit and other code inside Samba,
    which should only indirect conn->server_info on calls which require AS_USER to
    be set in our process table. I could fix all these issues, but there's no
    guarentee that someone might not add more code that fails this assumption, as
    it's a hard assumption to break (it's usually true).
    
    So what I've done is to ensure that on SMBulogoff the previously used
    conn->server_info struct is kept around to be used for print debugging purposes
    (it won't be used to change to an invalid user context, as such calls need
    AS_USER set). This isn't strictly correct, as there's no association with the
    (now invalid) context being freed and the call that causes conn->server_info to
    be indirected, but it's good enough for most cases.
    
    The hard part was to ensure that once a valid context is used again (via new
    sessionsetupX calls, or new calls on a still valid vuid on this tid) that we
    don't leak memory by simply replacing the stored conn->server_info pointer. We
    would never actually leak the memory (as all conn->server_info pointers are
    talloc children of conn), but with the previous patch a malicious client could
    cause many server_info structs to be talloced by the right combination of SMB
    calls. This new patch introduces free_conn_server_info_if_unused(), which
    protects against the above.
    Jeremy.

-----------------------------------------------------------------------

Summary of changes:
 source3/smbd/uid.c |   47 ++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 44 insertions(+), 3 deletions(-)


Changeset truncated at 500 lines:

diff --git a/source3/smbd/uid.c b/source3/smbd/uid.c
index b8ed321..72837ff 100644
--- a/source3/smbd/uid.c
+++ b/source3/smbd/uid.c
@@ -52,6 +52,26 @@ bool change_to_guest(void)
 	return true;
 }
 
+/****************************************************************************
+ talloc free the conn->server_info if not used in the vuid cache.
+****************************************************************************/
+
+static void free_conn_server_info_if_unused(connection_struct *conn)
+{
+	unsigned int i;
+
+	for (i = 0; i < VUID_CACHE_SIZE; i++) {
+		struct vuid_cache_entry *ent;
+		ent = &conn->vuid_cache.array[i];
+		if (ent->vuid != UID_FIELD_INVALID &&
+				conn->server_info == ent->server_info) {
+			return;
+		}
+	}
+	/* Not used, safe to free. */
+	TALLOC_FREE(conn->server_info);
+}
+
 /*******************************************************************
  Check if a username is OK.
 
@@ -75,6 +95,7 @@ static bool check_user_ok(connection_struct *conn,
 		for (i=0; i<VUID_CACHE_SIZE; i++) {
 			ent = &conn->vuid_cache.array[i];
 			if (ent->vuid == vuid) {
+				free_conn_server_info_if_unused(conn);
 				conn->server_info = ent->server_info;
 				conn->read_only = ent->read_only;
 				conn->admin_user = ent->admin_user;
@@ -140,6 +161,7 @@ static bool check_user_ok(connection_struct *conn,
 		ent->vuid = vuid;
 		ent->read_only = readonly_share;
 		ent->admin_user = admin_user;
+		free_conn_server_info_if_unused(conn);
 		conn->server_info = ent->server_info;
 	}
 
@@ -151,6 +173,7 @@ static bool check_user_ok(connection_struct *conn,
 
 /****************************************************************************
  Clear a vuid out of the connection's vuid cache
+ This is only called on SMBulogoff.
 ****************************************************************************/
 
 void conn_clear_vuid_cache(connection_struct *conn, uint16_t vuid)
@@ -164,11 +187,29 @@ void conn_clear_vuid_cache(connection_struct *conn, uint16_t vuid)
 
 		if (ent->vuid == vuid) {
 			ent->vuid = UID_FIELD_INVALID;
-			/* Ensure we're not freeing an active pointer. */
+			/*
+			 * We need to keep conn->server_info around
+			 * if it's equal to ent->server_info as a SMBulogoff
+			 * is often followed by a SMBtdis (with an invalid
+			 * vuid). The debug code (or regular code in
+			 * vfs_full_audit) wants to refer to the
+			 * conn->server_info pointer to print debug
+			 * statements. Theoretically this is a bug,
+			 * as once the vuid is gone the server_info
+			 * on the conn struct isn't valid any more,
+			 * but there's enough code that assumes
+			 * conn->server_info is never null that
+			 * it's easier to hold onto the old pointer
+			 * until we get a new sessionsetupX.
+			 * As everything is hung off the
+			 * conn pointer as a talloc context we're not
+			 * leaking memory here. See bug #6315. JRA.
+			 */
 			if (conn->server_info == ent->server_info) {
-				conn->server_info = NULL;
+				ent->server_info = NULL;
+			} else {
+				TALLOC_FREE(ent->server_info);
 			}
-			TALLOC_FREE(ent->server_info);
 			ent->read_only = False;
 			ent->admin_user = False;
 		}


-- 
Samba Shared Repository


More information about the samba-cvs mailing list