Horton

Thread

Commits

  1. Don't allocate large buffer on the stack in pg_verifybackup

  1. Should we add a compiler warning for large stack frames?

    Andres Freund <andres@anarazel.de> — 2024-04-11T19:01:47Z

    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.

Unoptimized build:
[138/1940 42   7%] Compiling C object src/common/libpgcommon_shlib.a.p/blkreftable.c.o
../../../../../home/andres/src/postgresql/src/common/blkreftable.c: In function 'WriteBlockRefTable':
../../../../../home/andres/src/postgresql/src/common/blkreftable.c:474:1: warning: stack usage is 65696 bytes [-Wstack-usage=]
  474 | WriteBlockRefTable(BlockRefTable *brtab,
      | ^~~~~~~~~~~~~~~~~~
[173/1940 42   8%] Compiling C object src/common/libpgcommon.a.p/blkreftable.c.o
../../../../../home/andres/src/postgresql/src/common/blkreftable.c: In function 'WriteBlockRefTable':
../../../../../home/andres/src/postgresql/src/common/blkreftable.c:474:1: warning: stack usage is 65696 bytes [-Wstack-usage=]
  474 | WriteBlockRefTable(BlockRefTable *brtab,
      | ^~~~~~~~~~~~~~~~~~
[281/1940 42  14%] Compiling C object src/common/libpgcommon_srv.a.p/blkreftable.c.o
../../../../../home/andres/src/postgresql/src/common/blkreftable.c: In function 'WriteBlockRefTable':
../../../../../home/andres/src/postgresql/src/common/blkreftable.c:474:1: warning: stack usage is 65696 bytes [-Wstack-usage=]
  474 | WriteBlockRefTable(BlockRefTable *brtab,
      | ^~~~~~~~~~~~~~~~~~
[1311/1940 42  67%] Compiling C object src/bin/pg_basebackup/pg_basebackup.p/pg_basebackup.c.o
../../../../../home/andres/src/postgresql/src/bin/pg_basebackup/pg_basebackup.c: In function 'BaseBackup':
../../../../../home/andres/src/postgresql/src/bin/pg_basebackup/pg_basebackup.c:1753:1: warning: stack usage might be 66976 bytes [-Wstack-usage=]
 1753 | BaseBackup(char *compression_algorithm, char *compression_detail,
      | ^~~~~~~~~~
[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,
      | ^~~~~~~~~~~~~~~~~~~~
[1349/1940 42  69%] Compiling C object src/bin/pg_waldump/pg_waldump.p/pg_waldump.c.o
../../../../../home/andres/src/postgresql/src/bin/pg_waldump/pg_waldump.c: In function 'main':
../../../../../home/andres/src/postgresql/src/bin/pg_waldump/pg_waldump.c:792:1: warning: stack usage might be 105104 bytes [-Wstack-usage=]
  792 | main(int argc, char **argv)
      | ^~~~
[1563/1940 42  80%] Compiling C object contrib/pg_walinspect/pg_walinspect.so.p/pg_walinspect.c.o
../../../../../home/andres/src/postgresql/contrib/pg_walinspect/pg_walinspect.c: In function 'GetWalStats':
../../../../../home/andres/src/postgresql/contrib/pg_walinspect/pg_walinspect.c:762:1: warning: stack usage is 104624 bytes [-Wstack-usage=]
  762 | GetWalStats(FunctionCallInfo fcinfo, XLogRecPtr start_lsn, XLogRecPtr end_lsn,
      | ^~~~~~~~~~~
[1581/1940 42  81%] Compiling C object src/test/modules/test_dsa/test_dsa.so.p/test_dsa.c.o
../../../../../home/andres/src/postgresql/src/test/modules/test_dsa/test_dsa.c: In function 'test_dsa_resowners':
../../../../../home/andres/src/postgresql/src/test/modules/test_dsa/test_dsa.c:64:1: warning: stack usage is 80080 bytes [-Wstack-usage=]
   64 | test_dsa_resowners(PG_FUNCTION_ARGS)
      | ^~~~~~~~~~~~~~~~~~


There is one warning that is just visible in an optimized build,
otherwise the precise amount of stack usage just differs some:
[1165/2017 42  57%] Compiling C object src/backend/postgres_lib.a.p/tsearch_spell.c.o
../../../../../home/andres/src/postgresql/src/backend/tsearch/spell.c: In function 'NIImportAffixes':
../../../../../home/andres/src/postgresql/src/backend/tsearch/spell.c:1425:1: warning: stack usage might be 74080 bytes [-Wstack-usage=]
 1425 | NIImportAffixes(IspellDict *Conf, const char *filename)
      | ^~~~~~~~~~~~~~~


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.

I did verify this would have warned about d8f5acbdb9b^:

[1/2 1  50%] Compiling C object src/backend/postgres_lib.a.p/backup_basebackup_incremental.c.o
../../../../../home/andres/src/postgresql/src/backend/backup/basebackup_incremental.c: In function 'GetFileBackupMethod':
../../../../../home/andres/src/postgresql/src/backend/backup/basebackup_incremental.c:742:1: warning: stack usage is 524400 bytes [-Wstack-usage=]
  742 | GetFileBackupMethod(IncrementalBackupInfo *ib, const char *path,
      | ^~~~~~~~~~~~~~~~~~~


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

Greetings,

Andres Freund

  2. Re: Should we add a compiler warning for large stack frames?

    Andrew Dunstan <andrew@dunslane.net> — 2024-04-11T19:16:57Z

    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

  3. Re: Should we add a compiler warning for large stack frames?

    Andres Freund <andres@anarazel.de> — 2024-04-11T20:17:13Z

    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

  4. Re: Should we add a compiler warning for large stack frames?

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-11T20:35:58Z

    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

  5. Re: Should we add a compiler warning for large stack frames?

    Andres Freund <andres@anarazel.de> — 2024-04-11T22:07:11Z

    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

  6. Re: Should we add a compiler warning for large stack frames?

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-12T00:56:54Z

    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

  7. Re: Should we add a compiler warning for large stack frames?

    Andres Freund <andres@anarazel.de> — 2024-04-12T02:15:22Z

    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

  8. Re: Should we add a compiler warning for large stack frames?

    Andrew Dunstan <andrew@dunslane.net> — 2024-04-12T12:29:29Z

    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