NetBSD Problem Report #57721

From www@netbsd.org  Fri Nov 24 15:45:00 2023
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))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 19CE91A9238
	for <gnats-bugs@gnats.NetBSD.org>; Fri, 24 Nov 2023 15:45:00 +0000 (UTC)
Message-Id: <20231124154458.EF07D1A9239@mollari.NetBSD.org>
Date: Fri, 24 Nov 2023 15:44:58 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: pthread_attr_setstack incorrectly adjusts address as if for guard page
X-Send-Pr-Version: www-1.0

>Number:         57721
>Category:       lib
>Synopsis:       pthread_attr_setstack incorrectly adjusts address as if for guard page
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    lib-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Nov 24 15:45:00 +0000 2023
>Closed-Date:    Sat Dec 09 14:15:00 +0000 2023
>Last-Modified:  Sat Dec 09 14:15:00 +0000 2023
>Originator:     Taylor R Campbell
>Release:        current
>Organization:
The NetBSD Threadstacktion
>Environment:
>Description:
pthread_attr_setstack is required by POSIX to make any threads with the attribute _not_ have a guard page:

> If the stackaddr attribute has been set (that is, the caller is allocating and managing its own thread stacks), the guardsize attribute shall be ignored and no protection shall be provided by the implementation. It is the responsibility of the application to manage stack overflow along with stack allocation and management in this case.
> https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_attr_getguardsize.html

NetBSD's libpthread correctly avoids mprotecting any pages PROT_NONE for threads with attributes having pthread_attr_setstack, but it incorrectly adjusts the stack address for the (possibly default) guard size.
>How-To-Repeat:
	void *stackaddr = ...;
	size_t stacksize = ...;
	pthread_attr_t attr;
	pthread_t t;
	int error;

	error = pthread_attr_init(&attr);
	if (error)
		errc(EXIT_FAILURE, error, "pthread_attr_init");
	error = pthread_attr_setstack(&attr, stackaddr, stacksize);
	if (error)
		errc(EXIT_FAILURE, error, "pthread_attr_setstack");
	error = pthread_create(&t, &attr, ...);
	if (error)
		errc(EXIT_FAILURE, error, "pthread_create");

A thread created this way will have a pthread_attr_getstack yield a stack address of (char *)stackaddr + sysctl(vm.thread_guard_size), not stackaddr as it should.
>Fix:
Yes, please!

>Release-Note:

>Audit-Trail:
From: Taylor R Campbell <riastradh@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc: netbsd-bugs@NetBSD.org, wiz@NetBSD.org
Subject: Re: lib/57721: pthread_attr_setstack incorrectly adjusts address as if for guard page
Date: Fri, 24 Nov 2023 15:57:17 +0000

 The workaround is to use pthread_attr_setguardsize(..., 0) too, but
 this shouldn't be necessary.  Fortunately this won't leave anything
 broken once we fix it, so it's a safe workaround to just start using
 now, like wiz@ did for devel/p5-Coco in pkgsrc.

From: Taylor R Campbell <riastradh@NetBSD.org>
To: gnats-bugs@NetBSD.org
Cc: netbsd-bugs@NetBSD.org
Subject: Re: lib/57721: pthread_attr_setstack incorrectly adjusts address as if for guard page
Date: Fri, 24 Nov 2023 16:00:40 +0000

 Citation for POSIX quote:

 https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_attr_get=
 guardsize.html

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57721 CVS commit: src
Date: Fri, 24 Nov 2023 16:21:17 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Fri Nov 24 16:21:17 UTC 2023

 Modified Files:
 	src/distrib/sets/lists/debug: mi
 	src/distrib/sets/lists/tests: mi
 	src/tests/lib/libpthread: Makefile
 Added Files:
 	src/tests/lib/libpthread: t_stack.c

 Log Message:
 pthread: Add tests for pthread user stack allocation.

 PR lib/57721

 XXX pullup-10
 XXX pullup-9
 XXX pullup-8


 To generate a diff of this commit:
 cvs rdiff -u -r1.423 -r1.424 src/distrib/sets/lists/debug/mi
 cvs rdiff -u -r1.1296 -r1.1297 src/distrib/sets/lists/tests/mi
 cvs rdiff -u -r1.15 -r1.16 src/tests/lib/libpthread/Makefile
 cvs rdiff -u -r0 -r1.1 src/tests/lib/libpthread/t_stack.c

 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/57721 CVS commit: src/tests/lib/libpthread
Date: Mon, 27 Nov 2023 22:15:08 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Mon Nov 27 22:15:08 UTC 2023

 Modified Files:
 	src/tests/lib/libpthread: t_stack.c

 Log Message:
 libpthread/t_stack: Make this more robust to the guard size bug.

 Make sure to allocate enough space for the thread's stack for a guard
 even though there shouldn't be one, so that when we run the thread,
 it doesn't start with the stack pointer pointing into someone else's
 allocation (like malloc) causing stack frames to trash another data
 structure -- or causing the user of that data structure to trash the
 stack frames.

 PR lib/57721

 XXX pullup-10
 XXX pullup-9
 XXX pullup-8


 To generate a diff of this commit:
 cvs rdiff -u -r1.1 -r1.2 src/tests/lib/libpthread/t_stack.c

 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/57721 CVS commit: src/tests/lib/libpthread
Date: Mon, 27 Nov 2023 22:17:41 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Mon Nov 27 22:17:41 UTC 2023

 Modified Files:
 	src/tests/lib/libpthread: t_stack.c

 Log Message:
 libpthread/t_stack: Omit needless cast in previous.

 Arose from an earlier draft of the change.

 PR lib/57721

 XXX pullup-10
 XXX pullup-9
 XXX pullup-8


 To generate a diff of this commit:
 cvs rdiff -u -r1.2 -r1.3 src/tests/lib/libpthread/t_stack.c

 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/57721 CVS commit: src/tests/lib/libpthread
Date: Mon, 27 Nov 2023 22:18:29 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Mon Nov 27 22:18:29 UTC 2023

 Modified Files:
 	src/tests/lib/libpthread: t_stack.c

 Log Message:
 libpthread/t_stack: Appease gcc12 maybe-uninitialized warning.

 The jmp_buf is not, in fact, uninitialized at the point of use, but
 it doesn't hurt to narrow the scope a bit to between when the jmp_buf
 is initialized by setjmp, and when the signal handler might be called
 after sigaction.

 Noted by prlw1.

 PR lib/57721

 XXX pullup-10
 XXX pullup-9
 XXX pullup-8


 To generate a diff of this commit:
 cvs rdiff -u -r1.3 -r1.4 src/tests/lib/libpthread/t_stack.c

 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/57721 CVS commit: src/tests/lib/libpthread
Date: Tue, 28 Nov 2023 00:27:05 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Tue Nov 28 00:27:05 UTC 2023

 Modified Files:
 	src/tests/lib/libpthread: t_stack.c

 Log Message:
 libpthread/t_stack: Fix format string for size_t.

 Tested this on i386 since that had been crashing before, but i386
 doesn't see %zu for unsigned int as a problem.

 PR lib/57721

 XXX pullup-10
 XXX pullup-9
 XXX pullup-8


 To generate a diff of this commit:
 cvs rdiff -u -r1.4 -r1.5 src/tests/lib/libpthread/t_stack.c

 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/57721 CVS commit: src
Date: Tue, 28 Nov 2023 02:54:33 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Tue Nov 28 02:54:33 UTC 2023

 Modified Files:
 	src/lib/libpthread: pthread.c
 	src/tests/lib/libpthread: t_stack.c

 Log Message:
 pthread: Don't adjust user-allocated stack addresses by guardsize.

 PR lib/57721

 XXX pullup-10
 XXX pullup-9
 XXX pullup-8


 To generate a diff of this commit:
 cvs rdiff -u -r1.183 -r1.184 src/lib/libpthread/pthread.c
 cvs rdiff -u -r1.5 -r1.6 src/tests/lib/libpthread/t_stack.c

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

State-Changed-From-To: open->needs-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Tue, 28 Nov 2023 02:58:22 +0000
State-Changed-Why:
test and fix committed to HEAD


State-Changed-From-To: needs-pullups->pending-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Tue, 28 Nov 2023 03:08:47 +0000
State-Changed-Why:
pullup-10 #478 https://releng.netbsd.org/cgi-bin/req-10.cgi?show=478
pullup-9 and pullup-8 stalled on minor set list conflicts; patches welcome


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57721 CVS commit: [netbsd-10] src
Date: Tue, 28 Nov 2023 13:17:11 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Tue Nov 28 13:17:11 UTC 2023

 Modified Files:
 	src/distrib/sets/lists/debug [netbsd-10]: mi
 	src/distrib/sets/lists/tests [netbsd-10]: mi
 	src/lib/libpthread [netbsd-10]: pthread.c
 	src/tests/lib/libpthread [netbsd-10]: Makefile
 Added Files:
 	src/tests/lib/libpthread [netbsd-10]: t_stack.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #478):

 	tests/lib/libpthread/Makefile: revision 1.16
 	lib/libpthread/pthread.c: revision 1.184
 	distrib/sets/lists/debug/mi: revision 1.424
 	distrib/sets/lists/tests/mi: revision 1.1297
 	tests/lib/libpthread/t_stack.c: revision 1.1
 	tests/lib/libpthread/t_stack.c: revision 1.2
 	tests/lib/libpthread/t_stack.c: revision 1.3
 	tests/lib/libpthread/t_stack.c: revision 1.4
 	tests/lib/libpthread/t_stack.c: revision 1.5
 	tests/lib/libpthread/t_stack.c: revision 1.6

 pthread: Add tests for pthread user stack allocation.
 PR lib/57721

 libpthread/t_stack: Make this more robust to the guard size bug.
 Make sure to allocate enough space for the thread's stack for a guard
 even though there shouldn't be one, so that when we run the thread,
 it doesn't start with the stack pointer pointing into someone else's
 allocation (like malloc) causing stack frames to trash another data
 structure -- or causing the user of that data structure to trash the
 stack frames.
 PR lib/57721

 libpthread/t_stack: Omit needless cast in previous.
 Arose from an earlier draft of the change.
 PR lib/57721

 libpthread/t_stack: Appease gcc12 maybe-uninitialized warning.
 The jmp_buf is not, in fact, uninitialized at the point of use, but
 it doesn't hurt to narrow the scope a bit to between when the jmp_buf
 is initialized by setjmp, and when the signal handler might be called
 after sigaction.
 Noted by prlw1.
 PR lib/57721

 libpthread/t_stack: Fix format string for size_t.
 Tested this on i386 since that had been crashing before, but i386
 doesn't see %zu for unsigned int as a problem.
 PR lib/57721

 pthread: Don't adjust user-allocated stack addresses by guardsize.
 PR lib/57721


 To generate a diff of this commit:
 cvs rdiff -u -r1.394.2.3 -r1.394.2.4 src/distrib/sets/lists/debug/mi
 cvs rdiff -u -r1.1238.2.4 -r1.1238.2.5 src/distrib/sets/lists/tests/mi
 cvs rdiff -u -r1.181 -r1.181.2.1 src/lib/libpthread/pthread.c
 cvs rdiff -u -r1.15 -r1.15.6.1 src/tests/lib/libpthread/Makefile
 cvs rdiff -u -r0 -r1.6.2.2 src/tests/lib/libpthread/t_stack.c

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

From: Christos Zoulas <christos@zoulas.com>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org,
 campbell+netbsd@mumble.net
Subject: Re: PR/57721 CVS commit: [netbsd-10] src
Date: Tue, 28 Nov 2023 09:30:03 -0500

 Needs also the latest Makefile to fix the SSP build.

 christos

From: Taylor R Campbell <riastradh@NetBSD.org>
To: Christos Zoulas <christos@zoulas.com>
Cc: gnats-bugs@netbsd.org, lib-bug-people@netbsd.org,
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: PR/57721 CVS commit: [netbsd-10] src
Date: Tue, 28 Nov 2023 22:12:59 +0000

 > Date: Tue, 28 Nov 2023 09:30:03 -0500
 > From: Christos Zoulas <christos@zoulas.com>
 > 
 > Needs also the latest Makefile to fix the SSP build.

 How do you provoke an SSP build error?

 I tried building t_stack with USE_SSP=yes, and verified with
 MAKEVERBOSE=2 that it includes `-Wstack-protector -fstack-protector'
 but not `-Wno-error=stack-protector', on i386, amd64, and alpha.
 Built just fine on i386 and amd64, and failed on alpha only because
 -fstack-protector isn't supported.

From: Christos Zoulas <christos@zoulas.com>
To: gnats-bugs@netbsd.org
Cc: lib-bug-people@netbsd.org,
 gnats-admin@netbsd.org,
 netbsd-bugs@netbsd.org,
 campbell+netbsd@mumble.net
Subject: Re: PR/57721 CVS commit: [netbsd-10] src
Date: Tue, 28 Nov 2023 18:04:46 -0500

 Sorry that was in t_setrlimit because it uses alloca(). Got confused by =
 the stack commits.

 christos

From: Taylor R Campbell <riastradh@NetBSD.org>
To: Christos Zoulas <christos@zoulas.com>
Cc: gnats-bugs@netbsd.org, lib-bug-people@netbsd.org,
	gnats-admin@netbsd.org, netbsd-bugs@netbsd.org, campbell+netbsd@mumble.net
Subject: Re: PR/57721 CVS commit: [netbsd-10] src
Date: Wed, 29 Nov 2023 01:57:23 +0000

 > Date: Tue, 28 Nov 2023 18:04:46 -0500
 > From: Christos Zoulas <christos@zoulas.com>
 > 
 > Sorry that was in t_setrlimit because it uses alloca(). Got confused
 > by the stack commits.

 Can you request pullups for that change too, referencing PR 57711?

 https://gnats.NetBSD.org/57711

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57721 CVS commit: [netbsd-9] src
Date: Sat, 9 Dec 2023 13:24:24 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Sat Dec  9 13:24:24 UTC 2023

 Modified Files:
 	src/distrib/sets/lists/debug [netbsd-9]: mi
 	src/distrib/sets/lists/tests [netbsd-9]: mi
 	src/lib/libpthread [netbsd-9]: pthread.c
 	src/tests/lib/libpthread [netbsd-9]: Makefile
 Added Files:
 	src/tests/lib/libpthread [netbsd-9]: t_stack.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #1775):

 	tests/lib/libpthread/Makefile: revision 1.16
 	lib/libpthread/pthread.c: revision 1.184
 	distrib/sets/lists/debug/mi: revision 1.424
 	distrib/sets/lists/tests/mi: revision 1.1297
 	tests/lib/libpthread/t_stack.c: revision 1.1
 	tests/lib/libpthread/t_stack.c: revision 1.2
 	tests/lib/libpthread/t_stack.c: revision 1.3
 	tests/lib/libpthread/t_stack.c: revision 1.4
 	tests/lib/libpthread/t_stack.c: revision 1.5
 	tests/lib/libpthread/t_stack.c: revision 1.6

 pthread: Add tests for pthread user stack allocation.
 PR lib/57721

 libpthread/t_stack: Make this more robust to the guard size bug.
 Make sure to allocate enough space for the thread's stack for a guard
 even though there shouldn't be one, so that when we run the thread,
 it doesn't start with the stack pointer pointing into someone else's
 allocation (like malloc) causing stack frames to trash another data
 structure -- or causing the user of that data structure to trash the
 stack frames.
 PR lib/57721

 libpthread/t_stack: Omit needless cast in previous.
 Arose from an earlier draft of the change.
 PR lib/57721

 libpthread/t_stack: Appease gcc12 maybe-uninitialized warning.
 The jmp_buf is not, in fact, uninitialized at the point of use, but
 it doesn't hurt to narrow the scope a bit to between when the jmp_buf
 is initialized by setjmp, and when the signal handler might be called
 after sigaction.
 Noted by prlw1.
 PR lib/57721

 libpthread/t_stack: Fix format string for size_t.
 Tested this on i386 since that had been crashing before, but i386
 doesn't see %zu for unsigned int as a problem.
 PR lib/57721

 pthread: Don't adjust user-allocated stack addresses by guardsize.
 PR lib/57721


 To generate a diff of this commit:
 cvs rdiff -u -r1.285.2.2 -r1.285.2.3 src/distrib/sets/lists/debug/mi
 cvs rdiff -u -r1.818.2.3 -r1.818.2.4 src/distrib/sets/lists/tests/mi
 cvs rdiff -u -r1.153.2.2 -r1.153.2.3 src/lib/libpthread/pthread.c
 cvs rdiff -u -r1.14 -r1.14.2.1 src/tests/lib/libpthread/Makefile
 cvs rdiff -u -r0 -r1.6.4.2 src/tests/lib/libpthread/t_stack.c

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

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57721 CVS commit: [netbsd-8] src
Date: Sat, 9 Dec 2023 13:36:03 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Sat Dec  9 13:36:03 UTC 2023

 Modified Files:
 	src/distrib/sets/lists/debug [netbsd-8]: mi
 	src/distrib/sets/lists/tests [netbsd-8]: mi
 	src/lib/libpthread [netbsd-8]: pthread.c
 	src/tests/lib/libpthread [netbsd-8]: Makefile
 Added Files:
 	src/tests/lib/libpthread [netbsd-8]: t_stack.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #1924):

 	tests/lib/libpthread/Makefile: revision 1.16
 	lib/libpthread/pthread.c: revision 1.184
 	distrib/sets/lists/debug/mi: revision 1.424
 	distrib/sets/lists/tests/mi: revision 1.1297
 	tests/lib/libpthread/t_stack.c: revision 1.1
 	tests/lib/libpthread/t_stack.c: revision 1.2
 	tests/lib/libpthread/t_stack.c: revision 1.3
 	tests/lib/libpthread/t_stack.c: revision 1.4
 	tests/lib/libpthread/t_stack.c: revision 1.5
 	tests/lib/libpthread/t_stack.c: revision 1.6

 pthread: Add tests for pthread user stack allocation.
 PR lib/57721

 libpthread/t_stack: Make this more robust to the guard size bug.
 Make sure to allocate enough space for the thread's stack for a guard
 even though there shouldn't be one, so that when we run the thread,
 it doesn't start with the stack pointer pointing into someone else's
 allocation (like malloc) causing stack frames to trash another data
 structure -- or causing the user of that data structure to trash the
 stack frames.
 PR lib/57721

 libpthread/t_stack: Omit needless cast in previous.
 Arose from an earlier draft of the change.
 PR lib/57721

 libpthread/t_stack: Appease gcc12 maybe-uninitialized warning.
 The jmp_buf is not, in fact, uninitialized at the point of use, but
 it doesn't hurt to narrow the scope a bit to between when the jmp_buf
 is initialized by setjmp, and when the signal handler might be called
 after sigaction.
 Noted by prlw1.
 PR lib/57721

 libpthread/t_stack: Fix format string for size_t.
 Tested this on i386 since that had been crashing before, but i386
 doesn't see %zu for unsigned int as a problem.
 PR lib/57721

 pthread: Don't adjust user-allocated stack addresses by guardsize.
 PR lib/57721


 To generate a diff of this commit:
 cvs rdiff -u -r1.216.2.10 -r1.216.2.11 src/distrib/sets/lists/debug/mi
 cvs rdiff -u -r1.752.2.11 -r1.752.2.12 src/distrib/sets/lists/tests/mi
 cvs rdiff -u -r1.147.8.3 -r1.147.8.4 src/lib/libpthread/pthread.c
 cvs rdiff -u -r1.12.6.1 -r1.12.6.2 src/tests/lib/libpthread/Makefile
 cvs rdiff -u -r0 -r1.6.6.2 src/tests/lib/libpthread/t_stack.c

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

State-Changed-From-To: pending-pullups->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Sat, 09 Dec 2023 14:15:00 +0000
State-Changed-Why:
fixed and pulled up to 10, 9, 8


>Unformatted:

Home	PR Database Search