Should we add a compiler warning for large stack frames?

On 2024-04-11 Th 15:01, Andres Freund wrote:

Hi,

d8f5acbdb9b made me wonder if we should add a compiler option to warn when
stack frames are large. gcc compatible compilers have -Wstack-usage=limit, so
that's not hard.

Huge stack frames are somewhat dangerous, as they can defeat our stack-depth
checking logic. There are also some cases where large stack frames defeat
stack-protector logic by compilers/libc/os.

It's not always obvious how large the stack will be. Even if you look at all
the sizes of the variables defined in a function, inlining can increase that
substantially.

Here are all the cases a limit of 64k finds.

[1345/1940 42 69%] Compiling C object src/bin/pg_verifybackup/pg_verifybackup.p/pg_verifybackup.c.o
../../../../../home/andres/src/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c: In function 'verify_file_checksum':
../../../../../home/andres/src/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c:842:1: warning: stack usage is 131232 bytes [-Wstack-usage=]
842 | verify_file_checksum(verifier_context *context, manifest_file *m,
| ^~~~~~~~~~~~~~~~~~~~

This one's down to me. I asked Robert some time back why we were using a
very conservative buffer size, and he agreed we could probably make it
larger, but the number chosen is mine, not his. It was a completely
arbitrary choice.

I'm happy to reduce it, but it's not clear to me why we care that much
for a client binary. There's no stack depth checking going on here.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Hi,

On 2024-04-11 15:16:57 -0400, Andrew Dunstan wrote:

On 2024-04-11 Th 15:01, Andres Freund wrote:

[1345/1940 42 69%] Compiling C object src/bin/pg_verifybackup/pg_verifybackup.p/pg_verifybackup.c.o
../../../../../home/andres/src/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c: In function 'verify_file_checksum':
../../../../../home/andres/src/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c:842:1: warning: stack usage is 131232 bytes [-Wstack-usage=]
842 | verify_file_checksum(verifier_context *context, manifest_file *m,
| ^~~~~~~~~~~~~~~~~~~~

This one's down to me. I asked Robert some time back why we were using a
very conservative buffer size, and he agreed we could probably make it
larger, but the number chosen is mine, not his. It was a completely
arbitrary choice.

I'm happy to reduce it, but it's not clear to me why we care that much for a
client binary. There's no stack depth checking going on here.

There isn't - but it that doesn't necessarily mean it's great to just use a
lot of stack. It can even mean that you ought to be more careful, because
you'll just crash, rather than get an error about having exceeded the stack
size. When the stack size is increased by more than a few pages, the OS /
compiler defenses for detecting that don't work as well, if you're unlucky.

128k is probably not going to be an issue in practice. However, it also seems
not great from a performance POV to use this much stack in a function that's
called fairly often. I'd allocate the buffer in verify_backup_checksums() and
reuse it across all to-be-checked files.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

d8f5acbdb9b made me wonder if we should add a compiler option to warn when
stack frames are large. gcc compatible compilers have -Wstack-usage=limit, so
that's not hard.

Huge stack frames are somewhat dangerous, as they can defeat our stack-depth
checking logic. There are also some cases where large stack frames defeat
stack-protector logic by compilers/libc/os.

Indeed. I recall reading, not long ago, some Linux kernel docs to the
effect that automatic stack growth is triggered by a reference into
the page just below what is currently mapped as your stack, and
therefore allocating a stack frame greater than one page has the
potential to cause SIGSEGV rather than the desired stack extension.
(If you feel like digging in the archives, I think this was brought
up in the last round of lets-add-some-more-check_stack_depth-calls.)

Warnings in src/bin aren't as interesting as warnings in backend code, as the
latter is much more likely to be "exposed" to deep stacks and could be
vulnerable due to stack overflows.

Probably, but it's still the case that such code is relying on the
original stack allocation being large enough already.

I don't really have an opinion about the concrete warning limit to use.

Given the above, I'm tempted to say we should make it 8K. But I know
we have a bunch of places that allocate page images as stack space,
so maybe that'd be too painful.

regards, tom lane

Hi,

On 2024-04-11 16:35:58 -0400, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

d8f5acbdb9b made me wonder if we should add a compiler option to warn when
stack frames are large. gcc compatible compilers have -Wstack-usage=limit, so
that's not hard.

Huge stack frames are somewhat dangerous, as they can defeat our stack-depth
checking logic. There are also some cases where large stack frames defeat
stack-protector logic by compilers/libc/os.

Indeed. I recall reading, not long ago, some Linux kernel docs to the
effect that automatic stack growth is triggered by a reference into
the page just below what is currently mapped as your stack, and
therefore allocating a stack frame greater than one page has the
potential to cause SIGSEGV rather than the desired stack extension.
(If you feel like digging in the archives, I think this was brought
up in the last round of lets-add-some-more-check_stack_depth-calls.)

I think it's more than a single page, but I'm not entirely sure either. I
think some compilers inject artificial stack accesses when extending the stack
by a lot, but I don't remember the details.

There certainly was the issue that strict memory overcommit does not reliably
work with larger stack extensions.

Could be worth writing a test program for...

I don't really have an opinion about the concrete warning limit to use.

Given the above, I'm tempted to say we should make it 8K.

Hm, why 8k? That's 2x the typical page size, isn't it?

But I know we have a bunch of places that allocate page images as stack
space, so maybe that'd be too painful.

8k does generate a fair number of warnings, 111 here. I think it might also
be hard to ensure that inlining doesn't end up creating bigger stack frames.

frame size warnings
4096 155
8192 111
16384 36
32768 14
65536 8

Suggests that starting somewhere around 16-32k might be reasonable?

One issue is of course that configuring a larger blocksize or wal_blocksize
will trigger more warnings. I guess we'd need to set the limit based on
Max(blocksize, wal_blocksize) * 2 or such.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2024-04-11 16:35:58 -0400, Tom Lane wrote:

Indeed. I recall reading, not long ago, some Linux kernel docs to the
effect that automatic stack growth is triggered by a reference into
the page just below what is currently mapped as your stack, and
therefore allocating a stack frame greater than one page has the
potential to cause SIGSEGV rather than the desired stack extension.

I think it's more than a single page, but I'm not entirely sure either. I
think some compilers inject artificial stack accesses when extending the stack
by a lot, but I don't remember the details.

Hmm. You're right that I was misremembering the typical RAM page
size. The kernel must be allowing stack frames bigger than 4K,
or we'd see problems everywhere. I wonder how much bigger ...

frame size warnings
4096 155
8192 111
16384 36
32768 14
65536 8

Suggests that starting somewhere around 16-32k might be reasonable?

I'm hesitant to touch more than a handful of places on the strength
of the info we've got; either it's wasted work or it's not enough
work, and we don't know which. Might be time for some
experimentation.

regards, tom lane

Hi,

On 2024-04-11 15:07:11 -0700, Andres Freund wrote:

On 2024-04-11 16:35:58 -0400, Tom Lane wrote:

Indeed. I recall reading, not long ago, some Linux kernel docs to the
effect that automatic stack growth is triggered by a reference into
the page just below what is currently mapped as your stack, and
therefore allocating a stack frame greater than one page has the
potential to cause SIGSEGV rather than the desired stack extension.
(If you feel like digging in the archives, I think this was brought
up in the last round of lets-add-some-more-check_stack_depth-calls.)

I think it's more than a single page, but I'm not entirely sure either. I
think some compilers inject artificial stack accesses when extending the stack
by a lot, but I don't remember the details.

There certainly was the issue that strict memory overcommit does not reliably
work with larger stack extensions.

Could be worth writing a test program for...

It looks like it's a mess.

In the good cases the kernel doesn't map anything within ulimit -R of the
stack, and the stack is extended whenever memory in that range is accessed.
Nothing is mapped into that region unless MAP_FIXED is used.

However, in some cases linux maps the heap and the stack fairly close to each
other at program startup. I've observed this with an executable compiled with
-static-pie and executed with randomization disabled (via setarch -R). In
that case the the layout at program start is

...
7ffff7fff000-7ffff8021000 rw-p 00000000 00:00 0 [heap]
7ffffffdd000-7ffffffff000 rw-p 00000000 00:00 0 [stack]

Here the start of the heap and the end of the stack are only 128MB appart. The
heap grows upwards, the stack downwards.

Which means that if glibc allocates a bunch of memory via sbrk() and the stack
grows, they clash into each other.

I think this may be a glibc bug. If I compile with musl instead, this doesn't
happen, because musl stops using sbrk() style allocations before stack and
program break get too close to each other.

Greetings,

Andres Freund

On 2024-04-11 Th 16:17, Andres Freund wrote:

128k is probably not going to be an issue in practice. However, it also seems
not great from a performance POV to use this much stack in a function that's
called fairly often. I'd allocate the buffer in verify_backup_checksums() and
reuse it across all to-be-checked files.

Yes, I agree. I'll make that happen in the next day or two.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com