public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v28 09/11] pg_ls_*/pg_stat_file to show file *type*.. 16+ messages / 8 participants [nested] [flat]
* [PATCH v28 09/11] pg_ls_*/pg_stat_file to show file *type*.. @ 2020-03-31 19:40 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 16+ messages in thread From: Justin Pryzby @ 2020-03-31 19:40 UTC (permalink / raw) ..not just "isdir" Also show special file types, now that their type is shown. --- doc/src/sgml/func.sgml | 26 +++++---- src/backend/utils/adt/genfile.c | 58 ++++++++++++++++---- src/include/catalog/pg_proc.dat | 36 ++++++------ src/test/regress/expected/misc_functions.out | 30 +++++----- src/test/regress/output/tablespace.source | 8 +-- src/test/regress/sql/misc_functions.sql | 4 +- 6 files changed, 101 insertions(+), 61 deletions(-) diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 402c10e30b..f81323771d 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -25836,7 +25836,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); <parameter>modification</parameter> <type>timestamp with time zone</type>, <parameter>change</parameter> <type>timestamp with time zone</type>, <parameter>creation</parameter> <type>timestamp with time zone</type>, - <parameter>isdir</parameter> <type>boolean</type> ) + <parameter>type</parameter> <type>char</type> ) </para> <para> For each file in the specified directory, list the file and its @@ -26886,13 +26886,13 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size <parameter>modification</parameter> <type>timestamp with time zone</type>, <parameter>change</parameter> <type>timestamp with time zone</type>, <parameter>creation</parameter> <type>timestamp with time zone</type>, - <parameter>isdir</parameter> <type>boolean</type> ) + <parameter>type</parameter> <type>char</type> ) </para> <para> For each file in the server's log directory, return the file's name, along with the metadata columns returned by <function>pg_stat_file</function>. - Filenames beginning with a dot and special file types are excluded. + Filenames beginning with a dot are excluded. </para> <para> This function is restricted to superusers and members of @@ -26914,13 +26914,13 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size <parameter>modification</parameter> <type>timestamp with time zone</type>, <parameter>change</parameter> <type>timestamp with time zone</type>, <parameter>creation</parameter> <type>timestamp with time zone</type>, - <parameter>isdir</parameter> <type>boolean</type> ) + <parameter>type</parameter> <type>char</type> ) </para> <para> For each file in the server's write-ahead log (WAL) directory, list the file's name along with the metadata columns returned by <function>pg_stat_file</function>. - Filenames beginning with a dot and special files types are excluded. + Filenames beginning with a dot are excluded. </para> <para> This function is restricted to superusers and members of @@ -26942,14 +26942,14 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size <parameter>modification</parameter> <type>timestamp with time zone</type>, <parameter>change</parameter> <type>timestamp with time zone</type>, <parameter>creation</parameter> <type>timestamp with time zone</type>, - <parameter>isdir</parameter> <type>boolean</type> ) + <parameter>type</parameter> <type>char</type> ) </para> <para> For each file in the server's WAL archive status directory (<filename>pg_wal/archive_status</filename>), list the file's name along with the metadata columns returned by <function>pg_stat_file</function>. - Filenames beginning with a dot and special file types are excluded. + Filenames beginning with a dot are excluded. </para> <para> This function is restricted to superusers and members of @@ -26972,7 +26972,7 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size <parameter>modification</parameter> <type>timestamp with time zone</type>, <parameter>change</parameter> <type>timestamp with time zone</type>, <parameter>creation</parameter> <type>timestamp with time zone</type>, - <parameter>isdir</parameter> <type>boolean</type> ) + <parameter>type</parameter> <type>char</type> ) </para> <para> For each file in the temporary directory within the given @@ -26982,7 +26982,7 @@ SELECT pg_size_pretty(sum(pg_relation_size(relid))) AS total_size Directories are used for temporary files shared by parallel processes. If <parameter>tablespace</parameter> is not provided, the <literal>pg_default</literal> tablespace is examined. - Filenames beginning with a dot and special file types are excluded. + Filenames beginning with a dot are excluded. </para> <para> This function is restricted to superusers and members of @@ -27056,13 +27056,15 @@ SELECT convert_from(pg_read_binary_file('file_in_utf8.txt'), 'UTF8'); <parameter>modification</parameter> <type>timestamp with time zone</type>, <parameter>change</parameter> <type>timestamp with time zone</type>, <parameter>creation</parameter> <type>timestamp with time zone</type>, - <parameter>isdir</parameter> <type>boolean</type> ) + <parameter>type</parameter> <type>char</type> ) </para> <para> Returns a record containing the file's size, last access time stamp, last modification time stamp, last file status change time stamp (Unix - platforms only), file creation time stamp (Windows only), and a flag - indicating if it is a directory. + platforms only), file creation time stamp (Windows only), and a + character representing the file's type: regular (-), directory (d), link + or junction (l), character device (c), block device (b), fifo (p), + socket (s) or other/unknown (?). </para> <para> This function is restricted to superusers by default, but other users diff --git a/src/backend/utils/adt/genfile.c b/src/backend/utils/adt/genfile.c index 17e05b853e..af666d658b 100644 --- a/src/backend/utils/adt/genfile.c +++ b/src/backend/utils/adt/genfile.c @@ -36,11 +36,12 @@ #include "utils/syscache.h" #include "utils/timestamp.h" +static char get_file_type(mode_t mode, const char *path); static void values_from_stat(struct stat *fst, const char *path, Datum *values, bool *nulls); static Datum pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags); -#define LS_DIR_ISDIR (1<<0) /* Show column: isdir */ +#define LS_DIR_TYPE (1<<0) /* Show column: type */ #define LS_DIR_METADATA (1<<1) /* Show columns: mtime, size */ #define LS_DIR_MISSING_OK (1<<2) /* Ignore ENOENT if the toplevel dir is missing */ #define LS_DIR_SKIP_DOT_DIRS (1<<3) /* Do not show . or .. */ @@ -49,7 +50,7 @@ static Datum pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags #define LS_DIR_SKIP_SPECIAL (1<<6) /* Do not show special file types */ /* Shortcut for common behavior */ -#define LS_DIR_COMMON (LS_DIR_SKIP_HIDDEN | LS_DIR_SKIP_SPECIAL | LS_DIR_METADATA) +#define LS_DIR_COMMON (LS_DIR_SKIP_HIDDEN | LS_DIR_METADATA) /* * Convert a "text" filename argument to C string, and check it's allowable. @@ -402,6 +403,43 @@ pg_read_binary_file_all(PG_FUNCTION_ARGS) return pg_read_binary_file(fcinfo); } +/* Return a character indicating the type of file, or '?' if unknown type */ +static char +get_file_type(mode_t mode, const char *path) +{ + if (S_ISREG(mode)) + return '-'; + + if (S_ISDIR(mode)) + return 'd'; +#ifndef WIN32 + if (S_ISLNK(mode)) + return 'l'; +#else + if (pgwin32_is_junction(path)) + return 'l'; +#endif + +#ifdef S_ISCHR + if (S_ISCHR(mode)) + return 'c'; +#endif +#ifdef S_ISBLK + if (S_ISBLK(mode)) + return 'b'; +#endif +#ifdef S_ISFIFO + if (S_ISFIFO(mode)) + return 'p'; +#endif +#ifdef S_ISSOCK + if (S_ISSOCK(mode)) + return 's'; +#endif + + return '?'; +} + /* * Populate values and nulls from fst and path. * Used for pg_stat_file() and pg_ls_dir_files() @@ -421,7 +459,7 @@ values_from_stat(struct stat *fst, const char *path, Datum *values, bool *nulls) nulls[3] = true; values[4] = TimestampTzGetDatum(time_t_to_timestamptz(fst->st_ctime)); #endif - values[5] = BoolGetDatum(S_ISDIR(fst->st_mode)); + values[5] = CharGetDatum(get_file_type(fst->st_mode, path)); } /* @@ -471,7 +509,7 @@ pg_stat_file(PG_FUNCTION_ARGS) TupleDescInitEntry(tupdesc, (AttrNumber) 5, "creation", TIMESTAMPTZOID, -1, 0); TupleDescInitEntry(tupdesc, (AttrNumber) 6, - "isdir", BOOLOID, -1, 0); + "type", CHAROID, -1, 0); BlessTupleDesc(tupdesc); memset(nulls, false, sizeof(nulls)); @@ -540,10 +578,10 @@ pg_ls_dir_files(FunctionCallInfo fcinfo, const char *dir, int flags) MemoryContext oldcontext; TypeFuncClass tuptype ; - /* isdir depends on metadata */ - Assert(!(flags&LS_DIR_ISDIR) || (flags&LS_DIR_METADATA)); - /* Unreasonable to show isdir and skip dirs */ - Assert(!(flags&LS_DIR_ISDIR) || !(flags&LS_DIR_SKIP_DIRS)); + /* type depends on metadata */ + Assert(!(flags&LS_DIR_TYPE) || (flags&LS_DIR_METADATA)); + /* Unreasonable to show type and skip dirs XXX */ + Assert(!(flags&LS_DIR_TYPE) || !(flags&LS_DIR_SKIP_DIRS)); /* check the optional arguments */ if (PG_NARGS() == 3) @@ -741,7 +779,7 @@ pg_ls_dir_metadata(PG_FUNCTION_ARGS) char *dirname = convert_and_check_filename(PG_GETARG_TEXT_PP(0)); return pg_ls_dir_files(fcinfo, dirname, - LS_DIR_METADATA | LS_DIR_SKIP_SPECIAL | LS_DIR_ISDIR); + LS_DIR_METADATA | LS_DIR_TYPE); } /* @@ -756,5 +794,5 @@ pg_ls_dir_metadata_1arg(PG_FUNCTION_ARGS) char *dirname = convert_and_check_filename(PG_GETARG_TEXT_PP(0)); return pg_ls_dir_files(fcinfo, dirname, - LS_DIR_METADATA | LS_DIR_SKIP_SPECIAL | LS_DIR_ISDIR); + LS_DIR_METADATA | LS_DIR_TYPE); } diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 08eea899d3..2fc1793b5a 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -6319,16 +6319,16 @@ { oid => '2623', descr => 'get information about file', proname => 'pg_stat_file', provolatile => 'v', prorettype => 'record', proargtypes => 'text', - proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', + proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{i,o,o,o,o,o,o}', - proargnames => '{filename,size,access,modification,change,creation,isdir}', + proargnames => '{filename,size,access,modification,change,creation,type}', prosrc => 'pg_stat_file_1arg' }, { oid => '3307', descr => 'get information about file', proname => 'pg_stat_file', provolatile => 'v', prorettype => 'record', proargtypes => 'text bool', - proallargtypes => '{text,bool,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', + proallargtypes => '{text,bool,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{i,i,o,o,o,o,o,o}', - proargnames => '{filename,missing_ok,size,access,modification,change,creation,isdir}', + proargnames => '{filename,missing_ok,size,access,modification,change,creation,type}', prosrc => 'pg_stat_file' }, { oid => '2624', descr => 'read text from a file', proname => 'pg_read_file', provolatile => 'v', prorettype => 'text', @@ -11510,41 +11510,41 @@ { oid => '3353', descr => 'list files in the log directory', proname => 'pg_ls_logdir', procost => '10', prorows => '20', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => '', - proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}', - proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_logdir' }, + proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{o,o,o,o,o,o,o}', + proargnames => '{name,size,access,modification,change,creation,type}', prosrc => 'pg_ls_logdir' }, { oid => '3354', descr => 'list of files in the WAL directory', proname => 'pg_ls_waldir', procost => '10', prorows => '20', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => '', - proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}', - proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_waldir' }, + proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{o,o,o,o,o,o,o}', + proargnames => '{name,size,access,modification,change,creation,type}', prosrc => 'pg_ls_waldir' }, { oid => '5031', descr => 'list of files in the archive_status directory', proname => 'pg_ls_archive_statusdir', procost => '10', prorows => '20', proretset => 't', provolatile => 'v', prorettype => 'record', - proargtypes => '', proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', - proargmodes => '{o,o,o,o,o,o,o}', proargnames => '{name,size,access,modification,change,creation,isdir}', + proargtypes => '', proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', + proargmodes => '{o,o,o,o,o,o,o}', proargnames => '{name,size,access,modification,change,creation,type}', prosrc => 'pg_ls_archive_statusdir' }, { oid => '5029', descr => 'list files in the pgsql_tmp directory', proname => 'pg_ls_tmpdir', procost => '10', prorows => '20', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => '', - proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{o,o,o,o,o,o,o}', - proargnames => '{name,size,access,modification,change,creation,isdir}', prosrc => 'pg_ls_tmpdir_noargs' }, + proallargtypes => '{text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{o,o,o,o,o,o,o}', + proargnames => '{name,size,access,modification,change,creation,type}', prosrc => 'pg_ls_tmpdir_noargs' }, { oid => '5030', descr => 'list files in the pgsql_tmp directory', proname => 'pg_ls_tmpdir', procost => '10', prorows => '20', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => 'oid', - proallargtypes => '{oid,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,o,o,o,o,o,o,o}', - proargnames => '{tablespace,name,size,access,modification,change,creation,isdir}', + proallargtypes => '{oid,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{i,o,o,o,o,o,o,o}', + proargnames => '{tablespace,name,size,access,modification,change,creation,type}', prosrc => 'pg_ls_tmpdir_1arg' }, { oid => '8450', descr => 'list directory with metadata', proname => 'pg_ls_dir_metadata', procost => '10', prorows => '20', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => 'text bool bool', - proallargtypes => '{text,bool,bool,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,i,i,o,o,o,o,o,o,o}', - proargnames => '{dirname,missing_ok,include_dot_dirs,filename,size,access,modification,change,creation,isdir}', + proallargtypes => '{text,bool,bool,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{i,i,i,o,o,o,o,o,o,o}', + proargnames => '{dirname,missing_ok,include_dot_dirs,filename,size,access,modification,change,creation,type}', prosrc => 'pg_ls_dir_metadata' }, { oid => '8451', descr => 'list directory with metadata', proname => 'pg_ls_dir_metadata', procost => '10', prorows => '20', proretset => 't', provolatile => 'v', prorettype => 'record', proargtypes => 'text', - proallargtypes => '{text,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,bool}', proargmodes => '{i,o,o,o,o,o,o,o}', - proargnames => '{dirname,filename,size,access,modification,change,creation,isdir}', + proallargtypes => '{text,text,int8,timestamptz,timestamptz,timestamptz,timestamptz,char}', proargmodes => '{i,o,o,o,o,o,o,o}', + proargnames => '{dirname,filename,size,access,modification,change,creation,type}', prosrc => 'pg_ls_dir_metadata_1arg' }, # hash partitioning constraint function diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out index ddd35b79f5..db3d64de3b 100644 --- a/src/test/regress/expected/misc_functions.out +++ b/src/test/regress/expected/misc_functions.out @@ -170,8 +170,8 @@ select count(*) > 0 as ok from (select pg_ls_waldir()) ss; -- Test not-run-to-completion cases. select * from pg_ls_waldir() limit 0; - name | size | access | modification | change | creation | isdir -------+------+--------+--------------+--------+----------+------- + name | size | access | modification | change | creation | type +------+------+--------+--------------+--------+----------+------ (0 rows) select count(*) > 0 as ok from (select * from pg_ls_waldir() limit 1) ss; @@ -234,32 +234,32 @@ select pg_ls_dir('does not exist'); -- fails with missingok=false ERROR: could not open directory "does not exist": No such file or directory -- Check that expected columns are present select * from pg_stat_file('.') limit 0; - size | access | modification | change | creation | isdir -------+--------+--------------+--------+----------+------- + size | access | modification | change | creation | type +------+--------+--------------+--------+----------+------ (0 rows) -- This tests the missing_ok parameter, which causes pg_ls_tmpdir to succeed even if the tmpdir doesn't exist yet -- The name='' condition is never true, so the function runs to completion but returns zero rows. select * from pg_ls_tmpdir() where name='Does not exist'; - name | size | access | modification | change | creation | isdir -------+------+--------+--------------+--------+----------+------- + name | size | access | modification | change | creation | type +------+------+--------+--------------+--------+----------+------ (0 rows) -select filename, isdir from pg_ls_dir_metadata('.') where filename='.'; - filename | isdir -----------+------- - . | t +select filename, type from pg_ls_dir_metadata('.') where filename='.'; + filename | type +----------+------ + . | d (1 row) -select filename, isdir from pg_ls_dir_metadata('.', false, false) where filename='.'; -- include_dot_dirs=false - filename | isdir -----------+------- +select filename, type from pg_ls_dir_metadata('.', false, false) where filename='.'; -- include_dot_dirs=false + filename | type +----------+------ (0 rows) -- Check that expected columns are present select * from pg_ls_dir_metadata('.') limit 0; - filename | size | access | modification | change | creation | isdir -----------+------+--------+--------------+--------+----------+------- + filename | size | access | modification | change | creation | type +----------+------+--------+--------------+--------+----------+------ (0 rows) -- diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source index 34f874d3f1..21f3e68392 100644 --- a/src/test/regress/output/tablespace.source +++ b/src/test/regress/output/tablespace.source @@ -17,15 +17,15 @@ CREATE TABLESPACE regress_tblspace LOCATION '@testtablespace@'; -- The name='' condition is never true, so the function runs to completion but returns zero rows. -- The query is written to ERROR if the tablespace doesn't exist, rather than silently failing to call pg_ls_tmpdir() SELECT c.* FROM (SELECT oid FROM pg_tablespace b WHERE b.spcname='regress_tblspace' UNION SELECT 0 ORDER BY 1 DESC LIMIT 1) AS b , pg_ls_tmpdir(oid) AS c WHERE c.name='Does not exist'; - name | size | access | modification | change | creation | isdir -------+------+--------+--------------+--------+----------+------- + name | size | access | modification | change | creation | type +------+------+--------+--------------+--------+----------+------ (0 rows) -- This tests the missing_ok parameter. If that's not functioning, this would ERROR if the logdir doesn't exist yet. -- The name='' condition is never true, so the function runs to completion but returns zero rows. SELECT * FROM pg_ls_logdir() WHERE name='Does not exist'; - name | size | access | modification | change | creation | isdir -------+------+--------+--------------+--------+----------+------- + name | size | access | modification | change | creation | type +------+------+--------+--------------+--------+----------+------ (0 rows) -- try setting and resetting some properties for the new tablespace diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql index c169e527d9..55bed7b9e0 100644 --- a/src/test/regress/sql/misc_functions.sql +++ b/src/test/regress/sql/misc_functions.sql @@ -81,9 +81,9 @@ select * from pg_stat_file('.') limit 0; -- The name='' condition is never true, so the function runs to completion but returns zero rows. select * from pg_ls_tmpdir() where name='Does not exist'; -select filename, isdir from pg_ls_dir_metadata('.') where filename='.'; +select filename, type from pg_ls_dir_metadata('.') where filename='.'; -select filename, isdir from pg_ls_dir_metadata('.', false, false) where filename='.'; -- include_dot_dirs=false +select filename, type from pg_ls_dir_metadata('.', false, false) where filename='.'; -- include_dot_dirs=false -- Check that expected columns are present select * from pg_ls_dir_metadata('.') limit 0; -- 2.17.0 --vk/v8fjDPiDepTtA Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v28-0010-Preserve-pg_stat_file-isdir.patch" ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-07 09:09 Mats Kindahl <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Mats Kindahl @ 2024-02-07 09:09 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; [email protected] On Tue, Feb 6, 2024 at 9:56 PM Tom Lane <[email protected]> wrote: > Nathan Bossart <[email protected]> writes: > > Even if the glibc issue doesn't apply to Postgres, I'm tempted to suggest > > that we make it project policy that comparison functions must be > > transitive. There might be no real issues today, but if we write all > > comparison functions the way Mats is suggesting, it should be easier to > > reason about overflow risks. > > A comparison routine that is not is probably broken, agreed. > I didn't look through the details of the patch --- I was more > curious whether we had a version of the qsort bug, because > if we do, we should fix that too. > The patch basically removes the risk of overflow in three routines and just returns -1, 0, or 1, and adds a comment in one. The routines modified do a subtraction of int:s and return that, which can cause an overflow. This method is used for some int16 as well but since standard conversions in C will perform the arithmetics in "int" precision, this cannot overflow, so added a comment there. It might still be a good idea to follow the same pattern for the int16 routines, but since there is no bug there, I did not add them to the patch. Best wishes, Mats Kindahl > > regards, tom lane > ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-07 18:46 Heikki Linnakangas <[email protected]> parent: Mats Kindahl <[email protected]> 0 siblings, 2 replies; 16+ messages in thread From: Heikki Linnakangas @ 2024-02-07 18:46 UTC (permalink / raw) To: Mats Kindahl <[email protected]>; Tom Lane <[email protected]>; +Cc: Nathan Bossart <[email protected]>; [email protected] On 07/02/2024 11:09, Mats Kindahl wrote: > On Tue, Feb 6, 2024 at 9:56 PM Tom Lane <[email protected] > <mailto:[email protected]>> wrote: > > Nathan Bossart <[email protected] > <mailto:[email protected]>> writes: > > Even if the glibc issue doesn't apply to Postgres, I'm tempted to > suggest > > that we make it project policy that comparison functions must be > > transitive. There might be no real issues today, but if we write all > > comparison functions the way Mats is suggesting, it should be > easier to > > reason about overflow risks. > > A comparison routine that is not is probably broken, agreed. > I didn't look through the details of the patch --- I was more > curious whether we had a version of the qsort bug, because > if we do, we should fix that too. > > The patch basically removes the risk of overflow in three routines and > just returns -1, 0, or 1, and adds a comment in one. > > The routines modified do a subtraction of int:s and return that, which > can cause an overflow. This method is used for some int16 as well but > since standard conversions in C will perform the arithmetics in "int" > precision, this cannot overflow, so added a comment there. It might > still be a good idea to follow the same pattern for the int16 routines, > but since there is no bug there, I did not add them to the patch. Doesn't hurt to fix the comparison functions, and +1 on using the same pattern everywhere. However, we use our qsort() with user-defined comparison functions, and we cannot make any guarantees about what they might do. So we must ensure that our qsort() doesn't overflow, no matter what the comparison function does. Looking at our ST_SORT(), it seems safe to me. -- Heikki Linnakangas Neon (https://neon.tech) ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-07 20:56 Nathan Bossart <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Nathan Bossart @ 2024-02-07 20:56 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Mats Kindahl <[email protected]>; Tom Lane <[email protected]>; [email protected] On Wed, Feb 07, 2024 at 08:46:56PM +0200, Heikki Linnakangas wrote: > Doesn't hurt to fix the comparison functions, and +1 on using the same > pattern everywhere. I attached a new version of the patch with some small adjustments. I haven't looked through all in-tree qsort() comparators to see if any others need to be adjusted, but we should definitely do so as part of this thread. Mats, are you able to do this? > However, we use our qsort() with user-defined comparison functions, and we > cannot make any guarantees about what they might do. So we must ensure that > our qsort() doesn't overflow, no matter what the comparison function does. > > Looking at our ST_SORT(), it seems safe to me. Cool. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com Attachments: [text/x-diff] v2-0001-remove-direct-calls-to-pg_qsort.patch (3.6K, ../../20240207205600.GA378707@nathanxps13/2-v2-0001-remove-direct-calls-to-pg_qsort.patch) download | inline diff: From f5850579e92f201218c3025327b91d820eabd18e Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 7 Feb 2024 14:29:04 -0600 Subject: [PATCH v2 1/2] remove direct calls to pg_qsort() --- contrib/pg_prewarm/autoprewarm.c | 4 ++-- src/backend/access/brin/brin_minmax_multi.c | 2 +- src/backend/storage/buffer/bufmgr.c | 4 ++-- src/backend/utils/cache/syscache.c | 10 +++++----- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c index 06ee21d496..248b9914a3 100644 --- a/contrib/pg_prewarm/autoprewarm.c +++ b/contrib/pg_prewarm/autoprewarm.c @@ -346,8 +346,8 @@ apw_load_buffers(void) FreeFile(file); /* Sort the blocks to be loaded. */ - pg_qsort(blkinfo, num_elements, sizeof(BlockInfoRecord), - apw_compare_blockinfo); + qsort(blkinfo, num_elements, sizeof(BlockInfoRecord), + apw_compare_blockinfo); /* Populate shared memory state. */ apw_state->block_info_handle = dsm_segment_handle(seg); diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c index 3ffaad3e42..2c29aa3d4e 100644 --- a/src/backend/access/brin/brin_minmax_multi.c +++ b/src/backend/access/brin/brin_minmax_multi.c @@ -1369,7 +1369,7 @@ build_distances(FmgrInfo *distanceFn, Oid colloid, * Sort the distances in descending order, so that the longest gaps are at * the front. */ - pg_qsort(distances, ndistances, sizeof(DistanceValue), compare_distances); + qsort(distances, ndistances, sizeof(DistanceValue), compare_distances); return distances; } diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index eb1ec3b86d..07575ef312 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -3915,7 +3915,7 @@ DropRelationsAllBuffers(SMgrRelation *smgr_reln, int nlocators) /* sort the list of rlocators if necessary */ if (use_bsearch) - pg_qsort(locators, n, sizeof(RelFileLocator), rlocator_comparator); + qsort(locators, n, sizeof(RelFileLocator), rlocator_comparator); for (i = 0; i < NBuffers; i++) { @@ -4269,7 +4269,7 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels) /* sort the list of SMgrRelations if necessary */ if (use_bsearch) - pg_qsort(srels, nrels, sizeof(SMgrSortArray), rlocator_comparator); + qsort(srels, nrels, sizeof(SMgrSortArray), rlocator_comparator); for (i = 0; i < NBuffers; i++) { diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c index 162855b158..662f930864 100644 --- a/src/backend/utils/cache/syscache.c +++ b/src/backend/utils/cache/syscache.c @@ -146,14 +146,14 @@ InitCatalogCache(void) Assert(SysCacheSupportingRelOidSize <= lengthof(SysCacheSupportingRelOid)); /* Sort and de-dup OID arrays, so we can use binary search. */ - pg_qsort(SysCacheRelationOid, SysCacheRelationOidSize, - sizeof(Oid), oid_compare); + qsort(SysCacheRelationOid, SysCacheRelationOidSize, + sizeof(Oid), oid_compare); SysCacheRelationOidSize = qunique(SysCacheRelationOid, SysCacheRelationOidSize, sizeof(Oid), oid_compare); - pg_qsort(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize, - sizeof(Oid), oid_compare); + qsort(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize, + sizeof(Oid), oid_compare); SysCacheSupportingRelOidSize = qunique(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize, sizeof(Oid), oid_compare); @@ -668,7 +668,7 @@ RelationSupportsSysCache(Oid relid) /* - * OID comparator for pg_qsort + * OID comparator for qsort */ static int oid_compare(const void *a, const void *b) -- 2.25.1 [text/x-diff] v2-0002-Ensure-comparison-functions-are-transitive.patch (3.1K, ../../20240207205600.GA378707@nathanxps13/3-v2-0002-Ensure-comparison-functions-are-transitive.patch) download | inline diff: From f1a046c3c67266187f5861b27c2fad1daa25101c Mon Sep 17 00:00:00 2001 From: Mats Kindahl <[email protected]> Date: Tue, 6 Feb 2024 14:53:53 +0100 Subject: [PATCH v2 2/2] Ensure comparison functions are transitive There are a few comparison functions to qsort() that are non-transitive because they can cause an overflow. Fix these functions to instead use normal comparisons and return -1, 0, or 1 explicitly. --- src/backend/access/spgist/spgtextproc.c | 6 +++++- src/backend/utils/cache/relcache.c | 12 ++++++++---- src/bin/pg_upgrade/function.c | 20 ++++++++++++++++---- src/bin/psql/crosstabview.c | 9 ++++++++- 4 files changed, 37 insertions(+), 10 deletions(-) diff --git a/src/backend/access/spgist/spgtextproc.c b/src/backend/access/spgist/spgtextproc.c index b8fd0c2ad8..d0a2b4e6e1 100644 --- a/src/backend/access/spgist/spgtextproc.c +++ b/src/backend/access/spgist/spgtextproc.c @@ -325,7 +325,11 @@ cmpNodePtr(const void *a, const void *b) const spgNodePtr *aa = (const spgNodePtr *) a; const spgNodePtr *bb = (const spgNodePtr *) b; - return aa->c - bb->c; + if (aa->c > bb->c) + return 1; + if (aa->c < bb->c) + return -1; + return 0; } Datum diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index ac106b40e3..dc5a100d82 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -4517,10 +4517,14 @@ AttrDefaultFetch(Relation relation, int ndef) static int AttrDefaultCmp(const void *a, const void *b) { - const AttrDefault *ada = (const AttrDefault *) a; - const AttrDefault *adb = (const AttrDefault *) b; - - return ada->adnum - adb->adnum; + AttrNumber ana = ((const AttrDefault *) a)->adnum; + AttrNumber anb = ((const AttrDefault *) b)->adnum; + + if (ana > anb) + return 1; + if (ana < anb) + return -1; + return 0; } /* diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c index af998f74d3..483ede2ce4 100644 --- a/src/bin/pg_upgrade/function.c +++ b/src/bin/pg_upgrade/function.c @@ -32,14 +32,26 @@ library_name_compare(const void *p1, const void *p2) int slen1 = strlen(str1); int slen2 = strlen(str2); int cmp = strcmp(str1, str2); + int p1db = ((const LibraryInfo *) p1)->dbnum; + int p2db = ((const LibraryInfo *) p2)->dbnum; if (slen1 != slen2) - return slen1 - slen2; + { + if (slen1 > slen2) + return 1; + if (slen1 < slen2) + return -1; + return 0; + } + if (cmp != 0) return cmp; - else - return ((const LibraryInfo *) p1)->dbnum - - ((const LibraryInfo *) p2)->dbnum; + + if (p1db > p2db) + return 1; + if (p1db < p2db) + return -1; + return 0; } diff --git a/src/bin/psql/crosstabview.c b/src/bin/psql/crosstabview.c index c6116b7238..de6168bd67 100644 --- a/src/bin/psql/crosstabview.c +++ b/src/bin/psql/crosstabview.c @@ -709,5 +709,12 @@ pivotFieldCompare(const void *a, const void *b) static int rankCompare(const void *a, const void *b) { - return *((const int *) a) - *((const int *) b); + const int an = *(const int *) a; + const int bn = *(const int *) b; + + if (an > bn) + return 1; + if (an < bn) + return -1; + return 0; } -- 2.25.1 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-07 21:48 Andres Freund <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Andres Freund @ 2024-02-07 21:48 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: Mats Kindahl <[email protected]>; Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; [email protected] Hi, On 2024-02-07 20:46:56 +0200, Heikki Linnakangas wrote: > > The routines modified do a subtraction of int:s and return that, which > > can cause an overflow. This method is used for some int16 as well but > > since standard conversions in C will perform the arithmetics in "int" > > precision, this cannot overflow, so added a comment there. It might > > still be a good idea to follow the same pattern for the int16 routines, > > but since there is no bug there, I did not add them to the patch. > > Doesn't hurt to fix the comparison functions, and +1 on using the same > pattern everywhere. It actually can hurt - the generated code will often be slower. E.g. #include <stdint.h> int cmp_sub(int16_t a, int16_t b) { return (int32_t) a - (int32_t) b; } int cmp_if(int16_t a, int16_t b) { if (a < b) return -1; if (a > b) return 1; return 0; } yields cmp_sub: movsx eax, di movsx esi, si sub eax, esi ret cmp_if: xor eax, eax cmp di, si mov edx, -1 setg al cmovl eax, edx ret with gcc -O3. With other compilers, e.g. msvc, the difference is considerably bigger, due to msvc for some reason not using cmov. See https://godbolt.org/z/34qerPaPE for a few more details. Now, in most cases this won't matter, the sorting isn't performance critical. But I don't think it's a good idea to standardize on a generally slower pattern. Not that that's a good test, but I did quickly benchmark [1] this with intarray. There's about a 10% difference in performance between using the existing compASC() and one using return (int64) *(const int32 *) a - (int64) *(const int32 *) b; Perhaps we could have a central helper for this somewhere? Greetings, Andres Freund [1] -- prep CREATE EXTENSION IF NOT EXISTS intarray; DROP TABLE IF EXISTS arrays_to_sort; CREATE TABLE arrays_to_sort AS SELECT array_shuffle(a) arr FROM (SELECT ARRAY(SELECT generate_series(1, 1000000)) a), generate_series(1, 10); -- bench SELECT (sort(arr))[1] FROM arrays_to_sort; ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-07 22:21 Nathan Bossart <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Nathan Bossart @ 2024-02-07 22:21 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Mats Kindahl <[email protected]>; Tom Lane <[email protected]>; [email protected] On Wed, Feb 07, 2024 at 01:48:57PM -0800, Andres Freund wrote: > Now, in most cases this won't matter, the sorting isn't performance > critical. But I don't think it's a good idea to standardize on a generally > slower pattern. > > Not that that's a good test, but I did quickly benchmark [1] this with > intarray. There's about a 10% difference in performance between using the > existing compASC() and one using > return (int64) *(const int32 *) a - (int64) *(const int32 *) b; > > > Perhaps we could have a central helper for this somewhere? Maybe said helper could use __builtin_sub_overflow() and fall back to the slow "if" version only if absolutely necessary. The assembly for that looks encouraging, but I still need to actually test it... -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-08 00:42 Andres Freund <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Andres Freund @ 2024-02-08 00:42 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Mats Kindahl <[email protected]>; Tom Lane <[email protected]>; [email protected] Hi, On 2024-02-07 16:21:24 -0600, Nathan Bossart wrote: > On Wed, Feb 07, 2024 at 01:48:57PM -0800, Andres Freund wrote: > > Now, in most cases this won't matter, the sorting isn't performance > > critical. But I don't think it's a good idea to standardize on a generally > > slower pattern. > > > > Not that that's a good test, but I did quickly benchmark [1] this with > > intarray. There's about a 10% difference in performance between using the > > existing compASC() and one using > > return (int64) *(const int32 *) a - (int64) *(const int32 *) b; > > > > > > Perhaps we could have a central helper for this somewhere? > > Maybe said helper could use __builtin_sub_overflow() and fall back to the > slow "if" version only if absolutely necessary. I suspect that'll be worse code in the common case, given the cmov generated by gcc & clang for the typical branch-y formulation. But it's worth testing. > The assembly for that looks encouraging, but I still need to actually test > it... Possible. For 16bit upcasting to 32bit is clearly the best way. For 32 bit that doesn't work, given the 32bit return, so we need something more. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-08 01:52 Nathan Bossart <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 2 replies; 16+ messages in thread From: Nathan Bossart @ 2024-02-08 01:52 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Mats Kindahl <[email protected]>; Tom Lane <[email protected]>; [email protected] On Wed, Feb 07, 2024 at 04:42:07PM -0800, Andres Freund wrote: > On 2024-02-07 16:21:24 -0600, Nathan Bossart wrote: >> The assembly for that looks encouraging, but I still need to actually test >> it... > > Possible. For 16bit upcasting to 32bit is clearly the best way. For 32 bit > that doesn't work, given the 32bit return, so we need something more. For the same compASC() test, I see an ~8.4% improvement with your int64 code and a ~3.4% improvement with this: int compASC(const void *a, const void *b) { int result; if (unlikely(pg_sub_s32_overflow(*(const int32 *) a, *(const int32 *) b, &result))) { if (*(const int32 *) a > *(const int32 *) b) return 1; if (*(const int32 *) a < *(const int32 *) b) return -1; return 0; } return result; } -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-08 02:06 Andres Freund <[email protected]> parent: Nathan Bossart <[email protected]> 1 sibling, 2 replies; 16+ messages in thread From: Andres Freund @ 2024-02-08 02:06 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Mats Kindahl <[email protected]>; Tom Lane <[email protected]>; [email protected] Hi, On 2024-02-07 19:52:11 -0600, Nathan Bossart wrote: > On Wed, Feb 07, 2024 at 04:42:07PM -0800, Andres Freund wrote: > > On 2024-02-07 16:21:24 -0600, Nathan Bossart wrote: > >> The assembly for that looks encouraging, but I still need to actually test > >> it... > > > > Possible. For 16bit upcasting to 32bit is clearly the best way. For 32 bit > > that doesn't work, given the 32bit return, so we need something more. > > For the same compASC() test, I see an ~8.4% improvement with your int64 > code Just to be clear, that code unfortuntely isn't correct, the return value is a 32 bit integer, so the 64bit difference doesn't help. In contrast to the 16bit case. > and a ~3.4% improvement with this: I guess that's still something. Another branchless variant is (a > b) - (a < b). It seems to get a similar improvement as the overflow-checking version. Greetings, Andres Freund ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-08 02:38 Thomas Munro <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Thomas Munro @ 2024-02-08 02:38 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Heikki Linnakangas <[email protected]>; Mats Kindahl <[email protected]>; Tom Lane <[email protected]>; [email protected] On Thu, Feb 8, 2024 at 3:06 PM Andres Freund <[email protected]> wrote: > On 2024-02-07 19:52:11 -0600, Nathan Bossart wrote: > > On Wed, Feb 07, 2024 at 04:42:07PM -0800, Andres Freund wrote: > > > On 2024-02-07 16:21:24 -0600, Nathan Bossart wrote: > > >> The assembly for that looks encouraging, but I still need to actually test > > >> it... > > > > > > Possible. For 16bit upcasting to 32bit is clearly the best way. For 32 bit > > > that doesn't work, given the 32bit return, so we need something more. > > > > For the same compASC() test, I see an ~8.4% improvement with your int64 > > code > > Just to be clear, that code unfortuntely isn't correct, the return value is a > 32 bit integer, so the 64bit difference doesn't help. In contrast to the 16bit > case. Perhaps you could wrap it in a branch-free sign() function so you get a narrow answer? https://stackoverflow.com/questions/14579920/fast-sign-of-integer-in-c ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-08 02:45 Nathan Bossart <[email protected]> parent: Andres Freund <[email protected]> 1 sibling, 0 replies; 16+ messages in thread From: Nathan Bossart @ 2024-02-08 02:45 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Mats Kindahl <[email protected]>; Tom Lane <[email protected]>; [email protected] On Wed, Feb 07, 2024 at 06:06:37PM -0800, Andres Freund wrote: > Another branchless variant is (a > b) - (a < b). It seems to get a similar > improvement as the overflow-checking version. Well, that's certainly more elegant. I updated the patch to that approach for now. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com Attachments: [text/x-diff] v3-0001-remove-direct-calls-to-pg_qsort.patch (3.6K, ../../20240208024558.GB445153@nathanxps13/2-v3-0001-remove-direct-calls-to-pg_qsort.patch) download | inline diff: From 8668a0dd83ecbdd356c76416b8398138407ef829 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Wed, 7 Feb 2024 14:29:04 -0600 Subject: [PATCH v3 1/2] remove direct calls to pg_qsort() --- contrib/pg_prewarm/autoprewarm.c | 4 ++-- src/backend/access/brin/brin_minmax_multi.c | 2 +- src/backend/storage/buffer/bufmgr.c | 4 ++-- src/backend/utils/cache/syscache.c | 10 +++++----- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c index 06ee21d496..248b9914a3 100644 --- a/contrib/pg_prewarm/autoprewarm.c +++ b/contrib/pg_prewarm/autoprewarm.c @@ -346,8 +346,8 @@ apw_load_buffers(void) FreeFile(file); /* Sort the blocks to be loaded. */ - pg_qsort(blkinfo, num_elements, sizeof(BlockInfoRecord), - apw_compare_blockinfo); + qsort(blkinfo, num_elements, sizeof(BlockInfoRecord), + apw_compare_blockinfo); /* Populate shared memory state. */ apw_state->block_info_handle = dsm_segment_handle(seg); diff --git a/src/backend/access/brin/brin_minmax_multi.c b/src/backend/access/brin/brin_minmax_multi.c index 3ffaad3e42..2c29aa3d4e 100644 --- a/src/backend/access/brin/brin_minmax_multi.c +++ b/src/backend/access/brin/brin_minmax_multi.c @@ -1369,7 +1369,7 @@ build_distances(FmgrInfo *distanceFn, Oid colloid, * Sort the distances in descending order, so that the longest gaps are at * the front. */ - pg_qsort(distances, ndistances, sizeof(DistanceValue), compare_distances); + qsort(distances, ndistances, sizeof(DistanceValue), compare_distances); return distances; } diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index eb1ec3b86d..07575ef312 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -3915,7 +3915,7 @@ DropRelationsAllBuffers(SMgrRelation *smgr_reln, int nlocators) /* sort the list of rlocators if necessary */ if (use_bsearch) - pg_qsort(locators, n, sizeof(RelFileLocator), rlocator_comparator); + qsort(locators, n, sizeof(RelFileLocator), rlocator_comparator); for (i = 0; i < NBuffers; i++) { @@ -4269,7 +4269,7 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels) /* sort the list of SMgrRelations if necessary */ if (use_bsearch) - pg_qsort(srels, nrels, sizeof(SMgrSortArray), rlocator_comparator); + qsort(srels, nrels, sizeof(SMgrSortArray), rlocator_comparator); for (i = 0; i < NBuffers; i++) { diff --git a/src/backend/utils/cache/syscache.c b/src/backend/utils/cache/syscache.c index 162855b158..662f930864 100644 --- a/src/backend/utils/cache/syscache.c +++ b/src/backend/utils/cache/syscache.c @@ -146,14 +146,14 @@ InitCatalogCache(void) Assert(SysCacheSupportingRelOidSize <= lengthof(SysCacheSupportingRelOid)); /* Sort and de-dup OID arrays, so we can use binary search. */ - pg_qsort(SysCacheRelationOid, SysCacheRelationOidSize, - sizeof(Oid), oid_compare); + qsort(SysCacheRelationOid, SysCacheRelationOidSize, + sizeof(Oid), oid_compare); SysCacheRelationOidSize = qunique(SysCacheRelationOid, SysCacheRelationOidSize, sizeof(Oid), oid_compare); - pg_qsort(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize, - sizeof(Oid), oid_compare); + qsort(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize, + sizeof(Oid), oid_compare); SysCacheSupportingRelOidSize = qunique(SysCacheSupportingRelOid, SysCacheSupportingRelOidSize, sizeof(Oid), oid_compare); @@ -668,7 +668,7 @@ RelationSupportsSysCache(Oid relid) /* - * OID comparator for pg_qsort + * OID comparator for qsort */ static int oid_compare(const void *a, const void *b) -- 2.25.1 [text/x-diff] v3-0002-Ensure-comparison-functions-are-transitive.patch (3.0K, ../../20240208024558.GB445153@nathanxps13/3-v3-0002-Ensure-comparison-functions-are-transitive.patch) download | inline diff: From a74708f14d6e862b836b2d5106491a8db3114433 Mon Sep 17 00:00:00 2001 From: Mats Kindahl <[email protected]> Date: Tue, 6 Feb 2024 14:53:53 +0100 Subject: [PATCH v3 2/2] Ensure comparison functions are transitive There are a few comparison functions to qsort() that are non-transitive because they can cause an overflow. Fix these functions to instead use normal comparisons and return -1, 0, or 1 explicitly. --- src/backend/access/spgist/spgtextproc.c | 2 +- src/backend/utils/cache/relcache.c | 9 ++++++--- src/bin/pg_upgrade/function.c | 10 ++++++---- src/bin/psql/crosstabview.c | 5 ++++- 4 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/backend/access/spgist/spgtextproc.c b/src/backend/access/spgist/spgtextproc.c index b8fd0c2ad8..e561bde63e 100644 --- a/src/backend/access/spgist/spgtextproc.c +++ b/src/backend/access/spgist/spgtextproc.c @@ -325,7 +325,7 @@ cmpNodePtr(const void *a, const void *b) const spgNodePtr *aa = (const spgNodePtr *) a; const spgNodePtr *bb = (const spgNodePtr *) b; - return aa->c - bb->c; + return (aa->c > bb->c) - (aa->c < bb->c); } Datum diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index ac106b40e3..54682a0e60 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -4517,10 +4517,13 @@ AttrDefaultFetch(Relation relation, int ndef) static int AttrDefaultCmp(const void *a, const void *b) { - const AttrDefault *ada = (const AttrDefault *) a; - const AttrDefault *adb = (const AttrDefault *) b; + AttrNumber ana = ((const AttrDefault *) a)->adnum; + AttrNumber anb = ((const AttrDefault *) b)->adnum; - return ada->adnum - adb->adnum; + /* ensure upcasting approach is transitive */ + Assert(sizeof(AttrNumber) == 2); + + return (int) ana - (int) anb; } /* diff --git a/src/bin/pg_upgrade/function.c b/src/bin/pg_upgrade/function.c index af998f74d3..b421500bbb 100644 --- a/src/bin/pg_upgrade/function.c +++ b/src/bin/pg_upgrade/function.c @@ -32,14 +32,16 @@ library_name_compare(const void *p1, const void *p2) int slen1 = strlen(str1); int slen2 = strlen(str2); int cmp = strcmp(str1, str2); + int p1db = ((const LibraryInfo *) p1)->dbnum; + int p2db = ((const LibraryInfo *) p2)->dbnum; if (slen1 != slen2) - return slen1 - slen2; + return (slen1 > slen2) - (slen1 < slen2); + if (cmp != 0) return cmp; - else - return ((const LibraryInfo *) p1)->dbnum - - ((const LibraryInfo *) p2)->dbnum; + + return (p1db > p2db) - (p1db < p2db); } diff --git a/src/bin/psql/crosstabview.c b/src/bin/psql/crosstabview.c index c6116b7238..7660f0ed4b 100644 --- a/src/bin/psql/crosstabview.c +++ b/src/bin/psql/crosstabview.c @@ -709,5 +709,8 @@ pivotFieldCompare(const void *a, const void *b) static int rankCompare(const void *a, const void *b) { - return *((const int *) a) - *((const int *) b); + const int an = *(const int *) a; + const int bn = *(const int *) b; + + return (an > bn) - (an < bn); } -- 2.25.1 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-08 02:49 Thomas Munro <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 0 replies; 16+ messages in thread From: Thomas Munro @ 2024-02-08 02:49 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Heikki Linnakangas <[email protected]>; Mats Kindahl <[email protected]>; Tom Lane <[email protected]>; [email protected] On Thu, Feb 8, 2024 at 3:38 PM Thomas Munro <[email protected]> wrote: > Perhaps you could wrap it in a branch-free sign() function so you get > a narrow answer? > > https://stackoverflow.com/questions/14579920/fast-sign-of-integer-in-c Ah, strike that, it is much like the suggested (a > b) - (a < b) but with extra steps... ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-08 11:01 Mats Kindahl <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 16+ messages in thread From: Mats Kindahl @ 2024-02-08 11:01 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; Tom Lane <[email protected]>; [email protected] On Wed, Feb 7, 2024 at 9:56 PM Nathan Bossart <[email protected]> wrote: > On Wed, Feb 07, 2024 at 08:46:56PM +0200, Heikki Linnakangas wrote: > > Doesn't hurt to fix the comparison functions, and +1 on using the same > > pattern everywhere. > > I attached a new version of the patch with some small adjustments. I > haven't looked through all in-tree qsort() comparators to see if any others > need to be adjusted, but we should definitely do so as part of this thread. > Mats, are you able to do this? > Sure, I checked them and the only ones remaining are those using int16. Shall I modify those as well? > > However, we use our qsort() with user-defined comparison functions, and > we > > cannot make any guarantees about what they might do. So we must ensure > that > > our qsort() doesn't overflow, no matter what the comparison function > does. > > > > Looking at our ST_SORT(), it seems safe to me. > > Cool. > > -- > Nathan Bossart > Amazon Web Services: https://aws.amazon.com > ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-09 08:19 Andrey M. Borodin <[email protected]> parent: Nathan Bossart <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Andrey M. Borodin @ 2024-02-09 08:19 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; Heikki Linnakangas <[email protected]>; Mats Kindahl <[email protected]>; Tom Lane <[email protected]>; [email protected] > On 8 Feb 2024, at 06:52, Nathan Bossart <[email protected]> wrote: > > For the same compASC() test, I see an ~8.4% improvement with your int64 > code and a ~3.4% improvement with this: If we care about branch prediction in comparison function, maybe we could produce sorting that inlines comparator, thus eliminating function call to comparator? We convert comparison logic to int, to extract comparison back then. I bet “call" is more expensive than “if". Best regards, Andrey Borodin. ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-09 16:32 Nathan Bossart <[email protected]> parent: Andrey M. Borodin <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Nathan Bossart @ 2024-02-09 16:32 UTC (permalink / raw) To: Andrey M. Borodin <[email protected]>; +Cc: Andres Freund <[email protected]>; Heikki Linnakangas <[email protected]>; Mats Kindahl <[email protected]>; Tom Lane <[email protected]>; [email protected] On Fri, Feb 09, 2024 at 01:19:49PM +0500, Andrey M. Borodin wrote: > If we care about branch prediction in comparison function, maybe we could > produce sorting that inlines comparator, thus eliminating function call > to comparator? We convert comparison logic to int, to extract comparison > back then. > > I bet “call" is more expensive than “if". It might make sense to have a couple of built-in qsort implementations for pointers to integers, pointers to unsigned integers, etc. However, a lot of current use-cases require inspecting specific fields of structs, so (assuming I understand your proposal correctly), we'd end up with many qsort implementations. If that can be made simple and elegant and demonstrates substantial improvements, then it might be worth considering, but I'm somewhat skeptical that the current uses are performance-sensitive enough to be worth the effort. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: glibc qsort() vulnerability @ 2024-02-09 19:02 Andrey Borodin <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 0 replies; 16+ messages in thread From: Andrey Borodin @ 2024-02-09 19:02 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Andres Freund <[email protected]>; Heikki Linnakangas <[email protected]>; Mats Kindahl <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers mailing list <[email protected]> > On 9 Feb 2024, at 21:32, Nathan Bossart <[email protected]> wrote: > a lot > of current use-cases require inspecting specific fields of structs Yes, I'm proposing to pass to sorting routine not a comparator, but value extractor. And then rely on operators <,>,==. In a pseudocode: instead of sort(array, (a,b)->a.field-b.field) use sort(array, x->x.field). And rely on "field" being comparable. > If that can be made simple and elegant and > demonstrates substantial improvements I'll try to produce a PoC and measure it with Andres' intarray test. > On 9 Feb 2024, at 23:40, Andres Freund <[email protected]> wrote: > > We have some infrastructure for that actually, see sort_template.h. But > perhaps we should define a static inline of the generic pg_qsort() even. OTOH, > plenty places that'll just end up to a pointless amount of code emitted to > sort ~5 elements on average. I think there might be another benefit. It's easier to think about values order than function comparator that returns -1,0,+1... >> I bet “call" is more expensive than “if". > > Not really in this case. The call is perfectly predictable - a single qsort() > will use the same callback for every comparison, whereas the if is perfectly > *unpredictable*. A branch mispredict is far more expensive than a correctly > predicted function call. Oh, make sense... I did not understand that. But does cpu predicts what instruction to fetch even after a call instruction? These cpus are really neat things... so, probably, yes, it does. Best regards, Andrey Borodin. ^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2024-02-09 19:02 UTC | newest] Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-31 19:40 [PATCH v28 09/11] pg_ls_*/pg_stat_file to show file *type*.. Justin Pryzby <[email protected]> 2024-02-07 09:09 Re: glibc qsort() vulnerability Mats Kindahl <[email protected]> 2024-02-07 18:46 ` Re: glibc qsort() vulnerability Heikki Linnakangas <[email protected]> 2024-02-07 20:56 ` Re: glibc qsort() vulnerability Nathan Bossart <[email protected]> 2024-02-08 11:01 ` Re: glibc qsort() vulnerability Mats Kindahl <[email protected]> 2024-02-07 21:48 ` Re: glibc qsort() vulnerability Andres Freund <[email protected]> 2024-02-07 22:21 ` Re: glibc qsort() vulnerability Nathan Bossart <[email protected]> 2024-02-08 00:42 ` Re: glibc qsort() vulnerability Andres Freund <[email protected]> 2024-02-08 01:52 ` Re: glibc qsort() vulnerability Nathan Bossart <[email protected]> 2024-02-08 02:06 ` Re: glibc qsort() vulnerability Andres Freund <[email protected]> 2024-02-08 02:38 ` Re: glibc qsort() vulnerability Thomas Munro <[email protected]> 2024-02-08 02:49 ` Re: glibc qsort() vulnerability Thomas Munro <[email protected]> 2024-02-08 02:45 ` Re: glibc qsort() vulnerability Nathan Bossart <[email protected]> 2024-02-09 08:19 ` Re: glibc qsort() vulnerability Andrey M. Borodin <[email protected]> 2024-02-09 16:32 ` Re: glibc qsort() vulnerability Nathan Bossart <[email protected]> 2024-02-09 19:02 ` Re: glibc qsort() vulnerability Andrey Borodin <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox