[PATCH] fix a few talloc_tos leaks
Jeremy Allison
jra at samba.org
Fri Jul 6 18:31:37 UTC 2018
On Mon, Jun 25, 2018 at 01:28:19PM +0200, David Disseldorp via samba-technical wrote:
> Review / push appreciated.
Great work David, thanks !
RB+ and pushed.
Jeremy.
> From 4de8f919853b59eddfe9321d2e10ea8c89ff46fb Mon Sep 17 00:00:00 2001
> From: David Disseldorp <ddiss at samba.org>
> Date: Mon, 25 Jun 2018 01:59:33 +0200
> Subject: [PATCH 1/5] s3/service: use lp_const_servicename() where possible
>
> The majority of these lp_servicename(talloc_tos(), ...) callers leak
> onto the talloc stackframe. Drop the unnecessary heap allocations.
>
> Signed-off-by: David Disseldorp <ddiss at samba.org>
> ---
> source3/smbd/service.c | 66 +++++++++++++++++++++++++-------------------------
> 1 file changed, 33 insertions(+), 33 deletions(-)
>
> diff --git a/source3/smbd/service.c b/source3/smbd/service.c
> index a928de6c78f..2e4a1136254 100644
> --- a/source3/smbd/service.c
> +++ b/source3/smbd/service.c
> @@ -65,8 +65,8 @@ bool set_conn_connectpath(connection_struct *conn, const char *connectpath)
> return false;
> }
>
> - DEBUG(10,("set_conn_connectpath: service %s, connectpath = %s\n",
> - lp_servicename(talloc_tos(), SNUM(conn)), destname ));
> + DBG_DEBUG("service %s, connectpath = %s\n",
> + lp_const_servicename(SNUM(conn)), destname);
>
> talloc_free(conn->connectpath);
> conn->connectpath = destname;
> @@ -260,7 +260,7 @@ static NTSTATUS find_forced_group(bool force_user,
> }
>
> groupname = talloc_string_sub(talloc_tos(), groupname,
> - "%S", lp_servicename(talloc_tos(), snum));
> + "%S", lp_const_servicename(snum));
> if (groupname == NULL) {
> DEBUG(1, ("talloc_string_sub failed\n"));
> result = NT_STATUS_NO_MEMORY;
> @@ -341,20 +341,20 @@ static NTSTATUS create_connection_session_info(struct smbd_server_connection *sc
>
> if (security_session_user_level(session_info, NULL) < SECURITY_USER) {
> if (!lp_guest_ok(snum)) {
> - DEBUG(2, ("guest user (from session setup) "
> + DBG_WARNING("guest user (from session setup) "
> "not permitted to access this share "
> - "(%s)\n", lp_servicename(talloc_tos(), snum)));
> + "(%s)\n", lp_const_servicename(snum));
> return NT_STATUS_ACCESS_DENIED;
> }
> } else {
> if (!user_ok_token(session_info->unix_info->unix_name,
> session_info->info->domain_name,
> session_info->security_token, snum)) {
> - DEBUG(2, ("user '%s' (from session setup) not "
> + DBG_WARNING("user '%s' (from session setup) not "
> "permitted to access this share "
> "(%s)\n",
> session_info->unix_info->unix_name,
> - lp_servicename(talloc_tos(), snum)));
> + lp_const_servicename(snum));
> return NT_STATUS_ACCESS_DENIED;
> }
> }
> @@ -558,7 +558,7 @@ static NTSTATUS make_connection_snum(struct smbXsrv_connection *xconn,
> if (conn->encrypt_level == SMB_SIGNING_REQUIRED) {
> DBG_ERR("Service [%s] requires encryption, but "
> "it is disabled globally!\n",
> - lp_servicename(talloc_tos(), snum));
> + lp_const_servicename(snum));
> status = NT_STATUS_ACCESS_DENIED;
> goto err_root_exit;
> }
> @@ -582,7 +582,7 @@ static NTSTATUS make_connection_snum(struct smbXsrv_connection *xconn,
>
> {
> char *s = talloc_sub_advanced(talloc_tos(),
> - lp_servicename(talloc_tos(), SNUM(conn)),
> + lp_const_servicename(SNUM(conn)),
> conn->session_info->unix_info->unix_name,
> conn->connectpath,
> conn->session_info->unix_token->gid,
> @@ -599,8 +599,8 @@ static NTSTATUS make_connection_snum(struct smbXsrv_connection *xconn,
> status = NT_STATUS_NO_MEMORY;
> goto err_root_exit;
> }
> - DEBUG(3,("Connect path is '%s' for service [%s]\n",s,
> - lp_servicename(talloc_tos(), snum)));
> + DBG_NOTICE("Connect path is '%s' for service [%s]\n", s,
> + lp_const_servicename(snum));
> TALLOC_FREE(s);
> }
>
> @@ -627,8 +627,8 @@ static NTSTATUS make_connection_snum(struct smbXsrv_connection *xconn,
> /* Initialise VFS function pointers */
>
> if (!smbd_vfs_init(conn)) {
> - DEBUG(0, ("vfs_init failed for service %s\n",
> - lp_servicename(talloc_tos(), snum)));
> + DBG_ERR("vfs_init failed for service %s\n",
> + lp_const_servicename(snum));
> status = NT_STATUS_BAD_NETWORK_NAME;
> goto err_root_exit;
> }
> @@ -643,12 +643,12 @@ static NTSTATUS make_connection_snum(struct smbXsrv_connection *xconn,
> */
>
> if ((lp_max_connections(snum) > 0)
> - && (count_current_connections(lp_servicename(talloc_tos(), SNUM(conn)), True) >=
> + && (count_current_connections(lp_const_servicename(SNUM(conn)), true) >=
> lp_max_connections(snum))) {
>
> - DEBUG(1, ("Max connections (%d) exceeded for %s\n",
> + DBG_WARNING("Max connections (%d) exceeded for %s\n",
> lp_max_connections(snum),
> - lp_servicename(talloc_tos(), snum)));
> + lp_const_servicename(snum));
> status = NT_STATUS_INSUFFICIENT_RESOURCES;
> goto err_root_exit;
> }
> @@ -656,10 +656,10 @@ static NTSTATUS make_connection_snum(struct smbXsrv_connection *xconn,
> /* Invoke VFS make connection hook - this must be the first
> filesystem operation that we do. */
>
> - if (SMB_VFS_CONNECT(conn, lp_servicename(talloc_tos(), snum),
> + if (SMB_VFS_CONNECT(conn, lp_const_servicename(snum),
> conn->session_info->unix_info->unix_name) < 0) {
> DBG_WARNING("SMB_VFS_CONNECT for service '%s' at '%s' failed: %s\n",
> - lp_servicename(talloc_tos(), snum), conn->connectpath,
> + lp_const_servicename(snum), conn->connectpath,
> strerror(errno));
> status = NT_STATUS_UNSUCCESSFUL;
> goto err_root_exit;
> @@ -698,7 +698,7 @@ static NTSTATUS make_connection_snum(struct smbXsrv_connection *xconn,
> /* execute any "root preexec = " line */
> if (*lp_root_preexec(talloc_tos(), snum)) {
> char *cmd = talloc_sub_advanced(talloc_tos(),
> - lp_servicename(talloc_tos(), SNUM(conn)),
> + lp_const_servicename(SNUM(conn)),
> conn->session_info->unix_info->unix_name,
> conn->connectpath,
> conn->session_info->unix_token->gid,
> @@ -736,7 +736,7 @@ static NTSTATUS make_connection_snum(struct smbXsrv_connection *xconn,
> /* execute any "preexec = " line */
> if (*lp_preexec(talloc_tos(), snum)) {
> char *cmd = talloc_sub_advanced(talloc_tos(),
> - lp_servicename(talloc_tos(), SNUM(conn)),
> + lp_const_servicename(SNUM(conn)),
> conn->session_info->unix_info->unix_name,
> conn->connectpath,
> conn->session_info->unix_token->gid,
> @@ -776,10 +776,10 @@ static NTSTATUS make_connection_snum(struct smbXsrv_connection *xconn,
> */
> if (!lp_widelinks(snum)) {
> if (!canonicalize_connect_path(conn)) {
> - DEBUG(0, ("canonicalize_connect_path failed "
> + DBG_ERR("canonicalize_connect_path failed "
> "for service %s, path %s\n",
> - lp_servicename(talloc_tos(), snum),
> - conn->connectpath));
> + lp_const_servicename(snum),
> + conn->connectpath);
> status = NT_STATUS_BAD_NETWORK_NAME;
> goto err_root_exit;
> }
> @@ -815,15 +815,15 @@ static NTSTATUS make_connection_snum(struct smbXsrv_connection *xconn,
> if ((ret = SMB_VFS_STAT(conn, smb_fname_cpath)) != 0 ||
> !S_ISDIR(smb_fname_cpath->st.st_ex_mode)) {
> if (ret == 0 && !S_ISDIR(smb_fname_cpath->st.st_ex_mode)) {
> - DEBUG(0,("'%s' is not a directory, when connecting to "
> + DBG_ERR("'%s' is not a directory, when connecting to "
> "[%s]\n", conn->connectpath,
> - lp_servicename(talloc_tos(), snum)));
> + lp_const_servicename(snum));
> } else {
> - DEBUG(0,("'%s' does not exist or permission denied "
> + DBG_ERR("'%s' does not exist or permission denied "
> "when connecting to [%s] Error was %s\n",
> conn->connectpath,
> - lp_servicename(talloc_tos(), snum),
> - strerror(errno) ));
> + lp_const_servicename(snum),
> + strerror(errno));
> }
> status = NT_STATUS_BAD_NETWORK_NAME;
> goto err_root_exit;
> @@ -852,7 +852,7 @@ static NTSTATUS make_connection_snum(struct smbXsrv_connection *xconn,
> talloc_tos()) );
> dbgtext( "%s", srv_is_signing_active(xconn) ? "signed " : "");
> dbgtext( "connect to service %s ",
> - lp_servicename(talloc_tos(), snum) );
> + lp_const_servicename(snum) );
> dbgtext( "initially as user %s ",
> conn->session_info->unix_info->unix_name );
> dbgtext( "(uid=%d, gid=%d) ", (int)effuid, (int)effgid );
> @@ -1046,7 +1046,7 @@ connection_struct *make_connection(struct smb_request *req,
> dev, status);
> } else if ((vuser->homes_snum != -1)
> && strequal(service_in,
> - lp_servicename(talloc_tos(), vuser->homes_snum))) {
> + lp_const_servicename(vuser->homes_snum))) {
> DEBUG(5, ("making a connection to 'homes' service [%s] "
> "created at session setup time\n", service_in));
> return make_connection_smb1(req, now,
> @@ -1126,7 +1126,7 @@ void close_cnum(connection_struct *conn, uint64_t vuid)
> get_remote_machine_name(),
> tsocket_address_string(conn->sconn->remote_address,
> talloc_tos()),
> - lp_servicename(talloc_tos(), SNUM(conn))));
> + lp_const_servicename(SNUM(conn))));
>
> /* make sure we leave the directory available for unmount */
> vfs_ChDir(conn, &root_fname);
> @@ -1138,7 +1138,7 @@ void close_cnum(connection_struct *conn, uint64_t vuid)
> if (*lp_postexec(talloc_tos(), SNUM(conn)) &&
> change_to_user(conn, vuid)) {
> char *cmd = talloc_sub_advanced(talloc_tos(),
> - lp_servicename(talloc_tos(), SNUM(conn)),
> + lp_const_servicename(SNUM(conn)),
> conn->session_info->unix_info->unix_name,
> conn->connectpath,
> conn->session_info->unix_token->gid,
> @@ -1154,7 +1154,7 @@ void close_cnum(connection_struct *conn, uint64_t vuid)
> /* execute any "root postexec = " line */
> if (*lp_root_postexec(talloc_tos(), SNUM(conn))) {
> char *cmd = talloc_sub_advanced(talloc_tos(),
> - lp_servicename(talloc_tos(), SNUM(conn)),
> + lp_const_servicename(SNUM(conn)),
> conn->session_info->unix_info->unix_name,
> conn->connectpath,
> conn->session_info->unix_token->gid,
> --
> 2.13.7
>
>
> From f4c1c31f2e932077f3bcc8c2f215c14a37c2145c Mon Sep 17 00:00:00 2001
> From: David Disseldorp <ddiss at samba.org>
> Date: Mon, 25 Jun 2018 02:19:34 +0200
> Subject: [PATCH 2/5] s3/uid: use lp_const_servicename() where possible
>
> The majority of these lp_servicename(talloc_tos(), ...) callers leak
> onto the talloc stackframe. Drop the unnecessary heap allocations.
>
> Signed-off-by: David Disseldorp <ddiss at samba.org>
> ---
> source3/smbd/uid.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/source3/smbd/uid.c b/source3/smbd/uid.c
> index 623d3dec9d5..9d5321cf4cc 100644
> --- a/source3/smbd/uid.c
> +++ b/source3/smbd/uid.c
> @@ -92,7 +92,7 @@ static uint32_t create_share_access_mask(int snum,
> uint32_t share_access = 0;
>
> share_access_check(token,
> - lp_servicename(talloc_tos(), snum),
> + lp_const_servicename(snum),
> MAXIMUM_ALLOWED_ACCESS,
> &share_access);
>
> @@ -150,10 +150,10 @@ NTSTATUS check_user_share_access(connection_struct *conn,
>
> if ((share_access & (FILE_READ_DATA|FILE_WRITE_DATA)) == 0) {
> /* No access, read or write. */
> - DEBUG(3,("user %s connection to %s denied due to share "
> + DBG_NOTICE("user %s connection to %s denied due to share "
> "security descriptor.\n",
> session_info->unix_info->unix_name,
> - lp_servicename(talloc_tos(), snum)));
> + lp_const_servicename(snum));
> return NT_STATUS_ACCESS_DENIED;
> }
>
> @@ -161,9 +161,9 @@ NTSTATUS check_user_share_access(connection_struct *conn,
> !(share_access & FILE_WRITE_DATA)) {
> /* smb.conf allows r/w, but the security descriptor denies
> * write. Fall back to looking at readonly. */
> - readonly_share = True;
> - DEBUG(5,("falling back to read-only access-evaluation due to "
> - "security descriptor\n"));
> + readonly_share = true;
> + DBG_INFO("falling back to read-only access-evaluation due to "
> + "security descriptor\n");
> }
>
> *p_share_access = share_access;
> @@ -318,11 +318,11 @@ static bool change_to_user_internal(connection_struct *conn,
>
> ok = check_user_ok(conn, vuid, session_info, snum);
> if (!ok) {
> - DEBUG(2,("SMB user %s (unix user %s) "
> + DBG_WARNING("SMB user %s (unix user %s) "
> "not permitted access to share %s.\n",
> session_info->unix_info->sanitized_username,
> session_info->unix_info->unix_name,
> - lp_servicename(talloc_tos(), snum)));
> + lp_const_servicename(snum));
> return false;
> }
>
> @@ -419,9 +419,9 @@ bool change_to_user(connection_struct *conn, uint64_t vuid)
> vuser = get_valid_user_struct(conn->sconn, vuid);
> if (vuser == NULL) {
> /* Invalid vuid sent */
> - DEBUG(2,("Invalid vuid %llu used on share %s.\n",
> - (unsigned long long)vuid, lp_servicename(talloc_tos(),
> - snum)));
> + DBG_WARNING("Invalid vuid %llu used on share %s.\n",
> + (unsigned long long)vuid,
> + lp_const_servicename(snum));
> return false;
> }
>
> --
> 2.13.7
>
>
> From 525493467b6900a11fa4f272c5fbe0e69fe75991 Mon Sep 17 00:00:00 2001
> From: David Disseldorp <ddiss at samba.org>
> Date: Mon, 25 Jun 2018 02:08:25 +0200
> Subject: [PATCH 3/5] s3/service: convert lp_force_group() to const
>
> set_conn_force_user_group() and change_to_user_internal() leak onto
> the callers' talloc stackframe. Drop the unnecessary heap allocations.
>
> Signed-off-by: David Disseldorp <ddiss at samba.org>
> ---
> docs-xml/smbdotconf/security/forcegroup.xml | 1 +
> source3/smbd/service.c | 19 +++++++------------
> source3/smbd/uid.c | 3 ++-
> 3 files changed, 10 insertions(+), 13 deletions(-)
>
> diff --git a/docs-xml/smbdotconf/security/forcegroup.xml b/docs-xml/smbdotconf/security/forcegroup.xml
> index d101f1c1b86..b6e729be927 100644
> --- a/docs-xml/smbdotconf/security/forcegroup.xml
> +++ b/docs-xml/smbdotconf/security/forcegroup.xml
> @@ -1,6 +1,7 @@
> <samba:parameter name="force group"
> context="S"
> type="string"
> + constant="1"
> xmlns:samba="http://www.samba.org/samba/DTD/samba-doc">
> <synonym>group</synonym>
> <description>
> diff --git a/source3/smbd/service.c b/source3/smbd/service.c
> index 2e4a1136254..df72172d9b0 100644
> --- a/source3/smbd/service.c
> +++ b/source3/smbd/service.c
> @@ -243,23 +243,18 @@ static NTSTATUS find_forced_group(bool force_user,
> TALLOC_CTX *frame = talloc_stackframe();
> struct dom_sid group_sid;
> enum lsa_SidType type;
> + const char *force_group;
> char *groupname;
> bool user_must_be_member = False;
> gid_t gid;
>
> - groupname = lp_force_group(talloc_tos(), snum);
> - if (groupname == NULL) {
> - DEBUG(1, ("talloc_strdup failed\n"));
> - result = NT_STATUS_NO_MEMORY;
> - goto done;
> - }
> -
> - if (groupname[0] == '+') {
> - user_must_be_member = True;
> - groupname += 1;
> + force_group = lp_force_group(snum);
> + if (force_group[0] == '+') {
> + user_must_be_member = true;
> + force_group += 1;
> }
>
> - groupname = talloc_string_sub(talloc_tos(), groupname,
> + groupname = talloc_string_sub(talloc_tos(), force_group,
> "%S", lp_const_servicename(snum));
> if (groupname == NULL) {
> DEBUG(1, ("talloc_string_sub failed\n"));
> @@ -427,7 +422,7 @@ NTSTATUS set_conn_force_user_group(connection_struct *conn, int snum)
> * any groupid stored for the connecting user.
> */
>
> - if (*lp_force_group(talloc_tos(), snum)) {
> + if (*lp_force_group(snum)) {
>
> status = find_forced_group(
> conn->force_user, snum, conn->session_info->unix_info->unix_name,
> diff --git a/source3/smbd/uid.c b/source3/smbd/uid.c
> index 9d5321cf4cc..b6a754b8cfd 100644
> --- a/source3/smbd/uid.c
> +++ b/source3/smbd/uid.c
> @@ -335,7 +335,8 @@ static bool change_to_user_internal(connection_struct *conn,
> * See if we should force group for this service. If so this overrides
> * any group set in the force user code.
> */
> - if((group_c = *lp_force_group(talloc_tos(), snum))) {
> + group_c = *lp_force_group(snum);
> + if (group_c != '\0') {
>
> SMB_ASSERT(conn->force_group_gid != (gid_t)-1);
>
> --
> 2.13.7
>
>
> From 65f9c2656aacf982cf45fb0f58326a4061536c3c Mon Sep 17 00:00:00 2001
> From: David Disseldorp <ddiss at samba.org>
> Date: Mon, 25 Jun 2018 02:22:31 +0200
> Subject: [PATCH 4/5] s3/service: convert lp_force_user() to const
>
> Avoid set_conn_force_user_group() talloc stackframe leaks in doing so.
>
> Signed-off-by: David Disseldorp <ddiss at samba.org>
> ---
> docs-xml/smbdotconf/security/forceuser.xml | 1 +
> source3/smbd/service.c | 4 ++--
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/docs-xml/smbdotconf/security/forceuser.xml b/docs-xml/smbdotconf/security/forceuser.xml
> index ff5c7a2f3f1..3316afe14ac 100644
> --- a/docs-xml/smbdotconf/security/forceuser.xml
> +++ b/docs-xml/smbdotconf/security/forceuser.xml
> @@ -1,5 +1,6 @@
> <samba:parameter name="force user"
> type="string"
> + constant="1"
> context="S"
> xmlns:samba="http://www.samba.org/samba/DTD/samba-doc">
> <description>
> diff --git a/source3/smbd/service.c b/source3/smbd/service.c
> index df72172d9b0..2fa7c608a2b 100644
> --- a/source3/smbd/service.c
> +++ b/source3/smbd/service.c
> @@ -372,7 +372,7 @@ NTSTATUS set_conn_force_user_group(connection_struct *conn, int snum)
> {
> NTSTATUS status;
>
> - if (*lp_force_user(talloc_tos(), snum)) {
> + if (*lp_force_user(snum)) {
>
> /*
> * Replace conn->session_info with a completely faked up one
> @@ -384,7 +384,7 @@ NTSTATUS set_conn_force_user_group(connection_struct *conn, int snum)
> struct auth_session_info *forced_serverinfo;
> bool guest;
>
> - fuser = talloc_string_sub(conn, lp_force_user(talloc_tos(), snum), "%S",
> + fuser = talloc_string_sub(conn, lp_force_user(snum), "%S",
> lp_const_servicename(snum));
> if (fuser == NULL) {
> return NT_STATUS_NO_MEMORY;
> --
> 2.13.7
>
>
> From 100267799bc321f136aef8ef66023b83bf84750c Mon Sep 17 00:00:00 2001
> From: David Disseldorp <ddiss at samba.org>
> Date: Mon, 25 Jun 2018 02:28:41 +0200
> Subject: [PATCH 5/5] s3/loadparm: fix a few talloc stackframe leaks
>
> Signed-off-by: David Disseldorp <ddiss at samba.org>
> ---
> source3/param/loadparm.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
> index ecff22eb638..2f4bdfcbd9d 100644
> --- a/source3/param/loadparm.c
> +++ b/source3/param/loadparm.c
> @@ -1530,6 +1530,7 @@ bool lp_add_home(const char *pszHomename, int iDefaultService,
> const char *user, const char *pszHomedir)
> {
> int i;
> + char *global_path;
>
> if (pszHomename == NULL || user == NULL || pszHomedir == NULL ||
> pszHomedir[0] == '\0') {
> @@ -1541,12 +1542,13 @@ bool lp_add_home(const char *pszHomename, int iDefaultService,
> if (i < 0)
> return false;
>
> + global_path = lp_path(talloc_tos(), GLOBAL_SECTION_SNUM);
> if (!(*(ServicePtrs[iDefaultService]->path))
> - || strequal(ServicePtrs[iDefaultService]->path,
> - lp_path(talloc_tos(), GLOBAL_SECTION_SNUM))) {
> + || strequal(ServicePtrs[iDefaultService]->path, global_path)) {
> lpcfg_string_set(ServicePtrs[i], &ServicePtrs[i]->path,
> pszHomedir);
> }
> + TALLOC_FREE(global_path);
>
> if (!(*(ServicePtrs[i]->comment))) {
> char *comment = talloc_asprintf(talloc_tos(), "Home directory of %s", user);
> @@ -4156,6 +4158,7 @@ void lp_dump(FILE *f, bool show_defaults, int maxtoprint)
> fprintf(f,"\n");
> lp_dump_one(f, show_defaults, iService);
> }
> + TALLOC_FREE(lp_ctx);
> }
>
> /***************************************************************************
> @@ -4209,7 +4212,7 @@ int lp_servicenumber(const char *pszServiceName)
>
> if (!usershare_exists(iService, &last_mod)) {
> /* Remove the share security tdb entry for it. */
> - delete_share_security(lp_servicename(talloc_tos(), iService));
> + delete_share_security(lp_const_servicename(iService));
> /* Remove it from the array. */
> free_service_byindex(iService);
> /* Doesn't exist anymore. */
> @@ -4526,10 +4529,10 @@ void widelinks_warning(int snum)
> }
>
> if (lp_unix_extensions() && lp_wide_links(snum)) {
> - DEBUG(0,("Share '%s' has wide links and unix extensions enabled. "
> + DBG_ERR("Share '%s' has wide links and unix extensions enabled. "
> "These parameters are incompatible. "
> "Wide links will be disabled for this share.\n",
> - lp_servicename(talloc_tos(), snum) ));
> + lp_const_servicename(snum));
> }
> }
>
> --
> 2.13.7
>
More information about the samba-technical
mailing list