Обсуждение: Patch for checking file parameters to psql before password prompt
Patch for the changes discussed in http://archives.postgresql.org/pgsql-hackers/2010-10/msg00919.php attached (eventually ...) In summary: If the input file (-f) doesn't exist or the ouput or log files (-o and -l) can't be created psql exits before prompting for a password. Regards, Alastair.
Вложения
On Sun, Dec 2, 2012 at 6:37 AM, Alastair Turner <bell@ctrlf5.co.za> wrote: > Patch for the changes discussed in > http://archives.postgresql.org/pgsql-hackers/2010-10/msg00919.php > attached (eventually ...) > > In summary: If the input file (-f) doesn't exist or the ouput or log > files (-o and -l) can't be created psql exits before prompting for a > password. s/ouput/output/ Please add this patch here so we don't lose track of it: https://commitfest.postgresql.org/action/commitfest_view/open -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sun, Dec 2, 2012 at 4:37 AM, Alastair Turner <bell@ctrlf5.co.za> wrote: > Patch for the changes discussed in > http://archives.postgresql.org/pgsql-hackers/2010-10/msg00919.php > attached (eventually ...) > > In summary: If the input file (-f) doesn't exist or the ouput or log > files (-o and -l) can't be created psql exits before prompting for a > password. I assume you meant "-L" instead of "-l" here for specifying psql's log file. I don't think that the inability to write to psql's log file should be treated as a fatal error, especially since it is not treated as such by the current code: $ psql test -L /tmp/not_allowed psql: could not open log file "/tmp/not_allowed": Permission denied [... proceeds to psqlprompt from here ...] and the user (or script) may still usefully perform his work. Whereas with your patch: $ psql test -L /tmp/not_allowed psql: could not open log file "/tmp/not_allowed": Permission denied $ And IMO the same concern applies to the query results file, "-o". Although +1 for the part about having psql exit early if the input filename does not exist, since psql already bails out in this case, and there is nothing else to be done in such case. Josh
Josh, * Josh Kupershmidt (schmiddy@gmail.com) wrote: > I assume you meant "-L" instead of "-l" here for specifying psql's log > file. I don't think that the inability to write to psql's log file > should be treated as a fatal error, especially since it is not treated > as such by the current code: I disagree- if we're being asked to log or to send output somewhere, we should error out if we're unable to do so. Consider doing that from the shell: sfrost@beorn:~$ psql > /bin/qq bash: /bin/qq: Permission denied sfrost@beorn:~$ Thanks, Stephen
Alastair, * Alastair Turner (bell@ctrlf5.co.za) wrote: > Patch for the changes discussed in > http://archives.postgresql.org/pgsql-hackers/2010-10/msg00919.php > attached (eventually ...) > > In summary: If the input file (-f) doesn't exist or the ouput or log > files (-o and -l) can't be created psql exits before prompting for a > password. On a once-over, the patch looks reasonably good. A couple things though: Please be good with the whitespace: Above 'if (options.logfilename)' you introduce an extra \n, while you don't have one between the end of that if() { } block and the next. You also have a whole diff block that's just adding a \n (between "pset.inputfile = oldfilename;" and "return result;"). Reviewing your patches before sending them is a good way to find these things. :) Silly stuff, sure, but since it's your first patch, I figured I'd mention it. :) Also, if you're doing the error-reporting in get_fd_for_process and then every time it's called and returns failure immediately exiting, why not just error-exit from get_fd_for_process directly..? Lastly, I'm not convinced that how you broke up process_file() and process_fd() actually works. Inside the existing process_file(), filename will be updated to '<stdin>' for error reporting when the input in 'stdin', but that's now lost in the new process_file() and process_fd() will always get whatever is in options.action_string, which could be a '-' instead. In reviewing the patch, I was hoping that process_fd() wouldn't actually need to have the filename passed in with the fd, but it does because psql_error() depends on pset.inputfile being set, which has to be done by the code which calls into MainLoop(), which is process_fd() with your patch. Perhaps there's a better way to handle that? Thanks, Stephen
On 12/29/12 3:36 PM, Stephen Frost wrote: > Perhaps there's a better way to handle that? Responding to feedback would be a nice start. This submissions has been dead at "Waiting on Author" for at least 3 months now. Time to give it the "Returned with Feedback" boot and see if it comes around again later. I'll do the kicking myself now. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.com