Re: proposal: psql \setfileref - Mailing list pgsql-hackers

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