| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
| Cc: | Peter Eisentraut <peter(at)eisentraut(dot)org>, jian he <jian(dot)universality(at)gmail(dot)com> |
| Subject: | Fix bug of COPY (on_error set_null) |
| Date: | 2026-05-15 01:12:35 |
| Message-ID: | 90EF4FA7-9FE2-4EDE-AC04-C349487F7C2A@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hi,
I just tested “Add COPY (column list) (on_error set_null) option”. While tracing a normal case, I found a mistake:
In BeginCopyFrom(), cstate->domain_with_constraint is allocated using the length of the specified column list, and set using the index in that column list:
```
int attr_count = list_length(cstate->attnumlist);
/*
* When data type conversion fails and ON_ERROR is SET_NULL, we need
* ensure that the input column allow null values. ExecConstraints()
* will cover most of the cases, but it does not verify domain
* constraints. Therefore, for constrained domains, the null value
* check must be performed during the initial string-to-datum
* conversion (see CopyFromTextLikeOneRow()).
*/
cstate->domain_with_constraint = palloc0_array(bool, attr_count); <== allocate with length of column list from SQL
foreach_int(attno, cstate->attnumlist)
{
int i = foreach_current_index(attno);
Form_pg_attribute att = TupleDescAttr(tupDesc, attno - 1);
cstate->domain_with_constraint[i] = DomainHasConstraints(att->atttypid, NULL); <= set with index of column list from SQL
}
```
However, cstate->domain_with_constraint is read in CopyFromTextLikeOneRow(), where it is accessed using the actual attribute number:
```
/* Loop to read the user attributes on the line. */
foreach(cur, cstate->attnumlist)
{
int attnum = lfirst_int(cur);
int m = attnum - 1;
...
if (!cstate->domain_with_constraint[m] ||
```
So, if the column list specified in SQL is shorter than the table’s actual attribute list, this may cause an out-of-bounds read.
Based on this finding, it was easy to create a repro:
```
evantest=# CREATE DOMAIN dnn AS int NOT NULL;
CREATE DOMAIN
evantest=# CREATE TABLE t (a int, b dnn);
CREATE TABLE
evantest=# COPY t(b) FROM STDIN WITH (on_error set_null);
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself, or an EOF signal.
>> x
>> \.
NOTICE: in 1 row, columns were set to null due to data type incompatibility
COPY 1
evantest=# select * from t;
a | b
---+---
|
(1 row)
```
Here, the table has two columns, and the COPY command only specifies column b. Since I gave invalid input for b, COPY tries to set it to NULL. However, b is a NOT NULL domain, so the COPY should fail. Instead, a row with NULL in b is successfully inserted, which proves this is a real bug.
To fix the bug, there are two options:
1. In BeginCopyFrom(), allocate cstate->domain_with_constraint using the actual number of table attributes, and set it by actual attribute number.
2. In CopyFromTextLikeOneRow(), access cstate->domain_with_constraint using the index in the SQL column list.
I chose option 1, because it keeps cstate->domain_with_constraint in the same form as other arrays such as cstate->typioparams[m], which makes the code easier to read. This may use a little extra memory, but since cstate->domain_with_constraint is a small boolean array and is short-lived, I don’t think that matters much.
While here, I also noticed a few palloc calls that could be switched to palloc_array, so I changed them as well.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
| Attachment | Content-Type | Size |
|---|---|---|
| v1-0001-Fix-ON_ERROR-SET_NULL-domain-check-with-reordered.patch | application/octet-stream | 6.7 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Tom Lane | 2026-05-15 02:09:29 | Re: Re-add recently-removed tests for ltree and intarray |
| Previous Message | Sami Imseih | 2026-05-15 00:59:18 | Re: Track skipped tables during autovacuum and autoanalyze |