NetBSD Problem Report #59401

From www@netbsd.org  Tue May  6 22:43:04 2025
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
	 key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256
	 client-signature RSA-PSS (2048 bits) client-digest SHA256)
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 1100F1A923D
	for <gnats-bugs@gnats.NetBSD.org>; Tue,  6 May 2025 22:43:04 +0000 (UTC)
Message-Id: <20250506224302.A66161A923E@mollari.NetBSD.org>
Date: Tue,  6 May 2025 22:43:02 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: libc: thr_sigsetmask definition is incoherent
X-Send-Pr-Version: www-1.0

>Number:         59401
>Category:       lib
>Synopsis:       libc: thr_sigsetmask definition is incoherent
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    lib-bug-people
>State:          open
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Tue May 06 22:45:00 +0000 2025
>Last-Modified:  Tue May 06 23:20:03 +0000 2025
>Originator:     Taylor R Campbell
>Release:        
>Organization:
The NetBSD _Reentrancy
>Environment:
>Description:
If _REENTRANT is defined (default), thr_sigsetmask gets you either:

- a wrapper for sigprocmask(2), in single-threaded applications; or
- a per-thread version of sigprocmask(2), in multi-threaded applications linked against libpthread.

But if _REENTRANT is _not_ defined, thr_sigsetmask is a macro that expands to nothing.

I recently wrote some code (for softfloat SIGFPE delivery, https://mail-index.netbsd.org/source-changes/2025/04/27/msg156650.html) assuming that it would block signals for the current thread -- not realizing it does nothing in non-_REENTRANT builds.

This thr_sigsetmask definition appears to date back to the import of Sun TI-RPC code back in 2000:

https://mail-index.netbsd.org/source-changes/2000/06/02/msg077344.html

(At the time, _REENTRANT was called __REENT; it was renamed with the nathanw_sa merge.)

And, the TI-RPC code seems to _rely_ on thr_sigsetmask being a noop when !_REENTRANT:

    105 #ifdef _REENTRANT
    106 #define __rpc_lock_value __isthreaded;
    107 static cond_t	*dg_cv;
    108 #define	release_fd_lock(fd, mask) {		\
    109 	mutex_lock(&clnt_fd_lock);	\
    110 	dg_fd_locks[fd] = 0;		\
    111 	mutex_unlock(&clnt_fd_lock);	\
    112 	thr_sigsetmask(SIG_SETMASK, &(mask), NULL);	\
    113 	cond_signal(&dg_cv[fd]);	\
    114 }
    115 #else
    116 #define release_fd_lock(fd,mask)
    117 #define __rpc_lock_value 0
    118 #endif

https://nxr.netbsd.org/xref/src/lib/libc/rpc/clnt_dg.c?r=1.33#105

    328 #ifdef _REENTRANT
    329 	sigset_t mask, *maskp = &mask;
    330 #else
    331 	sigset_t *maskp = NULL;
    332 #endif
...
    342 	__clnt_sigfillset(&newmask);
    343 	thr_sigsetmask(SIG_SETMASK, &newmask, &mask);
...
    412 		n = pollts(&cu->cu_pfdp, 1, &ts, maskp);
...
    501 out:
    502 	release_fd_lock(cu->cu_fd, mask);

https://nxr.netbsd.org/xref/src/lib/libc/rpc/clnt_dg.c?r=1.33#328

This strikes me as likely to be incoherent: I bet this code really does expect to have signals masked, but it got lost in translation somewhere along the decades and I can't find the original TI-RPC source that fvdl@ derived the lib/libc/rpc/ code from.  However, we also don't have automatic tests for any signal business in lib/libc/rpc/ code, so I'm reluctant to make changes to it.
>How-To-Repeat:
1. code inspection
2. build libc without _REENTRANT
>Fix:
Yes, please!

Either:

(a) sprinkle more _REENTRANT conditionals (ugh) to use thr_sigsetmask if _REENTRANT and sigprocmask if not (ugh); or
(b) find some way to test changes to signal masking in lib/libc/rpc/, change reentrant.h to do #define thr_sigsetmask(f, n, o) (sigprocmask(f, n, o) ? errno : 0), and change lib/libc/rpc/ to do #define release_fd_lock(fd, lock) thr_sigsetmask(SIG_SETMASK, &(mask), NULL)

(Also: the name `reentrant' is wrong, but it's baked into some historic feature test macros like for lgamma_r(3), so, bleh.)

>Audit-Trail:
From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59401 CVS commit: src/lib/libc
Date: Tue, 6 May 2025 23:18:28 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Tue May  6 23:18:27 UTC 2025

 Modified Files:
 	src/lib/libc/gen: pthread_atfork.c
 	src/lib/libc/softfloat: softfloat-specialize

 Log Message:
 libc: Sprinkle #ifdef _REENTRANT around thr_sigsetmask.

 Workaround -- temporary, I hope -- for:

 PR lib/59401: libc: thr_sigsetmask definition is incoherent


 To generate a diff of this commit:
 cvs rdiff -u -r1.27 -r1.28 src/lib/libc/gen/pthread_atfork.c
 cvs rdiff -u -r1.10 -r1.11 src/lib/libc/softfloat/softfloat-specialize

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/59401 CVS commit: src/lib/libc/rpc
Date: Tue, 6 May 2025 23:18:38 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Tue May  6 23:18:38 UTC 2025

 Modified Files:
 	src/lib/libc/rpc: svc_fdset.c

 Log Message:
 libc: Fix _REENTRANT build.

 This svc_fdset.c has some pretty heinous abuse of thread_key_t, but
 I'll pretend I didn't see that for now so I don't have to wade
 further into the sunrpc business.

 Prompted by:

 PR lib/59401: libc: thr_sigsetmask definition is incoherent
 PR lib/59391: unnecessary __PIC__ conditionals clutter .S files


 To generate a diff of this commit:
 cvs rdiff -u -r1.16 -r1.17 src/lib/libc/rpc/svc_fdset.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

Home	PR Database Search