Ubsan complaint on kestrel - Mailing list pgsql-hackers

Hi,

I just upgraded buildfarm animal kestrel (a buildfarm animal running with
ubsan) to a newer version of clang.  Unfortunately this causes it to fail.

The buildfarm output doesn't contain enough information to debug that
unfortunately. But I triggered it manually and meson-logs/testlog.txt did
contain this:

----------------------------------- stdout -----------------------------------
Running in no-clean mode.  Mistakes will not be cleaned up.
The files belonging to this database system will be owned by user "bf".
This user must also own the server process.

The database cluster will be initialized with this locale configuration:
  locale provider:   libc
  LC_COLLATE:  en_US.UTF-8
  LC_CTYPE:    en_US.UTF-8
  LC_MESSAGES: C
  LC_MONETARY: en_US.UTF-8
  LC_NUMERIC:  en_US.UTF-8
  LC_TIME:     en_US.UTF-8
The default database encoding has accordingly been set to "UTF8".
The default text search configuration will be set to "english".

Data page checksums are enabled.

creating directory /home/bf/bf-build/kestrel/HEAD/pgsql.build/tmp_install/initdb-template ... ok
creating subdirectories ... ok
selecting dynamic shared memory implementation ... posix
selecting default "max_connections" ... 25
selecting default "autovacuum_worker_slots" ... 4
selecting default "shared_buffers" ... 400kB
selecting default time zone ... Etc/UTC
creating configuration files ... ok
running bootstrap script ...
----------------------------------- stderr -----------------------------------
../pgsql/src/backend/utils/hash/dynahash.c:1021:4: runtime error: call to function string_compare through pointer to
incorrectfunction type 'int (*)(const void *, const void *, unsigned long)'
 
/home/bf/bf-build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/utils/hash/dynahash.c:308: note: string_compare defined
here
    #0 0x55f30c0291dd in hash_search_with_hash_value
/home/bf/bf-build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/utils/hash/dynahash.c:1021:4
    #1 0x55f30c07f21d in pg_tzset /home/bf/bf-build/kestrel/HEAD/pgsql.build/../pgsql/src/timezone/pgtz.c:260:24
    #2 0x55f30baf5e44 in check_timezone
/home/bf/bf-build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/commands/variable.c:341:13
    #3 0x55f30c03d610 in call_string_check_hook
/home/bf/bf-build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/utils/misc/guc.c:6922:8
    #4 0x55f30c038490 in InitializeOneGUCOption
/home/bf/bf-build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/utils/misc/guc.c:1725:10
    #5 0x55f30c037bf2 in InitializeGUCOptions
/home/bf/bf-build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/utils/misc/guc.c:1556:3
    #6 0x55f30b9c46d4 in BootstrapModeMain
/home/bf/bf-build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/bootstrap/bootstrap.c:212:2
    #7 0x55f30bbc5c44 in main /home/bf/bf-build/kestrel/HEAD/pgsql.build/../pgsql/src/backend/main/main.c:213:4
    #8 0x7f4653f6bca7 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #9 0x7f4653f6bd64 in __libc_start_main csu/../csu/libc-start.c:360:3
    #10 0x55f30b84e3e0 in _start
(/home/bf/bf-build/kestrel/HEAD/pgsql.build/tmp_install/home/bf/bf-build/kestrel/HEAD/inst/bin/postgres+0x5f53e0)
(BuildId:63499d2fda05cc488eff7507a72a5be9ad156139)
 

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior ../pgsql/src/backend/utils/hash/dynahash.c:1021:4
Aborted (core dumped)
child process exited with exit code 134
initdb: data directory "/home/bf/bf-build/kestrel/HEAD/pgsql.build/tmp_install/initdb-template" not removed at user's
request
==============================================================================


And while that complaint is perhaps a bit nit-picky, it's not wrong.


/*
 * Key comparison functions must have this signature.  Comparison functions
 * return zero for match, nonzero for no match.  (The comparison function
 * definition is designed to allow memcmp() and strncmp() to be used directly
 * as key comparison functions.)
 */
typedef int (*HashCompareFunc) (const void *key1, const void *key2,
                                Size keysize);

vs

/*
 * HashCompareFunc for string keys
 *
 * Because we copy keys with strlcpy(), they will be truncated at keysize-1
 * bytes, so we can only compare that many ... hence strncmp is almost but
 * not quite the right thing.
 */
static int
string_compare(const char *key1, const char *key2, Size keysize)
{
    return strncmp(key1, key2, keysize - 1);
}

The only reason we don't get a compiler warning is that we cast the function
type:
    else if (hashp->hash == string_hash)
        hashp->match = (HashCompareFunc) string_compare;


I don't really understand why we define string_compare() with the wrong
signature just to then cast it to the right one?

<dig>

It's been this way for a long time. I suspect that this actually might have
been a holdover from when we directly used strncmp as the comparator function,
in

commit c92f7e258ee
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   2006-09-27 18:40:10 +0000

    Replace strncpy with strlcpy in selected places that seem possibly relevant
    to performance.  (A wholesale effort to get rid of strncpy should be
    undertaken sometime, but not during beta.)  This commit also fixes dynahash.c
    to correctly truncate overlength string keys for hashtables, so that its
    callers don't have to anymore.


Tom, do you see any reason to not instead do the typecase inside
string_compare()?


Greetings,

Andres Freund