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

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.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/