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
