diff mbox series

[liburing] examples/ucontext-cp.c: cope with variable SIGSTKSZ

Message ID 20210413150319.764600-1-stefanha@redhat.com
State Not Applicable
Headers show
Series [liburing] examples/ucontext-cp.c: cope with variable SIGSTKSZ | expand

Commit Message

Stefan Hajnoczi April 13, 2021, 3:03 p.m. UTC
The size of C arrays at file scope must be constant. The following
compiler error occurs with recent upstream glibc (2.33.9000):

  CC ucontext-cp
  ucontext-cp.c:31:23: error: variably modified ‘stack_buf’ at file scope
  31 |         unsigned char stack_buf[SIGSTKSZ];
     |                       ^~~~~~~~~
  make[1]: *** [Makefile:26: ucontext-cp] Error 1

The following glibc commit changed SIGSTKSZ from a constant value to a
variable:

  commit 6c57d320484988e87e446e2e60ce42816bf51d53
  Author: H.J. Lu <hjl.tools@gmail.com>
  Date:   Mon Feb 1 11:00:38 2021 -0800

    sysconf: Add _SC_MINSIGSTKSZ/_SC_SIGSTKSZ [BZ #20305]
  ...
  +# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)

Allocate the stack buffer explicitly to avoid declaring an array at file
scope.

Cc: H.J. Lu <hjl.tools@gmail.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
Perhaps the glibc change needs to be revised before releasing glibc 2.34
since it might break applications. That's up to the glibc folks. It
doesn't hurt for liburing to take a safer approach that copes with the
SIGSTKSZ change in any case.
---
 examples/ucontext-cp.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Stefan Hajnoczi April 19, 2021, 2:34 p.m. UTC | #1
On Tue, Apr 13, 2021 at 04:03:19PM +0100, Stefan Hajnoczi wrote:
> The size of C arrays at file scope must be constant. The following
> compiler error occurs with recent upstream glibc (2.33.9000):
> 
>   CC ucontext-cp
>   ucontext-cp.c:31:23: error: variably modified ‘stack_buf’ at file scope
>   31 |         unsigned char stack_buf[SIGSTKSZ];
>      |                       ^~~~~~~~~
>   make[1]: *** [Makefile:26: ucontext-cp] Error 1
> 
> The following glibc commit changed SIGSTKSZ from a constant value to a
> variable:
> 
>   commit 6c57d320484988e87e446e2e60ce42816bf51d53
>   Author: H.J. Lu <hjl.tools@gmail.com>
>   Date:   Mon Feb 1 11:00:38 2021 -0800
> 
>     sysconf: Add _SC_MINSIGSTKSZ/_SC_SIGSTKSZ [BZ #20305]
>   ...
>   +# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
> 
> Allocate the stack buffer explicitly to avoid declaring an array at file
> scope.
> 
> Cc: H.J. Lu <hjl.tools@gmail.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> Perhaps the glibc change needs to be revised before releasing glibc 2.34
> since it might break applications. That's up to the glibc folks. It
> doesn't hurt for liburing to take a safer approach that copes with the
> SIGSTKSZ change in any case.

glibc folks, please take a look. The commit referenced above broke
compilation of liburing's tests. It's possible that applications will
hit similar issues. Can you check whether the SIGSTKSZ change needs to
be reverted/fixed before releasing glibc 2.34?

Thanks,
Stefan

> ---
>  examples/ucontext-cp.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/examples/ucontext-cp.c b/examples/ucontext-cp.c
> index 0b2a6b5..ea0c934 100644
> --- a/examples/ucontext-cp.c
> +++ b/examples/ucontext-cp.c
> @@ -28,7 +28,7 @@
>  
>  typedef struct {
>  	struct io_uring *ring;
> -	unsigned char stack_buf[SIGSTKSZ];
> +	unsigned char *stack_buf;
>  	ucontext_t ctx_main, ctx_fnew;
>  } async_context;
>  
> @@ -115,8 +115,13 @@ static int setup_context(async_context *pctx, struct io_uring *ring)
>  		perror("getcontext");
>  		return -1;
>  	}
> -	pctx->ctx_fnew.uc_stack.ss_sp = &pctx->stack_buf;
> -	pctx->ctx_fnew.uc_stack.ss_size = sizeof(pctx->stack_buf);
> +	pctx->stack_buf = malloc(SIGSTKSZ);
> +	if (!pctx->stack_buf) {
> +		perror("malloc");
> +		return -1;
> +	}
> +	pctx->ctx_fnew.uc_stack.ss_sp = pctx->stack_buf;
> +	pctx->ctx_fnew.uc_stack.ss_size = SIGSTKSZ;
>  	pctx->ctx_fnew.uc_link = &pctx->ctx_main;
>  
>  	return 0;
> @@ -174,6 +179,7 @@ static void copy_file_wrapper(arguments_bundle *pbundle)
>  	free(iov.iov_base);
>  	close(pbundle->infd);
>  	close(pbundle->outfd);
> +	free(pbundle->pctx->stack_buf);
>  	free(pbundle->pctx);
>  	free(pbundle);
>  
> -- 
> 2.30.2
>
H.J. Lu April 19, 2021, 6:38 p.m. UTC | #2
On Mon, Apr 19, 2021 at 7:35 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>
> On Tue, Apr 13, 2021 at 04:03:19PM +0100, Stefan Hajnoczi wrote:
> > The size of C arrays at file scope must be constant. The following
> > compiler error occurs with recent upstream glibc (2.33.9000):
> >
> >   CC ucontext-cp
> >   ucontext-cp.c:31:23: error: variably modified ‘stack_buf’ at file scope
> >   31 |         unsigned char stack_buf[SIGSTKSZ];
> >      |                       ^~~~~~~~~
> >   make[1]: *** [Makefile:26: ucontext-cp] Error 1
> >
> > The following glibc commit changed SIGSTKSZ from a constant value to a
> > variable:
> >
> >   commit 6c57d320484988e87e446e2e60ce42816bf51d53
> >   Author: H.J. Lu <hjl.tools@gmail.com>
> >   Date:   Mon Feb 1 11:00:38 2021 -0800
> >
> >     sysconf: Add _SC_MINSIGSTKSZ/_SC_SIGSTKSZ [BZ #20305]
> >   ...
> >   +# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
> >
> > Allocate the stack buffer explicitly to avoid declaring an array at file
> > scope.
> >
> > Cc: H.J. Lu <hjl.tools@gmail.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > Perhaps the glibc change needs to be revised before releasing glibc 2.34
> > since it might break applications. That's up to the glibc folks. It
> > doesn't hurt for liburing to take a safer approach that copes with the
> > SIGSTKSZ change in any case.
>
> glibc folks, please take a look. The commit referenced above broke
> compilation of liburing's tests. It's possible that applications will
> hit similar issues. Can you check whether the SIGSTKSZ change needs to
> be reverted/fixed before releasing glibc 2.34?
>

It won't be changed for glibc 2.34.
Paul Eggert April 20, 2021, 12:05 a.m. UTC | #3
On 4/19/21 7:34 AM, Stefan Hajnoczi via Libc-alpha wrote:
> The commit referenced above broke
> compilation of liburing's tests. It's possible that applications will
> hit similar issues.

Yes, other applications have already had similar issues, and we've fixed 
that by recoding them to not assume that SIGSTKSZ is an integer constant 
expression. See, for example, this patch to Gnulib last September:

https://git.savannah.gnu.org/cgit/gnulib.git/commit/?id=f9e2b20a12a230efa30f1d479563ae07d276a94b

This patch appears in the latest GNU grep, for example.
Stefan Hajnoczi April 22, 2021, 9:59 a.m. UTC | #4
On Mon, Apr 19, 2021 at 11:38:07AM -0700, H.J. Lu wrote:
> On Mon, Apr 19, 2021 at 7:35 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Tue, Apr 13, 2021 at 04:03:19PM +0100, Stefan Hajnoczi wrote:
> > > The size of C arrays at file scope must be constant. The following
> > > compiler error occurs with recent upstream glibc (2.33.9000):
> > >
> > >   CC ucontext-cp
> > >   ucontext-cp.c:31:23: error: variably modified ‘stack_buf’ at file scope
> > >   31 |         unsigned char stack_buf[SIGSTKSZ];
> > >      |                       ^~~~~~~~~
> > >   make[1]: *** [Makefile:26: ucontext-cp] Error 1
> > >
> > > The following glibc commit changed SIGSTKSZ from a constant value to a
> > > variable:
> > >
> > >   commit 6c57d320484988e87e446e2e60ce42816bf51d53
> > >   Author: H.J. Lu <hjl.tools@gmail.com>
> > >   Date:   Mon Feb 1 11:00:38 2021 -0800
> > >
> > >     sysconf: Add _SC_MINSIGSTKSZ/_SC_SIGSTKSZ [BZ #20305]
> > >   ...
> > >   +# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
> > >
> > > Allocate the stack buffer explicitly to avoid declaring an array at file
> > > scope.
> > >
> > > Cc: H.J. Lu <hjl.tools@gmail.com>
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > ---
> > > Perhaps the glibc change needs to be revised before releasing glibc 2.34
> > > since it might break applications. That's up to the glibc folks. It
> > > doesn't hurt for liburing to take a safer approach that copes with the
> > > SIGSTKSZ change in any case.
> >
> > glibc folks, please take a look. The commit referenced above broke
> > compilation of liburing's tests. It's possible that applications will
> > hit similar issues. Can you check whether the SIGSTKSZ change needs to
> > be reverted/fixed before releasing glibc 2.34?
> >
> 
> It won't be changed for glibc 2.34.

Thanks for the response, H.J. and Paul.

In that case liburing needs this patch.

Stefan
Stefano Garzarella April 22, 2021, 2:22 p.m. UTC | #5
+Cc: io-uring@vger.kernel.org
+Cc: Pavel Begunkov <asml.silence@gmail.com>

Original message: 
https://www.spinics.net/lists/linux-block/msg67077.html

On Thu, Apr 22, 2021 at 10:59:42AM +0100, Stefan Hajnoczi wrote:
>On Mon, Apr 19, 2021 at 11:38:07AM -0700, H.J. Lu wrote:
>> On Mon, Apr 19, 2021 at 7:35 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>> >
>> > On Tue, Apr 13, 2021 at 04:03:19PM +0100, Stefan Hajnoczi wrote:
>> > > The size of C arrays at file scope must be constant. The following
>> > > compiler error occurs with recent upstream glibc (2.33.9000):
>> > >
>> > >   CC ucontext-cp
>> > >   ucontext-cp.c:31:23: error: variably modified ‘stack_buf’ at file scope
>> > >   31 |         unsigned char stack_buf[SIGSTKSZ];
>> > >      |                       ^~~~~~~~~
>> > >   make[1]: *** [Makefile:26: ucontext-cp] Error 1
>> > >
>> > > The following glibc commit changed SIGSTKSZ from a constant value to a
>> > > variable:
>> > >
>> > >   commit 6c57d320484988e87e446e2e60ce42816bf51d53
>> > >   Author: H.J. Lu <hjl.tools@gmail.com>
>> > >   Date:   Mon Feb 1 11:00:38 2021 -0800
>> > >
>> > >     sysconf: Add _SC_MINSIGSTKSZ/_SC_SIGSTKSZ [BZ #20305]
>> > >   ...
>> > >   +# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
>> > >
>> > > Allocate the stack buffer explicitly to avoid declaring an array at file
>> > > scope.
>> > >
>> > > Cc: H.J. Lu <hjl.tools@gmail.com>
>> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>> > > ---
>> > > Perhaps the glibc change needs to be revised before releasing glibc 2.34
>> > > since it might break applications. That's up to the glibc folks. It
>> > > doesn't hurt for liburing to take a safer approach that copes with the
>> > > SIGSTKSZ change in any case.
>> >
>> > glibc folks, please take a look. The commit referenced above broke
>> > compilation of liburing's tests. It's possible that applications will
>> > hit similar issues. Can you check whether the SIGSTKSZ change needs to
>> > be reverted/fixed before releasing glibc 2.34?
>> >
>>
>> It won't be changed for glibc 2.34.
>
>Thanks for the response, H.J. and Paul.
>
>In that case liburing needs this patch.
>

I think so:

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Pavel Begunkov April 23, 2021, 2:13 p.m. UTC | #6
On 4/22/21 3:22 PM, Stefano Garzarella wrote:
> +Cc: io-uring@vger.kernel.org
> +Cc: Pavel Begunkov <asml.silence@gmail.com>
> 
> Original message: https://www.spinics.net/lists/linux-block/msg67077.html
> 
> On Thu, Apr 22, 2021 at 10:59:42AM +0100, Stefan Hajnoczi wrote:
>> On Mon, Apr 19, 2021 at 11:38:07AM -0700, H.J. Lu wrote:
>>> On Mon, Apr 19, 2021 at 7:35 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
>>> >
>>> > On Tue, Apr 13, 2021 at 04:03:19PM +0100, Stefan Hajnoczi wrote:
>>> > > The size of C arrays at file scope must be constant. The following
>>> > > compiler error occurs with recent upstream glibc (2.33.9000):
>>> > >
>>> > >   CC ucontext-cp
>>> > >   ucontext-cp.c:31:23: error: variably modified ‘stack_buf’ at file scope
>>> > >   31 |         unsigned char stack_buf[SIGSTKSZ];
>>> > >      |                       ^~~~~~~~~
>>> > >   make[1]: *** [Makefile:26: ucontext-cp] Error 1
>>> > >
>>> > > The following glibc commit changed SIGSTKSZ from a constant value to a
>>> > > variable:
>>> > >
>>> > >   commit 6c57d320484988e87e446e2e60ce42816bf51d53
>>> > >   Author: H.J. Lu <hjl.tools@gmail.com>
>>> > >   Date:   Mon Feb 1 11:00:38 2021 -0800
>>> > >
>>> > >     sysconf: Add _SC_MINSIGSTKSZ/_SC_SIGSTKSZ [BZ #20305]
>>> > >   ...
>>> > >   +# define SIGSTKSZ sysconf (_SC_SIGSTKSZ)
>>> > >
>>> > > Allocate the stack buffer explicitly to avoid declaring an array at file
>>> > > scope.
>>> > >
>>> > > Cc: H.J. Lu <hjl.tools@gmail.com>
>>> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> > > ---
>>> > > Perhaps the glibc change needs to be revised before releasing glibc 2.34
>>> > > since it might break applications. That's up to the glibc folks. It
>>> > > doesn't hurt for liburing to take a safer approach that copes with the
>>> > > SIGSTKSZ change in any case.
>>> >
>>> > glibc folks, please take a look. The commit referenced above broke
>>> > compilation of liburing's tests. It's possible that applications will
>>> > hit similar issues. Can you check whether the SIGSTKSZ change needs to
>>> > be reverted/fixed before releasing glibc 2.34?
>>> >
>>>
>>> It won't be changed for glibc 2.34.
>>
>> Thanks for the response, H.J. and Paul.
>>
>> In that case liburing needs this patch.
>>
> 
> I think so:
> 
> Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Right, and there are already people complaining
https://github.com/axboe/liburing/issues/320
diff mbox series

Patch

diff --git a/examples/ucontext-cp.c b/examples/ucontext-cp.c
index 0b2a6b5..ea0c934 100644
--- a/examples/ucontext-cp.c
+++ b/examples/ucontext-cp.c
@@ -28,7 +28,7 @@ 
 
 typedef struct {
 	struct io_uring *ring;
-	unsigned char stack_buf[SIGSTKSZ];
+	unsigned char *stack_buf;
 	ucontext_t ctx_main, ctx_fnew;
 } async_context;
 
@@ -115,8 +115,13 @@  static int setup_context(async_context *pctx, struct io_uring *ring)
 		perror("getcontext");
 		return -1;
 	}
-	pctx->ctx_fnew.uc_stack.ss_sp = &pctx->stack_buf;
-	pctx->ctx_fnew.uc_stack.ss_size = sizeof(pctx->stack_buf);
+	pctx->stack_buf = malloc(SIGSTKSZ);
+	if (!pctx->stack_buf) {
+		perror("malloc");
+		return -1;
+	}
+	pctx->ctx_fnew.uc_stack.ss_sp = pctx->stack_buf;
+	pctx->ctx_fnew.uc_stack.ss_size = SIGSTKSZ;
 	pctx->ctx_fnew.uc_link = &pctx->ctx_main;
 
 	return 0;
@@ -174,6 +179,7 @@  static void copy_file_wrapper(arguments_bundle *pbundle)
 	free(iov.iov_base);
 	close(pbundle->infd);
 	close(pbundle->outfd);
+	free(pbundle->pctx->stack_buf);
 	free(pbundle->pctx);
 	free(pbundle);