powa-team / pg_qualstats

A PostgreSQL extension for collecting statistics about predicates, helping find what indices are missing
Other
272 stars 26 forks source link

different utf8const handling #43

Closed RekGRpth closed 2 years ago

RekGRpth commented 2 years ago

why different utf8const handling? https://github.com/powa-team/pg_qualstats/blob/75e1c0987cbcf30ed99c8b9726ee05b410e50e73/pg_qualstats.c#L1362 and https://github.com/powa-team/pg_qualstats/blob/75e1c0987cbcf30ed99c8b9726ee05b410e50e73/pg_qualstats.c#L1700

rjuju commented 2 years ago

Hi,

The 1st one is for bool constants. Those can only have a finite number of values:

    switch (expr->booltesttype)
    {
        case IS_TRUE:
            constant = "TRUE::bool";
            opoid = BooleanEqualOperator;
            break;
        case IS_FALSE:
            constant = "FALSE::bool";
            opoid = BooleanEqualOperator;
            break;
        case IS_NOT_TRUE:
            constant = "TRUE::bool";
            opoid = BooleanNotEqualOperator;
            break;
        case IS_NOT_FALSE:
            constant = "FALSE::bool";
            opoid = BooleanNotEqualOperator;
            break;
        case IS_UNKNOWN:
            constant = "NULL::bool";
            opoid = BooleanEqualOperator;
            break;
        case IS_NOT_UNKNOWN:
            constant = "NULL::bool";
            opoid = BooleanNotEqualOperator;
            break;
        default:
            /* Bail out */
            return NULL;
    }

Given their content, they shouldn't contain multibytes caracters and even if they did they would still fit in the fixed 80B constant size. We can therefore simply use strncpy() without any risk. That being said it would be better to eventually use some safer API.

For the 2nd one, we do have to handle the possibility of truncating the constant (not breaking multibyte caracters) so we use a safer API where there's no risk of buffer overflow. Note that this was originally written as the 1st one using strncpy, and later changed with a safer API to handle multibyte truncation.

RekGRpth commented 2 years ago

on first there is compiler warning

In function 'pgqs_process_booltest',
    inlined from 'pgqs_whereclause_tree_walker' at pg_qualstats.c:1765:4:
pg_qualstats.c:1362:4: warning: 'strncpy' specified bound depends on the length of the source argument [-Wstringop-overflow=]
1362 |    strncpy(entry->constvalue, utf8const, strlen(utf8const));
     |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
gcc -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Werror=vla -Wendif-labels -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -Wno-format-truncation -Wno-stringop-truncation -O2 -fomit-frame-pointer -fPIC pg_qualstats.o -L/usr/lib -Wl,--as-needed,-O1,--sort-common -L/usr/lib/llvm12/lib  -Wl,--as-needed  -shared -o pg_qualstats.so
rjuju commented 2 years ago

Ah indeed the length is wrong. Fortunately this can't lead to any bug right now but it should be fixed. Thanks for the report!

rjuju commented 2 years ago

Commit https://github.com/powa-team/pg_qualstats/commit/69fe2d401611b87b05f343f8cf746b3638182b24 should fix that issue!