Обсуждение: proposal: psql \setfileref

Hi Pavel,

I just tried to apply your patch psql-setfileref-initial.patch (using git apply) to the newest revision of postgres at
thetime (da6c4f6ca88) and it failed to patch startup.c.  Thinking that the patch was for some previous revision, I then
checkedout d062245b5, which was from 2016-08-31, and tried applying the patch from there.  I had the same problem:
 
   $ git apply psql-setfileref-initial.patch   error: patch failed: src/bin/psql/startup.c:106   error:
src/bin/psql/startup.c:patch does not apply
 

However, when I applied the changes to startup.c manually and removed them from the patch, the rest of the patch
appliedwithout a problem.  I don't know if I may have done something wrong in trying to apply the patch, but you may
wantto double check if you need to regenerate your patch from the latest revision so it will apply smoothly for
reviewers.

In the meantime, I haven't had a chance to try out the fileref feature yet but I'll give it a go when I get a chance!

Thanks!
Ryan

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, failed
Spec compliant:           tested, failed
Documentation:            not tested

Contents & Purpose
==================

This patch adds a new type of psql variables: file references, that can be
set using command \setfileref. The value of the named variable is the path
to the referenced file. It allows simple inserting of large data without
necessity of manual escaping or using LO api. Use:
postgres=# create table test (col1 bytea);postgres=# \setfileref a ~/avatar.gifpostgres=# insert into test values(:a);

Content of file is returned as bytea, the text output can be used when
escaping is not required, in this case use convert_from() to obtain a
text output.
postgres=# create table test (col1 text);postgres=# \setfileref a ~/README.txtpostgres=# insert into test
values(convert_from(:a,'utf8'));
 

The content of file reference variables is not persistent in memory.

List of file referenced variable can be listed using \set
postgres=# \set...b = ^'~/README.txt'

Empty file outputs an empty bytea (\x).

Initial Run
===========

The patch applies cleanly to HEAD and doesn't break anything, at least the
regression tests all pass successfully.

Feedback on testing
===============

I see several problems.

1) Error reading referenced file returns the system error and a syntax error
on variable:
postgres=# \setfileref a /etc/sudoerspostgres=# insert into test values (:a);/etc/sudoers: Permission deniedERROR:
syntaxerror at or near ":"LINE 1: insert into t1 values (:a);
 

2) Trying to load a file with size upper than the 1GB limit reports the following
error (size 2254110720 bytes):
postgres=# \setfileref b /home/VMs/ISOs/sol-10-u11-ga-x86-dvd.isopostgres=# insert into test values (:b);INSERT 0
1postgres=#ANALYZE test;postgres=# SELECT relname, relkind, relpages, pg_size_pretty(relpages::bigint*8192) FROM
pg_classWHERE relname='test'; relname | relkind | relpages | pg_size_pretty
---------+---------+----------+----------------test    | r       |        1 | 8192 bytes(1 row)
 
postgres=# select * from test; col1 ------ \x(1 row)

This should not insert an empty bytea but might raise an error instead.

Trying to load smaller file but with bytea escaping total size upper than
the 1GB limit (size 666894336 bytes) reports:
postgres=# \setfileref a /var/ISOs/CentOS-7-x86_64-Minimal-1503-01.isopostgres=# insert into t1 values (1, :a,
'tr');ERROR: out of memoryDETAIL:  Cannot enlarge string buffer containing 0 bytes by 1333788688 more bytes.Time:
1428.657ms (00:01.429)
 

This raise an error as bytea escaping increase content size which is the behavior expected.

3) The path doesn't not allow the use of pipe to system command, which is a secure
behavior, but it is quite easy to perform a DOS by using special files like:
postgres=# \setfileref b /dev/random                                       postgres=# insert into t1 (:b);.

this will never end until we kill the psql session. I think we must also prevent
non regular files to be referenced using S_ISREG.

I think all these three errors can be caught and prevented at variable declaration using some tests on files like:
is readable?is a regular file?is small size enough?

to report an error directly at variable file reference setting.

4) An other problem is that like this this patch will allow anyone to upload into a
column the content of any system file that can be read by postgres system user
and then allow non system user to read its content. For example, connected as
a basic PostgreSQL only user:
testdb=> select current_user; current_user -------------- user1(1 row)testdb=> \setfileref b /etc/passwdtestdb=> insert
intotest values (:b);INSERT 0 1
 

then to read the file:
testdb=> select convert_from(col1, 'utf8') from t1; 

Maybe the referenced files must be limited to some part of the filesystem
or the feature limited to superuser only.

5) There is no documentation at all on this feature. Here a proposal
for inclusion in doc/src/sgml/ref/psql-ref.sgml 
\setfileref name valueSets the internal variable name as a reference to the file pathset as value. To unset a variable,
usethe \unset command.
 
File references are shown as file path prefixed with the ^ characterwhen using the \set command alone.
Valid variable names can contain characters, digits, and underscores.See the section Variables below for details.
Variablenames arecase-sensitive.
 
More detail here about file size, file privilege, etc followingwhat will be decided.


6) I would also like to see \setfileref display all file references variables
defined instead of message "missing required argument". Just like \set and
\pset alone are working. \set can still show the file references variables, that's
not a problem for me.


The new status of this patch is: Waiting on Author

On Mon, Oct 3, 2016 at 3:54 PM, Gilles Darold <gilles@darold.net> wrote:
> 4) An other problem is that like this this patch will allow anyone to upload into a
> column the content of any system file that can be read by postgres system user
> and then allow non system user to read its content.

I thought this was a client-side feature, so that it would let a
client upload any file that the client can read, but not things that
can only be read by the postgres system user.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Le 03/10/2016 à 23:03, Robert Haas a écrit :
> On Mon, Oct 3, 2016 at 3:54 PM, Gilles Darold <gilles@darold.net> wrote:
>> 4) An other problem is that like this this patch will allow anyone to upload into a
>> column the content of any system file that can be read by postgres system user
>> and then allow non system user to read its content.
> I thought this was a client-side feature, so that it would let a
> client upload any file that the client can read, but not things that
> can only be read by the postgres system user.
>

Yes that's right, sorry for the noise, forget this fourth report.

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

Le 03/10/2016 à 23:23, Gilles Darold a écrit :
> Le 03/10/2016 à 23:03, Robert Haas a écrit :
>> On Mon, Oct 3, 2016 at 3:54 PM, Gilles Darold <gilles@darold.net> wrote:
>>> 4) An other problem is that like this this patch will allow anyone to upload into a
>>> column the content of any system file that can be read by postgres system user
>>> and then allow non system user to read its content.
>> I thought this was a client-side feature, so that it would let a
>> client upload any file that the client can read, but not things that
>> can only be read by the postgres system user.
>>
> Yes that's right, sorry for the noise, forget this fourth report.
>

After some more though there is still a security issue here. For a
PostgreSQL user who also have login acces to the server, it is possible
to read any file that the postgres system user can read, especially a
.pgpass or a recovery.conf containing password.


-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

2016-10-04 9:18 GMT+02:00 Gilles Darold <gilles.darold@dalibo.com>:

Le 03/10/2016 à 23:23, Gilles Darold a écrit :
> Le 03/10/2016 à 23:03, Robert Haas a écrit :
>> On Mon, Oct 3, 2016 at 3:54 PM, Gilles Darold <gilles@darold.net> wrote:
>>> 4) An other problem is that like this this patch will allow anyone to upload into a
>>> column the content of any system file that can be read by postgres system user
>>> and then allow non system user to read its content.
>> I thought this was a client-side feature, so that it would let a
>> client upload any file that the client can read, but not things that
>> can only be read by the postgres system user.
>>
> Yes that's right, sorry for the noise, forget this fourth report.
>

After some more though there is still a security issue here. For a
PostgreSQL user who also have login acces to the server, it is possible
to read any file that the postgres system user can read, especially a
.pgpass or a recovery.conf containing password.

This patch doesn't introduce any new server side functionality, so if there is some vulnerability, then it is exists now too.

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

On Sun, Oct  9, 2016 at 06:12:16PM +0200, Pavel Stehule wrote:
> 
> 
> 2016-10-09 17:27 GMT+02:00 Corey Huinker <corey.huinker@gmail.com>:
> 
>             here is new update - some mentioned issues are fixed + regress
>             tests and docs
> 
>        
> 
> 
>      
>     This might tie into some work I'm doing. Is there any way these filerefs
>     could be used as the inline portion of a COPY command?
>     i.e. like this:
> 
>     COPY my_table FROM STDIN
>     :file_ref
>     \.
> 
> 
> 
> 
> I understand, but I am not sure how difficult implementation it is. This part
> (COPY input) doesn't support parametrization - and parametrization can have a
> negative performance impact.

And it would need to be \:file_ref in COPY so real data doesn't trigger
it.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

On 10/9/16 9:54 PM, Bruce Momjian wrote:
>> I understand, but I am not sure how difficult implementation it is. This part
>> > (COPY input) doesn't support parametrization - and parametrization can have a
>> > negative performance impact.
> And it would need to be \:file_ref in COPY so real data doesn't trigger
> it.

ISTM it'd be much saner to just restrict file ref's to only working with 
psql's \COPY, and not server-side COPY.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461

hi

2016-10-04 9:18 GMT+02:00 Gilles Darold <gilles.darold@dalibo.com>:

Le 03/10/2016 à 23:23, Gilles Darold a écrit :
> Le 03/10/2016 à 23:03, Robert Haas a écrit :
>> On Mon, Oct 3, 2016 at 3:54 PM, Gilles Darold <gilles@darold.net> wrote:
>>> 4) An other problem is that like this this patch will allow anyone to upload into a
>>> column the content of any system file that can be read by postgres system user
>>> and then allow non system user to read its content.
>> I thought this was a client-side feature, so that it would let a
>> client upload any file that the client can read, but not things that
>> can only be read by the postgres system user.
>>
> Yes that's right, sorry for the noise, forget this fourth report.
>

After some more though there is still a security issue here. For a
PostgreSQL user who also have login acces to the server, it is possible
to read any file that the postgres system user can read, especially a
.pgpass or a recovery.conf containing password.

here is new update - some mentioned issues are fixed + regress tests and docs

postgres=# \setfileref a testfile.txt
postgres=# \setfileref
a = 'testfile.txt'

postgres=# \setfileref
...
a = ^'testfile.txt'

I think it would be better to prefixed the variable value with the ^ too like in the \set report even if we know by using this command that reported variables are file references.

2) You still allow special file to be used, I understand that this is from the user responsibility but I think it could be a wise precaution. 

postgres=# \setfileref b /dev/random
postgres=# insert into test (:b);

Process need to be killed using SIGTERM.

However if this last point is not critical and should be handle by the user, I think this patch is ready to be reviewed by a committer after fixing the first point.

Regards,

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

Gilles Darold wrote:

>    postgres=# \setfileref b /dev/random
>    postgres=# insert into test (:b);
>
> Process need to be killed using SIGTERM.

This can be fixed by setting sigint_interrupt_enabled to true
before operating on the file. Then ctrl-C would be able to cancel
the command.

See comment in common.c, above the declaration of
sigint_interrupt_enabled:

/*[....]* SIGINT is supposed to abort all long-running psql operations, not only* database queries.  In most places,
thisis accomplished by checking* cancel_pressed during long-running loops.  However, that won't work when* blocked on
userinput (in readline() or fgets()).  In those places, we* set sigint_interrupt_enabled TRUE while blocked,
instructingthe signal* catcher to longjmp through sigint_interrupt_jmp.  We assume readline and* fgets are coded to
handlepossible interruption.  (XXX currently this does* not work on win32, so control-C is less useful there)*/ 


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Le 10/10/2016 à 14:38, Daniel Verite a écrit :
>     Gilles Darold wrote:
>
>>    postgres=# \setfileref b /dev/random
>>    postgres=# insert into test (:b);
>>
>> Process need to be killed using SIGTERM.
> This can be fixed by setting sigint_interrupt_enabled to true
> before operating on the file. Then ctrl-C would be able to cancel
> the command.

If we do not prevent the user to be able to load special files that
would be useful yes.

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

2016-10-10 19:50 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

2016-10-10 15:17 GMT+02:00 Gilles Darold <gilles.darold@dalibo.com>:

Le 10/10/2016 à 14:38, Daniel Verite a écrit :
>       Gilles Darold wrote:
>
>>    postgres=# \setfileref b /dev/random
>>    postgres=# insert into test (:b);
>>
>> Process need to be killed using SIGTERM.
> This can be fixed by setting sigint_interrupt_enabled to true
> before operating on the file. Then ctrl-C would be able to cancel
> the command.

If we do not prevent the user to be able to load special files that
would be useful yes.

I don't think so special files has some sense, just I had not time fix this issue. I will do it early - I hope.

fresh patch - only regular files are allowed

Regards

Pavel

Thanks Pavel.

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

Pavel Stehule <pavel.stehule@gmail.com> writes:
> [ psql-setfileref-2016-10-11.patch ]

I haven't been paying any attention to this thread, but I got around to
looking at it finally.  I follow the idea of wanting to be able to shove
the contents of a file into a query literal, but there isn't much else
that I like about this patch.  In particular:

* I really dislike the notion of keying the behavior to a special type of
psql variable.  psql variables are untyped at the moment, and if we were
going to introduce typing, this wouldn't be what I'd want to use it for.
I really don't want to introduce typing and then invent one-off,
unextensible syntax like '^' prefixes to denote what type a variable is.

Aside from being conceptually a mess, I don't even find it particularly
convenient.  In the shell, if you want to source from a file, you write
"<filename".  You aren't compelled to assign the filename to a variable
and then write "<$filename" ... although you can if that's actually
helpful.

Going by the notion of driving it off syntax not variable type, I'd
suggest that we extend the colon-variablename syntax to indicate
desire to read a file.  :<filename< is one pretty obvious idea.
Maybe we could use :<:variablename< to indicate substituting the
content of a variable as the file name to read.

* I'm a bit queasy about the idea of automatically switching over to
parameterized queries when we have one of these things in the query.
I'm afraid that that will have user-visible consequences, so I would
rather that psql not do that behind the user's back.  Plus, that assumes
a fact not in evidence, namely that you only ever want to insert data
and not code this way.  (If \i were more flexible, that objection would
be moot, but you can't source just part of a query from \i AFAIK.)
There might be something to be said for a psql setting that controls
whether to handle colon-insertions this way, and make it apply to
the existing :'var' syntax as well as the filename syntax.

* I find the subthread about attaching this to COPY to be pretty much of
a red herring.  What would that do that you can't do today with \copy?
        regards, tom lane

Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2016-11-09 22:47 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
>> * I really dislike the notion of keying the behavior to a special type of
>> psql variable.

> still I am thinking so some differencing can be nice, because we can use
> psql file path tab autocomplete.
> Maybe \setfileref can stay - it will set any variable, but the autocomplete
> will be based on file path.

Personally, I'd rather have filename tab completion after ":<", and forget
\setfileref --- I do not think that setting a variable first is going to
be the primary use-case for this.  In fact, I could happily dispense with
the variable case entirely, except that sometimes people will want to
reference file names that don't syntactically fit into the colon syntax
(eg, names with spaces in them).  Even that seems surmountable, if people
are okay with requiring the '<' trailer --- I don't think we need to worry
too much about supporting file names with '<' in them.  (Likewise for
'>', if you feel like :<filename> is a less ugly syntax.)

> What do you thing about following example?

> INSERT INTO tab VALUES(1, :<varname); -- insert text value  -- used text
> escaping
> INSERT INTO tab VALUES(1, :<#varname); -- insert bytea value  -- used bytea
> escaping

Seems a bit arbitrary, and not readily extensible to more datatypes.

I guess an advantage of the parameterized-query approach is that we
wouldn't really have to distinguish different datatypes in the syntax,
because we could use the result of Describe Statement to figure out what
to do.  Maybe that outweighs the concern about magic behavioral changes.

On the other hand, it's also arguable that trying to cater for automatic
handling of raw files as bytea literals is overcomplicating the feature
in support of a very infrequent use-case, and giving up some other
infrequent use-cases to get that.  An example is that if we insist on
doing this through parameterized queries, it will not work for inserting
literals into utility commands.  I don't know which of these things is
more important to have.
        regards, tom lane

    Corey Huinker wrote:

> I am not asking for this feature now, but do you see any barriers to later
> adding x/xq/xb/xbq equivalents for executing a shell command?

For text contents, we already have this (pasted right from the doc):

testdb=> \set content `cat my_file.txt`
testdb=> INSERT INTO my_table VALUES (:'content');

Maybe we might look at why it doesn't work for binary string and fix that?

I can think of three reasons:

- psql use C-style '\0' terminated string implying no nul byte in variables.
That could potentially be fixed by tweaking the data structures and
accessors.

- the backtick operator trims ending '\n' from the result of the command
(like the shell), but we could add a variant that doesn't have this behavior.

- we don't have "interpolate as binary", an operator that will
essentially apply PQescapeByteaConn rather than PQescapeStringConn.

For example, I'd suggest this syntax:

-- slurp a file into a variable, with no translation whatsoever
\set content ``cat my_binary_file``

-- interpolate as binary (backquotes instead of quotes)
INSERT INTO my_table VALUES(:`content`);

If we had something like this, would we still need filerefs? I feel like
the point of filerefs is mainly to work around lack of support for
binary in variables, but maybe supporting the latter directly would
be an easier change to accept.


Best regards,
--
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

On Tue, Nov 15, 2016 at 11:48 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
>> For text contents, we already have this (pasted right from the doc):
>>
>> testdb=> \set content `cat my_file.txt`
>> testdb=> INSERT INTO my_table VALUES (:'content');
>>
>> Maybe we might look at why it doesn't work for binary string and fix that?
>>
>> I can think of three reasons:
>>
>> - psql use C-style '\0' terminated string implying no nul byte in
>> variables.
>> That could potentially be fixed by tweaking the data structures and
>> accessors.
>>
>> - the backtick operator trims ending '\n' from the result of the command
>> (like the shell), but we could add a variant that doesn't have this
>> behavior.
>>
>> - we don't have "interpolate as binary", an operator that will
>> essentially apply PQescapeByteaConn rather than PQescapeStringConn.
>>
>> For example, I'd suggest this syntax:
>>
>> -- slurp a file into a variable, with no translation whatsoever
>> \set content ``cat my_binary_file``
>>
>> -- interpolate as binary (backquotes instead of quotes)
>> INSERT INTO my_table VALUES(:`content`);
>>
>> If we had something like this, would we still need filerefs? I feel like
>> the point of filerefs is mainly to work around lack of support for
>> binary in variables, but maybe supporting the latter directly would
>> be an easier change to accept.
>
> I leaved a concept of fileref - see Tom's objection. I know, so I can hack
> ``, but I would not do it. It is used for shell (system) calls, and these
> functionality can depends on used os. The proposed content commands can be
> extended more, and you are doesn't depends on o.s. Another issue of your
> proposal is hard support for tab complete (file path).

On the other hand, it seems like you've invented a whole new system of
escaping and interpolation that is completely disconnected from the
one we already have.  psql is already full of features that nobody
knows about identified by incomprehensible character combinations.
Daniel's suggestion would at least be more similar to what already
exists.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Pavel Stehule <pavel.stehule@gmail.com> writes:
> The Daniel's proposal has important issues:

> 1. you need to store and hold the content in memory
> 2. you cannot use tab complete - without it this feature lost a usability
> 3. you have to do two steps
> 4. Depends on o.s.

I think you're putting *way* too much emphasis on tab completion of the
filename.  That's a nice-to-have, but it should not cause us to contort
the feature design to get it.

I'm not especially impressed by objections 3 and 4, either.  Point #1
has some merit, but since the wire protocol is going to limit us to
circa 1GB anyway, it doesn't seem fatal.

What I like about Daniel's proposal is that because it adds two separate
new behaviors, it can be used for more things than just "interpolate a
file".  What I don't like is that we're still stuck in the land of textual
interpolation into query strings, which as you noted upthread is not
very user-friendly for long values.  I thought there was some value in
your original idea of having a way to get psql to use extended query mode
with the inserted value being sent as a parameter.  But again, I'd rather
see that decoupled as a separate feature and not tightly tied to the
use-case of interpolating a file.

Maybe using extended mode could be driven off a setting rather than being
identified by syntax?  There doesn't seem to be much reason why you'd want
some of the :-interpolated values in a query to be inlined and others sent
as parameters.  Also, for testing purposes, it'd be mighty nice to have a
way of invoking extended query mode in psql with a clear way to define
which things are sent as parameters.  But I don't want to have to make
separate files to contain the values being sent.
        regards, tom lane