Attachment 18944 Details for Bug 14978 – Proposed patch.

[patch] Proposed patch.

bug-14978.patch (text/plain), 10.47 KB, created by Jeremy Allison on 2026-04-10 22:21:32 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2026-04-10 22:21:32 UTC

Size: 10.47 KB

patch

obsolete

>From bada89f2c70725620903031fefcd3fc062a57b36 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jallison@ciq.com>
>Date: Fri, 10 Apr 2026 14:15:48 -0700
>Subject: [PATCH 1/5] s3:loadparm: add lp_register_snum_in_use_fn() callback
> registration
>
>Add a mechanism for smbd to register a callback that checks whether
>a service number is currently in use by any active connection.
>
>This will be used by subsequent commits to guard free_service_byindex()
>calls in lp_servicenumber() and other sites that currently destroy
>services without checking if they are in use, which can leave active
>connections holding stale service numbers that lead to NULL pointer
>dereferences.
>
>The callback is registered by smbd during smbd_process() startup via
>connections_snum_used. Non-smbd programs (testparm, net, etc.) leave the
>callback as NULL, meaning no connections exist and it is always safe
>to free services.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14978
>
>Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source3/param/loadparm.c    | 15 +++++++++++++++
> source3/param/loadparm.h    |  2 ++
> source3/smbd/smb2_process.c |  1 +
> 3 files changed, 18 insertions(+)
>
>diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
>index b081b917a82..02ad996c265 100644
>--- a/source3/param/loadparm.c
>+++ b/source3/param/loadparm.c
>@@ -119,6 +119,21 @@ static bool defaults_saved = false;
> 
> static struct loadparm_global Globals;
> 
>+/*
>+ * Callback to check if a service number is currently in use
>+ * by any active connection. Registered by smbd at startup.
>+ * When NULL (non-smbd programs), no connections exist so
>+ * it is always safe to free services.
>+ */
>+static bool (*snum_in_use)(struct smbd_server_connection *unused,
>+			   int snum) = NULL;
>+
>+void lp_register_snum_in_use_fn(bool (*fn)(struct smbd_server_connection *,
>+					   int))
>+{
>+	snum_in_use = fn;
>+}
>+
> /* This is a default service used to prime a services structure */
> static const struct loadparm_service _sDefault =
> {
>diff --git a/source3/param/loadparm.h b/source3/param/loadparm.h
>index 72773a8b2ec..43f6c61804d 100644
>--- a/source3/param/loadparm.h
>+++ b/source3/param/loadparm.h
>@@ -144,6 +144,8 @@ void lp_killunused(struct smbd_server_connection *sconn,
> 		   bool (*snumused) (struct smbd_server_connection *, int));
> void lp_kill_all_services(void);
> void lp_killservice(int iServiceIn);
>+void lp_register_snum_in_use_fn(bool (*fn)(struct smbd_server_connection *,
>+					   int));
> const char* server_role_str(uint32_t role);
> enum usershare_err parse_usershare_file(TALLOC_CTX *ctx,
> 			SMB_STRUCT_STAT *psbuf,
>diff --git a/source3/smbd/smb2_process.c b/source3/smbd/smb2_process.c
>index 9290714e8ae..f16a1496438 100644
>--- a/source3/smbd/smb2_process.c
>+++ b/source3/smbd/smb2_process.c
>@@ -2031,6 +2031,7 @@ void smbd_process(struct tevent_context *ev_ctx,
> 	/* this is needed so that we get decent entries
> 	   in smbstatus for port 445 connects */
> 	set_remote_machine_name(remaddr, false);
>+	lp_register_snum_in_use_fn(connections_snum_used);
> 	reload_services(sconn, conn_snum_used, true);
> 	sub_set_socket_ids(remaddr,
> 			   sconn->remote_hostname,
>-- 
>2.47.3
>
>
>From df7ee7f0fba31d55b8bf2a143b8c885e24baf2eb Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jallison@ciq.com>
>Date: Fri, 10 Apr 2026 14:19:01 -0700
>Subject: [PATCH 2/5] s3:loadparm: guard free_service_byindex() in
> lp_servicenumber() with snum_in_use check
>MIME-Version: 1.0
>Content-Type: text/plain; charset=UTF-8
>Content-Transfer-Encoding: 8bit
>
>lp_servicenumber() calls free_service_byindex() to destroy usershare
>services when usershare_exists() returns false or when the usershare
>file has been modified. This is unsafe because active connections may
>still hold the service number â the destroyed service leaves a NULL
>ServicePtrs[] entry that causes a NULL pointer dereference when the
>connection subsequently calls lp_servicename() or similar functions.
>
>The crash path is:
>  get_referred_path() -> lp_servicenumber() -> usershare_exists()
>  fails (e.g. EACCES) -> free_service_byindex() destroys service ->
>  later request on same connection -> volume_label() ->
>  lp_servicename() -> FN_LOCAL_SUBSTITUTED_STRING falls back to
>  sDefault.szService (NULL) -> strlen(NULL) -> SIGSEGV
>
>Guard both free_service_byindex() call sites with the snum_in_use
>callback registered in the previous commit. When the service is in
>use by an active connection, skip the destruction and let the
>periodic load_usershare_shares() mark-and-sweep handle cleanup
>safely via its conn_snum_used() check.
>
>When snum_in_use is NULL (non-smbd programs), the original behaviour
>is preserved â services are freed immediately since no connections
>can exist.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14978
>
>Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source3/param/loadparm.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
>diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
>index 02ad996c265..e85b52fdc6d 100644
>--- a/source3/param/loadparm.c
>+++ b/source3/param/loadparm.c
>@@ -4412,6 +4412,15 @@ int lp_servicenumber(const char *pszServiceName)
> 		struct timespec last_mod;
> 
> 		if (!usershare_exists(iService, &last_mod)) {
>+			if (snum_in_use != NULL &&
>+			    snum_in_use(NULL, iService)) {
>+				/*
>+				 * Service is in use, don't destroy it.
>+				 * The periodic load_usershare_shares()
>+				 * sweep will clean it up safely.
>+				 */
>+				return GLOBAL_SECTION_SNUM;
>+			}
> 			/* Remove the share security tdb entry for it. */
> 			delete_share_security(lp_const_servicename(iService));
> 			/* Remove it from the array. */
>@@ -4423,6 +4432,15 @@ int lp_servicenumber(const char *pszServiceName)
> 		/* Has it been modified ? If so delete and reload. */
> 		if (timespec_compare(&ServicePtrs[iService]->usershare_last_mod,
> 				     &last_mod) < 0) {
>+			if (snum_in_use != NULL &&
>+			    snum_in_use(NULL, iService)) {
>+				/*
>+				 * Service is in use, defer the reload
>+				 * to load_usershare_shares() which can
>+				 * safely handle in-use services.
>+				 */
>+				return iService;
>+			}
> 			/* Remove it from the array. */
> 			free_service_byindex(iService);
> 			/* and now reload it. */
>-- 
>2.47.3
>
>
>From e19ceefe77d1b1a5c6f7a30263d9c9a09e4a4b3a Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jallison@ciq.com>
>Date: Fri, 10 Apr 2026 14:20:45 -0700
>Subject: [PATCH 3/5] s3:srvsvc: guard lp_killservice() in
> _srvsvc_NetShareDel() with connections_snum_used check
>
>_srvsvc_NetShareDel() unconditionally calls lp_killservice() to
>destroy the service after deleting a share via RPC. If any active
>connection is still using this service number, the destroyed service
>can cause a NULL pointer dereference on subsequent requests.
>
>Guard the call with connections_snum_used() so the service is only
>freed when no connections are using it. The periodic
>load_usershare_shares() sweep will clean up the stale service once
>all connections have disconnected.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14978
>
>Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source3/rpc_server/srvsvc/srv_srvsvc_nt.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/source3/rpc_server/srvsvc/srv_srvsvc_nt.c b/source3/rpc_server/srvsvc/srv_srvsvc_nt.c
>index bcc99e576f8..94130be0352 100644
>--- a/source3/rpc_server/srvsvc/srv_srvsvc_nt.c
>+++ b/source3/rpc_server/srvsvc/srv_srvsvc_nt.c
>@@ -2467,7 +2467,9 @@ WERROR _srvsvc_NetShareDel(struct pipes_struct *p,
> 	/* Delete the SD in the database. */
> 	delete_share_security(share_name);
> 
>-	lp_killservice(snum);
>+	if (!connections_snum_used(NULL, snum)) {
>+		lp_killservice(snum);
>+	}
> 
> 	return WERR_OK;
> }
>-- 
>2.47.3
>
>
>From c39998ac7dfb0f827c89f6040763465fc2fe019d Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jallison@ciq.com>
>Date: Fri, 10 Apr 2026 14:21:55 -0700
>Subject: [PATCH 4/5] s3:smbd: guard lp_killservice() in
> delete_and_reload_printers() with connections_snum_used check
>
>delete_and_reload_printers() unconditionally calls lp_killservice()
>to destroy autoloaded printer services that are no longer in the
>printer list. If any active connection is still using the printer
>service number, the destroyed service can cause a NULL pointer
>dereference on subsequent requests.
>
>Guard the call with connections_snum_used() so the service is only
>freed when no connections are using it.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14978
>
>Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source3/smbd/server_reload.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/source3/smbd/server_reload.c b/source3/smbd/server_reload.c
>index d3322d12f6a..eba87cc98ca 100644
>--- a/source3/smbd/server_reload.c
>+++ b/source3/smbd/server_reload.c
>@@ -109,7 +109,8 @@ void delete_and_reload_printers(void)
> 
> 		/* check printer, but avoid removing non-autoloaded printers */
> 		if (lp_autoloaded(snum) &&
>-		    !printer_list_printername_exists(pname)) {
>+		    !printer_list_printername_exists(pname) &&
>+		    !connections_snum_used(NULL, snum)) {
> 			lp_killservice(snum);
> 		}
> 	}
>-- 
>2.47.3
>
>
>From b0182f2488772ac40ff3b3ff7d933f360fc16b8d Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jallison@ciq.com>
>Date: Fri, 10 Apr 2026 14:24:34 -0700
>Subject: [PATCH 5/5] s3:loadparm: fix NULL pointer dereference in
> volume_label()
>
>volume_label() calls lp_servicename() as a fallback when lp_volume()
>returns an empty string. lp_servicename() is a FN_LOCAL_SUBSTITUTED_STRING
>that falls back to sDefault.szService when the service is invalid. Since
>sDefault.szService is initialized to NULL and is never set by
>init_globals(), the substitution returns NULL, and the subsequent
>strlen() call crashes with a segmentation fault.
>
>Add a NULL guard so volume_label() returns an empty string instead
>of crashing.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=14978
>
>Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>---
> source3/param/loadparm.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
>index e85b52fdc6d..dc0d8523172 100644
>--- a/source3/param/loadparm.c
>+++ b/source3/param/loadparm.c
>@@ -4471,6 +4471,9 @@ const char *volume_label(TALLOC_CTX *ctx, int snum)
> 	if (!*label) {
> 		label = lp_servicename(ctx, lp_sub, snum);
> 	}
>+	if (label == NULL) {
>+		label = "";
>+	}
> 
> 	/*
> 	 * Volume label can be a max of 32 bytes. Make sure to truncate
>-- 
>2.47.3
>

Actions: View

Attachments on bug 14978: 17162 | 17608 | 17881 | 18642 | 18643 | 18943 | 18944