Re: psql -f doesn't complain about directories

Letting psql execute a script file that is really a directory doesn't complain
at all:

$ psql -f /tmp

Should we do some kind of stat() before opening the file and abort if it's a
directory?

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut wrote:
> Letting psql execute a script file that is really a directory doesn't complain
> at all:
>
> $ psql -f /tmp
>
> Should we do some kind of stat() before opening the file and abort if it's a
> directory?

Actually anything other than a plain file, right? (Do we really want to
be able to psql -f a_pipe?)

--
Alvaro Herrera Developer, http://www.PostgreSQL.org/
Essentially, you're proposing Kevlar shoes as a solution for the problem
that you want to walk around carrying a loaded gun aimed at your foot.
(Tom Lane)

Alvaro Herrera wrote:
> Peter Eisentraut wrote:
>
>> Letting psql execute a script file that is really a directory doesn't complain
>> at all:
>>
>> $ psql -f /tmp
>>
>> Should we do some kind of stat() before opening the file and abort if it's a
>> directory?
>>
>
> Actually anything other than a plain file, right? (Do we really want to
> be able to psql -f a_pipe?)
>

I don't see why not.

cheers

andrew

On Wed, Nov 14, 2007 at 05:15:20PM -0300, Alvaro Herrera wrote:
> > Should we do some kind of stat() before opening the file and abort if it's a
> > directory?
>
> Actually anything other than a plain file, right? (Do we really want to
> be able to psql -f a_pipe?)

Sure, why not. To be honest I think that psql shouldn't be ignoring the
EISDIR error the kernel is returning.

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Those who make peaceful revolution impossible will make violent revolution inevitable.
> -- John F Kennedy

Martijn van Oosterhout wrote:
> On Wed, Nov 14, 2007 at 05:15:20PM -0300, Alvaro Herrera wrote:
>>> Should we do some kind of stat() before opening the file and abort if it's a
>>> directory?
>> Actually anything other than a plain file, right? (Do we really want to
>> be able to psql -f a_pipe?)
>
> Sure, why not. To be honest I think that psql shouldn't be ignoring the
> EISDIR error the kernel is returning.

But it works when you open directory in read-only mode. See posix
definition:

[EISDIR]
The named file is a directory and oflag includes O_WRONLY or O_RDWR.

Zdenek

Alvaro Herrera wrote:
> Peter Eisentraut wrote:
>> Letting psql execute a script file that is really a directory doesn't complain
>> at all:
>>
>> $ psql -f /tmp
>>
>> Should we do some kind of stat() before opening the file and abort if it's a
>> directory?
>
> Actually anything other than a plain file, right? (Do we really want to
> be able to psql -f a_pipe?)
>

What's about symlink to regular file/pipe?

Zdenek

On Wed, Nov 14, 2007 at 09:33:17PM +0100, Zdenek Kotala wrote:
> >Sure, why not. To be honest I think that psql shouldn't be ignoring the
> >EISDIR error the kernel is returning.
>
> But it works when you open directory in read-only mode. See posix
> definition:
>
> [EISDIR]
> The named file is a directory and oflag includes O_WRONLY or O_RDWR.

$ strace psql -f /tmp
<snip>
open("/tmp", O_RDONLY|O_LARGEFILE) = 4
<snip>
read(4, 0xb7f1b000, 4096) = -1 EISDIR (Is a directory)

Which is subsequently ignored. I'm hoping it doesn't ignore other
errors, like EIO or EPIPE,

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Those who make peaceful revolution impossible will make violent revolution inevitable.
> -- John F Kennedy

Martijn van Oosterhout wrote:
> To be honest I think that psql shouldn't be ignoring the
> EISDIR error the kernel is returning.

We use fopen(), which doesn't appear to pass that on.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

On Wed, Nov 14, 2007 at 10:25:23PM +0100, Peter Eisentraut wrote:
> Martijn van Oosterhout wrote:
> > To be honest I think that psql shouldn't be ignoring the
> > EISDIR error the kernel is returning.
>
> We use fopen(), which doesn't appear to pass that on.

It's not the fopen that fails, it's the fgets that returns NULL. We
don't subsequently check if that's due to an I/O error or EISDIR or if
it's an end-of-file.

Have a nice day,
--
Martijn van Oosterhout <kleptog(at)svana(dot)org> http://svana.org/kleptog/
> Those who make peaceful revolution impossible will make violent revolution inevitable.
> -- John F Kennedy

On Wed, Nov 14, 2007 at 05:15:20PM -0300, Alvaro Herrera wrote:
> Peter Eisentraut wrote:
> > Letting psql execute a script file that is really a directory
> > doesn't complain at all:
> >
> > $ psql -f /tmp
> >
> > Should we do some kind of stat() before opening the file and abort
> > if it's a directory?
>
> Actually anything other than a plain file, right? (Do we really
> want to be able to psql -f a_pipe?)

Yes, I have seen people use just this technique.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter wrote:
> On Wed, Nov 14, 2007 at 05:15:20PM -0300, Alvaro Herrera wrote:
> > Peter Eisentraut wrote:
> > > Letting psql execute a script file that is really a directory
> > > doesn't complain at all:
> > >
> > > $ psql -f /tmp
> > >
> > > Should we do some kind of stat() before opening the file and abort
> > > if it's a directory?
> >
> > Actually anything other than a plain file, right? (Do we really
> > want to be able to psql -f a_pipe?)
>
> Yes, I have seen people use just this technique.

Interesting. Why not just use a standard shell pipe from the command
writing into the named pipe, instead of piping through it?

--
Alvaro Herrera http://www.flickr.com/photos/alvherre/
"Having your biases confirmed independently is how scientific progress is
made, and hence made our great society what it is today" (Mary Gardiner)

Martijn van Oosterhout napsal(a):
> On Wed, Nov 14, 2007 at 09:33:17PM +0100, Zdenek Kotala wrote:
>>> Sure, why not. To be honest I think that psql shouldn't be ignoring the
>>> EISDIR error the kernel is returning.
>> But it works when you open directory in read-only mode. See posix
>> definition:
>>
>> [EISDIR]
>> The named file is a directory and oflag includes O_WRONLY or O_RDWR.
>
> $ strace psql -f /tmp
> <snip>
> open("/tmp", O_RDONLY|O_LARGEFILE) = 4
> <snip>
> read(4, 0xb7f1b000, 4096) = -1 EISDIR (Is a directory)
>
> Which is subsequently ignored. I'm hoping it doesn't ignore other
> errors, like EIO or EPIPE,

Yes, you have right I checked only open command which works fine, but read fails.

Zdenek

Am Mittwoch, 14. November 2007 schrieb Martijn van Oosterhout:
> It's not the fopen that fails, it's the fgets that returns NULL. We
> don't subsequently check if that's due to an I/O error or EISDIR or if
> it's an end-of-file.

Here is a patch for this.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Am Mittwoch, 14. November 2007 schrieb Martijn van Oosterhout:
>> It's not the fopen that fails, it's the fgets that returns NULL. We
>> don't subsequently check if that's due to an I/O error or EISDIR or if
>> it's an end-of-file.

> Here is a patch for this.

This seems too far removed from the scene of the crime --- I don't have
a lot of confidence that errno will still be unchanged back in the main
loop. I'd rather see the psql_error printout occur immediately after
the failed fgets call. Either that or you need to be a bit more
proactive about ensuring errno is returned undamaged.

Also, I think you overlooked the case where we get a read error after
having already loaded some data into gets_fromFile's result buffer.

regards, tom lane

Am Donnerstag, 15. November 2007 schrieb Tom Lane:
> This seems too far removed from the scene of the crime

Yeah, my zeroth attempt was to place this in gets_fromFile(), but there you
don't have any opportunity to report failure to the main loop. We'd need to
change the function signature to be able to pass that around. Maybe that's
better overall.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Am Donnerstag, 15. November 2007 schrieb Tom Lane:
>> This seems too far removed from the scene of the crime

> Yeah, my zeroth attempt was to place this in gets_fromFile(), but there you
> don't have any opportunity to report failure to the main loop. We'd need to
> change the function signature to be able to pass that around. Maybe that's
> better overall.

Well, you could still handle that the same as in your patch: on NULL
return, check ferror. It's just that I don't trust errno to stay
unchanged for very long.

regards, tom lane

Am Donnerstag, 15. November 2007 schrieb Tom Lane:
> Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> > Am Donnerstag, 15. November 2007 schrieb Tom Lane:
> >> This seems too far removed from the scene of the crime
> >
> > Yeah, my zeroth attempt was to place this in gets_fromFile(), but there
> > you don't have any opportunity to report failure to the main loop. We'd
> > need to change the function signature to be able to pass that around.
> > Maybe that's better overall.
>
> Well, you could still handle that the same as in your patch: on NULL
> return, check ferror. It's just that I don't trust errno to stay
> unchanged for very long.

This should do better:

diff -ur ../cvs-pgsql/src/bin/psql/input.c ./src/bin/psql/input.c
--- ../cvs-pgsql/src/bin/psql/input.c 2007-01-12 10:22:42.000000000 +0100
+++ ./src/bin/psql/input.c 2007-11-27 18:46:34.000000000 +0100
@@ -179,9 +179,16 @@
/* Disable SIGINT again */
sigint_interrupt_enabled = false;

- /* EOF? */
+ /* EOF or error? */
if (result == NULL)
+ {
+ if (ferror(source))
+ {
+ psql_error("could not read from input file: %s\n", strerror(errno));
+ return NULL;
+ }
break;
+ }

appendPQExpBufferStr(buffer, line);

diff -ur ../cvs-pgsql/src/bin/psql/mainloop.c ./src/bin/psql/mainloop.c
--- ../cvs-pgsql/src/bin/psql/mainloop.c 2007-01-12 10:22:42.000000000 +0100
+++ ./src/bin/psql/mainloop.c 2007-11-27 18:30:13.000000000 +0100
@@ -129,7 +129,11 @@
line = gets_interactive(get_prompt(prompt_status));
}
else
+ {
line = gets_fromFile(source);
+ if (!line && ferror(source))
+ successResult = EXIT_FAILURE;
+ }

/*
* query_buf holds query already accumulated. line is the malloc'd

--
Peter Eisentraut
http://developer.postgresql.org/~petere/

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> This should do better:

Looks good to me, though I'd suggest updating gets_fromFile's header comment:

- * The result is a malloc'd string.
+ * The result is a malloc'd string, or NULL on EOF or input error.

regards, tom lane