The Samba-Bugzilla – Attachment 18944 Details for
Bug 14978
Intermittent segfault+coredump when volume_label calls strlen
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[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:
Proposed patch.
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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Actions:
View
Attachments on
bug 14978
:
17162
|
17608
|
17881
|
18642
|
18643
|
18943
| 18944