pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedCOPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV
8+ messages / 4 participants
[nested] [flat]
* COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV
@ 2022-06-15 12:16 Svante Richter <pgsql-bugs@richter.id>
2022-08-09 20:48 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Svante Richter <pgsql-bugs@richter.id>
2022-08-14 07:00 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Noah Misch <noah@leadboat.com>
0 siblings, 2 replies; 8+ messages in thread
From: Svante Richter @ 2022-06-15 12:16 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Hello!
The documentation for COPY says "To avoid any misinterpretation, a `\.` data value appearing as a lone entry on a line is automatically quoted on output, and on input, if quoted, is not interpreted as the end-of-data marker".
The input part only seems to work when using the COPY FROM CSV command, not \COPY FROM CSV. This is mentioned in a previous message here https://www.postgresql.org/message-id/a89f47e1-f716-4ae3-b882-cab5032a5d66%40manitou-mail.org but not documented.
This means that COPY TO CSV produces data that \COPY FROM CSV cannot read, which I'm assuming should be fixed (or at the very least documented as a serious limitation of \COPY FROM CSV). I found this out by not being able to load a backup of a table that I had exported via COPY TO CSV.
As the above message also mentioned this can be a security risk if using \COPY FROM STDIN CSV with untrusted data (https://www.postgresql.org/message-id/20190128214448.GH26761%40momjian.us says "I think the question is how many people are using CSV/STDIN for insecure data loads?") but I would absolutely expect data produced with COPY TO CSV to be safe to pipe to a \COPY FROM CSV, but this bug makes that unsafe unless I also explicitly set ON_ERROR_STOP=1.
SQL to reproduce:
CREATE TABLE testtable (a TEXT);
INSERT INTO testtable VALUES ('
\.
');
COPY testtable TO '/run/postgresql/test.csv' CSV;
COPY testtable FROM '/run/postgresql/test.csv' CSV; -- This one works
\COPY testtable FROM '/run/postgresql/test.csv' CSV; -- This one does not work
Error message:
ERROR: unterminated CSV quoted field
CONTEXT: COPY testtable, line 1: ""
"
Versions tested:
psql (PostgreSQL) 14.3 (under arch linux)
psql (PostgreSQL) 13.7 (Ubuntu 13.7-0ubuntu0.21.10.1)
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV
2022-06-15 12:16 COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Svante Richter <pgsql-bugs@richter.id>
@ 2022-08-09 20:48 ` Svante Richter <pgsql-bugs@richter.id>
1 sibling, 0 replies; 8+ messages in thread
From: Svante Richter @ 2022-08-09 20:48 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
A gentle bump for this, hopefully that's alright!
On Wed, Jun 15, 2022, at 2:16 PM, Svante Richter wrote:
> Hello!
>
> The documentation for COPY says "To avoid any misinterpretation, a `\.` data value appearing as a lone entry on a line is automatically quoted on output, and on input, if quoted, is not interpreted as the end-of-data marker".
>
> The input part only seems to work when using the COPY FROM CSV command, not \COPY FROM CSV. This is mentioned in a previous message here https://www.postgresql.org/message-id/a89f47e1-f716-4ae3-b882-cab5032a5d66%40manitou-mail.org but not documented.
>
> This means that COPY TO CSV produces data that \COPY FROM CSV cannot read, which I'm assuming should be fixed (or at the very least documented as a serious limitation of \COPY FROM CSV). I found this out by not being able to load a backup of a table that I had exported via COPY TO CSV.
>
> As the above message also mentioned this can be a security risk if using \COPY FROM STDIN CSV with untrusted data (https://www.postgresql.org/message-id/20190128214448.GH26761%40momjian.us says "I think the question is how many people are using CSV/STDIN for insecure data loads?") but I would absolutely expect data produced with COPY TO CSV to be safe to pipe to a \COPY FROM CSV, but this bug makes that unsafe unless I also explicitly set ON_ERROR_STOP=1.
>
> SQL to reproduce:
>
> CREATE TABLE testtable (a TEXT);
> INSERT INTO testtable VALUES ('
> \.
> ');
> COPY testtable TO '/run/postgresql/test.csv' CSV;
> COPY testtable FROM '/run/postgresql/test.csv' CSV; -- This one works
> \COPY testtable FROM '/run/postgresql/test.csv' CSV; -- This one does not work
>
>
> Error message:
>
> ERROR: unterminated CSV quoted field
> CONTEXT: COPY testtable, line 1: ""
> "
>
> Versions tested:
>
> psql (PostgreSQL) 14.3 (under arch linux)
> psql (PostgreSQL) 13.7 (Ubuntu 13.7-0ubuntu0.21.10.1)
>
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV
2022-06-15 12:16 COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Svante Richter <pgsql-bugs@richter.id>
@ 2022-08-14 07:00 ` Noah Misch <noah@leadboat.com>
2022-08-14 14:08 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 8+ messages in thread
From: Noah Misch @ 2022-08-14 07:00 UTC (permalink / raw)
To: Svante Richter <pgsql-bugs@richter.id>; +Cc: pgsql-bugs@lists.postgresql.org
On Wed, Jun 15, 2022 at 02:16:14PM +0200, Svante Richter wrote:
> The documentation for COPY says "To avoid any misinterpretation, a `\.` data value appearing as a lone entry on a line is automatically quoted on output, and on input, if quoted, is not interpreted as the end-of-data marker".
>
> The input part only seems to work when using the COPY FROM CSV command, not \COPY FROM CSV. This is mentioned in a previous message here https://www.postgresql.org/message-id/a89f47e1-f716-4ae3-b882-cab5032a5d66%40manitou-mail.org but not documented.
I agree this warrants a doc mention. Also, the previous thread seems to have
ended with a focus on [\copy ... from stdin], but it's broader than that:
> COPY testtable FROM '/run/postgresql/test.csv' CSV; -- This one works
> \COPY testtable FROM '/run/postgresql/test.csv' CSV; -- This one does not work
The psql documentation says [\copy ... from stdin] looks for "\.". It says
nothing about doing that for [\copy ... from filename]. I wonder if psql
should change the filename case to send the whole file to the server, ignoring
"\." inside. (That is to say, change to match the psql documentation.) This,
too, has an avoidable case of the problem:
create table t (c text);
\copy t from program 'printf ''"foo\n\\.\nbar"''' with csv
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV
2022-06-15 12:16 COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Svante Richter <pgsql-bugs@richter.id>
2022-08-14 07:00 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Noah Misch <noah@leadboat.com>
@ 2022-08-14 14:08 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-08-16 15:26 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Bruce Momjian <bruce@momjian.us>
0 siblings, 1 reply; 8+ messages in thread
From: Tom Lane @ 2022-08-14 14:08 UTC (permalink / raw)
To: Noah Misch <noah@leadboat.com>; +Cc: Svante Richter <pgsql-bugs@richter.id>; pgsql-bugs@lists.postgresql.org
Noah Misch <noah@leadboat.com> writes:
> The psql documentation says [\copy ... from stdin] looks for "\.". It says
> nothing about doing that for [\copy ... from filename]. I wonder if psql
> should change the filename case to send the whole file to the server, ignoring
> "\." inside. (That is to say, change to match the psql documentation.)
This seems like a sensible solution. The need to use an in-band EOF
marker when reading from the command source file is clear, and there's
no non-kluge way there. But that doesn't mean we should extend it
to separate files. (I'm surprised to realize we do, actually.)
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV
2022-06-15 12:16 COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Svante Richter <pgsql-bugs@richter.id>
2022-08-14 07:00 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Noah Misch <noah@leadboat.com>
2022-08-14 14:08 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Tom Lane <tgl@sss.pgh.pa.us>
@ 2022-08-16 15:26 ` Bruce Momjian <bruce@momjian.us>
2022-08-17 05:26 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Noah Misch <noah@leadboat.com>
2023-10-28 15:38 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Bruce Momjian <bruce@momjian.us>
0 siblings, 2 replies; 8+ messages in thread
From: Bruce Momjian @ 2022-08-16 15:26 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Noah Misch <noah@leadboat.com>; Svante Richter <pgsql-bugs@richter.id>; pgsql-bugs@lists.postgresql.org
On Sun, Aug 14, 2022 at 10:08:55AM -0400, Tom Lane wrote:
> Noah Misch <noah@leadboat.com> writes:
> > The psql documentation says [\copy ... from stdin] looks for "\.". It says
> > nothing about doing that for [\copy ... from filename]. I wonder if psql
> > should change the filename case to send the whole file to the server, ignoring
> > "\." inside. (That is to say, change to match the psql documentation.)
>
> This seems like a sensible solution. The need to use an in-band EOF
> marker when reading from the command source file is clear, and there's
> no non-kluge way there. But that doesn't mean we should extend it
> to separate files. (I'm surprised to realize we do, actually.)
So, look what I found in psql/copy.c:
/* check for EOF marker, but not on a partial line */
if (at_line_begin)
{
/*
--> * This code erroneously assumes '\.' on a line alone
--> * inside a quoted CSV string terminates the \copy.
--> * https://www.postgresql.org/message-id/E1TdNVQ-0001ju-GO@wrigleys.postgresql.org
*/
if ((linelen == 3 && memcmp(fgresult, "\\.\n", 3) == 0) ||
(linelen == 4 && memcmp(fgresult, "\\.\r\n", 4) == 0))
{
copydone = true;
}
}
I wondered who would reference an email thread in the source code, so I
looked at the whole 2012 thread:
https://www.postgresql.org/message-id/flat/E1TdNVQ-0001ju-GO%40wrigleys.postgresql.org
and it was, of course, me. :-(
Tom's analysis was similar ten years ago:
> Ugh. This seems like a rather fundamental oversight in the CSV feature.
> The problem is that psql has no idea whether the copy is being done in
> CSV mode or not --- and even if it did, it doesn't parse the data fully
> enough to realize whether a \. line is inside quotes or not.
>
> In the case of out-of-line data files, it might be reasonable to just
> dispense with the check for \. altogether and always ship the whole file
> to the backend; I think there's a \. check on the backend side. (Not
> sure this is safe in V2 protocol, but I doubt anyone cares anymore
> about that.)
>
> In the case of in-line data in a script file, CSV mode seems a bit
> broken in any case; there's no concept of a terminator in CSV, AFAIK.
> So maybe we don't have to worry about that.
I think at this point we should either fix this or document it.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Indecision is a decision. Inaction is an action. Mark Batterson
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV
2022-06-15 12:16 COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Svante Richter <pgsql-bugs@richter.id>
2022-08-14 07:00 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Noah Misch <noah@leadboat.com>
2022-08-14 14:08 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Tom Lane <tgl@sss.pgh.pa.us>
2022-08-16 15:26 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Bruce Momjian <bruce@momjian.us>
@ 2022-08-17 05:26 ` Noah Misch <noah@leadboat.com>
1 sibling, 0 replies; 8+ messages in thread
From: Noah Misch @ 2022-08-17 05:26 UTC (permalink / raw)
To: Bruce Momjian <bruce@momjian.us>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Svante Richter <pgsql-bugs@richter.id>; pgsql-bugs@lists.postgresql.org
On Tue, Aug 16, 2022 at 11:26:47AM -0400, Bruce Momjian wrote:
> On Sun, Aug 14, 2022 at 10:08:55AM -0400, Tom Lane wrote:
> > Noah Misch <noah@leadboat.com> writes:
> > > The psql documentation says [\copy ... from stdin] looks for "\.". It says
> > > nothing about doing that for [\copy ... from filename]. I wonder if psql
> > > should change the filename case to send the whole file to the server, ignoring
> > > "\." inside. (That is to say, change to match the psql documentation.)
> >
> > This seems like a sensible solution. The need to use an in-band EOF
> > marker when reading from the command source file is clear, and there's
> > no non-kluge way there. But that doesn't mean we should extend it
> > to separate files. (I'm surprised to realize we do, actually.)
> Tom's analysis was similar ten years ago:
>
> > Ugh. This seems like a rather fundamental oversight in the CSV feature.
> > The problem is that psql has no idea whether the copy is being done in
> > CSV mode or not --- and even if it did, it doesn't parse the data fully
> > enough to realize whether a \. line is inside quotes or not.
> >
> > In the case of out-of-line data files, it might be reasonable to just
> > dispense with the check for \. altogether and always ship the whole file
> > to the backend; I think there's a \. check on the backend side. (Not
> > sure this is safe in V2 protocol, but I doubt anyone cares anymore
> > about that.)
> I think at this point we should either fix this or document it.
Things have gotten simpler in the last ten years, specifically commit 3174d69
removing protocol v2. Before that, the server would have mishandled "\." even
if the client didn't. These days, one can fix all but [\copy ... from stdin].
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV
2022-06-15 12:16 COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Svante Richter <pgsql-bugs@richter.id>
2022-08-14 07:00 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Noah Misch <noah@leadboat.com>
2022-08-14 14:08 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Tom Lane <tgl@sss.pgh.pa.us>
2022-08-16 15:26 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Bruce Momjian <bruce@momjian.us>
@ 2023-10-28 15:38 ` Bruce Momjian <bruce@momjian.us>
2023-11-03 17:58 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Bruce Momjian <bruce@momjian.us>
1 sibling, 1 reply; 8+ messages in thread
From: Bruce Momjian @ 2023-10-28 15:38 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Noah Misch <noah@leadboat.com>; Svante Richter <pgsql-bugs@richter.id>; pgsql-bugs@lists.postgresql.org
On Tue, Aug 16, 2022 at 11:26:47AM -0400, Bruce Momjian wrote:
> On Sun, Aug 14, 2022 at 10:08:55AM -0400, Tom Lane wrote:
> > Noah Misch <noah@leadboat.com> writes:
> > > The psql documentation says [\copy ... from stdin] looks for "\.". It says
> > > nothing about doing that for [\copy ... from filename]. I wonder if psql
> > > should change the filename case to send the whole file to the server, ignoring
> > > "\." inside. (That is to say, change to match the psql documentation.)
> >
> > This seems like a sensible solution. The need to use an in-band EOF
> > marker when reading from the command source file is clear, and there's
> > no non-kluge way there. But that doesn't mean we should extend it
> > to separate files. (I'm surprised to realize we do, actually.)
>
> So, look what I found in psql/copy.c:
>
> /* check for EOF marker, but not on a partial line */
> if (at_line_begin)
> {
> /*
> --> * This code erroneously assumes '\.' on a line alone
> --> * inside a quoted CSV string terminates the \copy.
> --> * https://www.postgresql.org/message-id/E1TdNVQ-0001ju-GO@wrigleys.postgresql.org
> */
> if ((linelen == 3 && memcmp(fgresult, "\\.\n", 3) == 0) ||
> (linelen == 4 && memcmp(fgresult, "\\.\r\n", 4) == 0))
> {
> copydone = true;
> }
> }
>
> I wondered who would reference an email thread in the source code, so I
> looked at the whole 2012 thread:
>
> https://www.postgresql.org/message-id/flat/E1TdNVQ-0001ju-GO%40wrigleys.postgresql.org
>
> and it was, of course, me. :-(
>
> Tom's analysis was similar ten years ago:
>
> > Ugh. This seems like a rather fundamental oversight in the CSV feature.
> > The problem is that psql has no idea whether the copy is being done in
> > CSV mode or not --- and even if it did, it doesn't parse the data fully
> > enough to realize whether a \. line is inside quotes or not.
> >
> > In the case of out-of-line data files, it might be reasonable to just
> > dispense with the check for \. altogether and always ship the whole file
> > to the backend; I think there's a \. check on the backend side. (Not
> > sure this is safe in V2 protocol, but I doubt anyone cares anymore
> > about that.)
> >
> > In the case of in-line data in a script file, CSV mode seems a bit
> > broken in any case; there's no concept of a terminator in CSV, AFAIK.
> > So maybe we don't have to worry about that.
>
> I think at this point we should either fix this or document it.
I have decided to document this with the attached patch. I plan to
apply it to all supported Postgres versions. If we ever fix FROM file
so only FROM STDIN is a problem, we can update the documentation.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
Attachments:
copy_csv.diff (text/x-diff, 1.4K) download
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index d94e3cacfc..cc7d797159 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1119,6 +1119,10 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
destination, because all data must pass through the client/server
connection. For large amounts of data the <acronym>SQL</acronym>
command might be preferable.
+ Also, because of this pass-through method, <literal>\copy
+ ... from</literal> in <acronym>CSV</acronym> mode will erroneously
+ treat a <literal>\.</literal> data value alone on a line as an
+ end-of-input marker.
</para>
</tip>
diff --git a/src/bin/psql/copy.c b/src/bin/psql/copy.c
index b3cc3d9a29..8d6ce4cedd 100644
--- a/src/bin/psql/copy.c
+++ b/src/bin/psql/copy.c
@@ -627,6 +627,7 @@ handleCopyIn(PGconn *conn, FILE *copystream, bool isbinary, PGresult **res)
* This code erroneously assumes '\.' on a line alone
* inside a quoted CSV string terminates the \copy.
* https://www.postgresql.org/message-id/E1TdNVQ-0001ju-GO@wrigleys.postgresql.org
+ * https://www.postgresql.org/message-id/bfcd57e4-8f23-4c3e-a5db-2571d09208e2@beta.fastmail.com
*/
if ((linelen == 3 && memcmp(fgresult, "\\.\n", 3) == 0) ||
(linelen == 4 && memcmp(fgresult, "\\.\r\n", 4) == 0))
* Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV
2022-06-15 12:16 COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Svante Richter <pgsql-bugs@richter.id>
2022-08-14 07:00 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Noah Misch <noah@leadboat.com>
2022-08-14 14:08 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Tom Lane <tgl@sss.pgh.pa.us>
2022-08-16 15:26 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Bruce Momjian <bruce@momjian.us>
2023-10-28 15:38 ` Re: COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Bruce Momjian <bruce@momjian.us>
@ 2023-11-03 17:58 ` Bruce Momjian <bruce@momjian.us>
0 siblings, 0 replies; 8+ messages in thread
From: Bruce Momjian @ 2023-11-03 17:58 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Noah Misch <noah@leadboat.com>; Svante Richter <pgsql-bugs@richter.id>; pgsql-bugs@lists.postgresql.org
On Sat, Oct 28, 2023 at 11:38:37AM -0400, Bruce Momjian wrote:
> On Tue, Aug 16, 2022 at 11:26:47AM -0400, Bruce Momjian wrote:
> > I think at this point we should either fix this or document it.
>
> I have decided to document this with the attached patch. I plan to
> apply it to all supported Postgres versions. If we ever fix FROM file
> so only FROM STDIN is a problem, we can update the documentation.
Patch applied to all supported versions.
--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2023-11-03 17:58 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-06-15 12:16 COPY TO CSV produces data that is incompatible/unsafe for \COPY FROM CSV Svante Richter <pgsql-bugs@richter.id>
2022-08-09 20:48 ` Svante Richter <pgsql-bugs@richter.id>
2022-08-14 07:00 ` Noah Misch <noah@leadboat.com>
2022-08-14 14:08 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-08-16 15:26 ` Bruce Momjian <bruce@momjian.us>
2022-08-17 05:26 ` Noah Misch <noah@leadboat.com>
2023-10-28 15:38 ` Bruce Momjian <bruce@momjian.us>
2023-11-03 17:58 ` Bruce Momjian <bruce@momjian.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox