ext/pgsql: route pg_copy_from table_name through build_tablename

[iliaal]

Add the table validation added in bug #62978 (which fixed pg_insert, pg_update, pg_select, pg_delete) but missed pg_copy_from. Not updating pg_copy_to since it allows (query) for the table name per ext/pgsql/tests/06_bug73498.phpt.

[devnexen]

Thanks for starting the work, but as hinted. it is preferable you really go through. I ll put some comments.

[devnexen]

I want to see $null_as parameter being challenged too, e.g.

pg_query($db, "DROP TABLE IF EXISTS table1");
  pg_query($db, "DROP TABLE IF EXISTS injected");
  pg_query($db, "CREATE TABLE table1 (v text)");

  $null_as = "X'; CREATE TABLE injected (v text); --";

  pg_copy_from($db, 'victim', ["row\n"], "\t", $null_as);
  
  $r = pg_query($db, "SELECT 1 FROM pg_tables WHERE tablename='injected'");
  var_dump(pg_num_rows($r));  => int(1)

[devnexen]

Add the table validation added in bug #62978 (which fixed pg_insert, pg_update, pg_select, pg_delete) but missed pg_copy_from. Not updating pg_copy_to since it allows (query) for the table name per ext/pgsql/tests/06_bug73498.phpt.

pg_copy_to needs to go through the grinder too.

[iliaal]

Both null_as and the delimiter now go through PQescapeLiteral, dropping the E'...' wrapper. Bare table_name in pg_copy_to routes through build_tablename. The (query) form per bug 73498 still passes through, but wrapped in an extra paren pair so something like (SELECT 1); DROP TABLE x; becomes a syntax error inside the outer parens instead of escaping out into a second statement.

[devnexen]

hmmmm ... can you try a test that looks like this ?

--TEST--
  pg_copy_to() / pg_copy_from() default null marker is "\N"
  --EXTENSIONS--
  pgsql
  --SKIPIF--
  <?php include("inc/skipif.inc"); ?>
  --FILE--
  <?php
  include('inc/config.inc');
  $t = "pg_copy_default_null";

  $db = pg_connect($conn_str);
  pg_query($db, "DROP TABLE IF EXISTS {$t}");
  pg_query($db, "CREATE TABLE {$t} (id int, v text)");
  pg_query($db, "INSERT INTO {$t} VALUES (1, 'hello'), (2, NULL)");

  $rows = pg_copy_to($db, $t);
  var_dump($rows);

  pg_query($db, "DELETE FROM {$t}");
  var_dump(pg_copy_from($db, $t, $rows));
  var_dump(pg_fetch_all(pg_query($db, "SELECT v FROM {$t} ORDER BY id")));
  ?>
  --CLEAN--
  <?php
  include('inc/config.inc');
  $db = pg_connect($conn_str);
  pg_query($db, "DROP TABLE IF EXISTS pg_copy_default_null");
  ?>
  --EXPECT--
  array(2) {
    [0]=>
    string(8) "1        hello
  "
    [1]=>
    string(4) "2        \N
  "
  }
  bool(true)
  array(2) {
    [0]=>
    array(1) {
      ["v"]=>
      string(5) "hello"
    }
    [1]=>
    array(1) {
      ["v"]=>
      NULL
    }
  }

[iliaal]

Ah yes, good catch. Just \\N is enough here.

[devnexen]

other things to think about (no rush, won t be committed today), what happens when query has already parens, your test need stricter output expectations ; i.e. pg_copy_from(): %s ... your new error messages need to be more specific, aka which parameter had failed.

[devnexen]

nit: code style in php-src is new line and braces even with just 1 line block

[iliaal]

Bah 😆 ok something to tweak

[iliaal]

other things to think about (no rush, won t be committed today), what happens when query has already parens, your test need stricter output expectations ; i.e. pg_copy_from(): %s ... your new error messages need to be more specific, aka which parameter had failed.

If already has parens shouldn't be an issue would just be double parens, I tested locally but let me double check and add to test.

[devnexen]

as mentioned, you have the specific information available, e.g. here pg_delimiter.

[iliaal]

Maybe I didn't quite get the ask here, you want the error message to include the delimiter itself, seems of limited value to me, but can be added I suppose

[devnexen]

yes just for clarification's sake.

[devnexen]

or even better...
zend_string *msgbuf = _php_pgsql_trim_message(PQerrorMessage(pgsql));
php_error_docref...
zend_string_release(msgbuf);

[iliaal]

Applied PQerrorMessage across all four errors and added params where needed, also for consistency promoted table escape error from notice to warning

[devnexen]

You covered most of the points I ll have a fresh look in the following days. Cheers !

[devnexen]

(re)using build_tablename narrows what these accept. old code passed table_name through raw via spprintf so callers could tack on any COPY syntax. now anything that isn't a plain identifier or schema.table gets wrapped as one quoted name and fails. Before:

• "mytable (col1, col2)" column list
• "ONLY mytable"
• '"my.table"' quoted name with a dot, memchr splits it

the (query) branch covers column lists but not ONLY or dotted quoted names. either gate build_tablename behind a simple-identifier check and error out, or add
explicit columns/only params.

[iliaal]

Added a strict gate ahead of build_tablename: bare table_name must match [A-Za-z_][A-Za-z0-9_]* or schema.table, anything else (column list, ONLY, quoted, quoted-with-literal-dot) returns false with a warning pointing at the argument. Column lists and ONLY for pg_copy_to remain reachable via the (query) form.

[devnexen]

ditto

[iliaal]

Same gate applied here.

[devnexen]

can you add a case that tries to close the wrapper early, like '(SELECT 1)); DROP TABLE pg_copy_to_qsource; --', plus the pg_tables check?

[iliaal]

Added the wrapper-close test case plus a paren-balance check on the (query) input: must start with (, end with ), and depth must reach 0 only at the final character. The original (SELECT 1); DROP...; -- case also hits the new gate, so its expected output flipped from a Postgres syntax error to the validation warning.