The branch, v3-4-test has been updated
via 4b3bd6d0ba3348659615e69b3508969aa41e7de4 (commit)
from c2eb0d87a2436614741119ebd14fda05b42a2ddd (commit)
http://gitweb.samba.org/?p=samba.git;a=shortlog;h=v3-4-test
- Log -----------------------------------------------------------------
commit 4b3bd6d0ba3348659615e69b3508969aa41e7de4
Author: Jeremy Allison <[email protected]>
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
