Re: Fix array access (src/bin/pg_dump/pg_dump.c)

Em sáb., 11 de out. de 2025 às 02:24, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
escreveu:

>
> On Oct 11, 2025, at 00:41, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
>
> Em ter., 12 de nov. de 2024 às 19:13, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
> escreveu:
>
>> Em ter., 12 de nov. de 2024 às 16:11, Alvaro Herrera <
>> alvherre(at)alvh(dot)no-ip(dot)org> escreveu:
>>
>>> On 2024-Nov-12, Ranier Vilela wrote:
>>>
>>> > Per Coverity.
>>> >
>>> > The function *determineNotNullFlags* has a little oversight.
>>> > The struct field *notnull_islocal* is an array.
>>> >
>>> > I think this is a simple typo.
>>> > Fix using array notation access.
>>>
>>> Yeah, thanks, I had been made aware of this bug. Before fixing I'd like
>>> to construct a test case that tickles that code, because it's currently
>>> uncovered *shudder*
>>>
>> Thanks for taking care of this.
>>
> Ping.
>
>
> I tried to debug this bug, and it looks like this bug can never be
> triggered.
>
> To do the test, I created two tables:
> ```
> evantest=# CREATE TABLE parent_nn(a int NOT NULL, b int);
> CREATE TABLE
> evantest=# CREATE TABLE child_nn() INHERITS (parent_nn);
> CREATE TABLE
> evantest=# ALTER TABLE child_nn ALTER COLUMN b SET NOT NULL;
> ALTER TABLE
> ```
>
> Then let’s look at the code:
>
> /*
> * In binary upgrade of inheritance child tables, must have a
> * constraint name that we can UPDATE later; same if there's a
> * comment on the constraint.
> */
> if ((dopt->binary_upgrade &&
> !tbinfo->ispartition &&
> !tbinfo->notnull_islocal) ||
> !PQgetisnull(res, r, i_notnull_comment)) // A
> {
> const char *val = PQgetvalue(res, r, i_notnull_name); // B
> tbinfo->notnull_constrs[j] =
> pstrdup(val);
> }
> else
> {
> char *default_name;
> const char *val = PQgetvalue(res, r, i_notnull_name);
>
> /* XXX should match ChooseConstraintName better */
> default_name = psprintf("%s_%s_not_null", tbinfo->dobj.name,
> tbinfo->attnames[j]);
> if (strcmp(default_name, // C
> val) == 0)
> tbinfo->notnull_constrs[j] = "";
> else
> {
> tbinfo->notnull_constrs[j] = // D
> pstrdup(val);
> }
> free(default_name);
> }
>
> Notice that I marked four lines with A/B/C/D.
>
> For child table’s column “b”, when it reaches line A, it hits the bug,
> as tbinfo->notnull_islocal[j] should be false, but tbinfo->notnull_islocal
> is always true.
>
> If the bug is fixed, as tbinfo->notnull_islocal[j] is false, it will enter
> the “if” clause, in line B, PGgetvalue() will return “parent_nn_a_not_null”.
>
> With this bug, if will go the the “else” clause, run strcmp() in line C.
> Here, “default_name” is built from the table name, its value is
> “child_nn_a_not_null”, while PGgetvalue() is “parent_nn_a_not_null”, thus
> it won’t meet the “if” of “strcmp”, instead it goes to line D, that runs
> the same assignment as in line B.
>
> So, Ranier, please let me know if you have an example that generates the
> wrong result, then I can verify the fix with your test case.
>
> But I believe we should still fix the bug: 1) otherwise the code is
> confusing 2) after fixing, when tbinfo->notnull_islocal[j] is false, the
> execution path is shorter 3) to make Coverity happy.
>
> I also added a TAP test with test cases for determining NULL:
>
> ```
> chaol(at)ChaodeMacBook-Air pg_dump % make check PROVE_TESTS='t/
> 011_dump_determine_null.pl'
> # +++ tap check in src/bin/pg_dump +++
> t/011_dump_determine_null.pl .. ok
> All tests successful.
> Files=1, Tests=5, 2 wallclock secs ( 0.00 usr 0.01 sys + 0.08 cusr
> 0.31 csys = 0.40 CPU)
> Result: PASS
> ```
>
> Please see the attached patch.
>
Did you read the entire thread?
I think it's very rude and inelegant to suggest a patch while taking
advantage of another patch.

Best regards,
Ranier Vilela