Thread: Re: [BUGS] COPY when 'filename' is a directory
[2002-01-16 17:47] Douglas Trainor said: | I'd like to report a bug in 7.1.3 psql (at least on a 64-bit Alpha Linux box). | Maybe someone can confirm this or maybe it's fixed in 7.2 The attached patch is for 7.2, but might apply (with some fuzz) to 7.1.x. files: src/backend/commands/copy.c src/bin/psql/copy.c note: Add fstat / S_ISDIR checks to make sure we're not trying to use a directory for COPY TO/FROM. cheers. brent -- "Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing." -- Duane Allman
Attachment
[2002-01-17 07:08] Brent Verner said: | [2002-01-16 17:47] Douglas Trainor said: | | I'd like to report a bug in 7.1.3 psql (at least on a 64-bit Alpha Linux box). | | Maybe someone can confirm this or maybe it's fixed in 7.2 | | The attached patch is for 7.2, but might apply (with some fuzz) to 7.1.x. | | files: | src/backend/commands/copy.c | src/bin/psql/copy.c | | note: | Add fstat / S_ISDIR checks to make sure we're not trying to use a directory | for COPY TO/FROM. ...too early in the AM. The proper/complete patch is attached this time. b -- "Develop your talent, man, and leave the world something. Records are really gifts from people. To think that an artist would love you enough to share his music with anyone is a beautiful thing." -- Duane Allman
Attachment
This has been saved for the 7.3 release:
http://candle.pha.pa.us/cgi-bin/pgpatches2
---------------------------------------------------------------------------
Brent Verner wrote:
> [2002-01-17 07:08] Brent Verner said:
> | [2002-01-16 17:47] Douglas Trainor said:
> | | I'd like to report a bug in 7.1.3 psql (at least on a 64-bit Alpha Linux box).
> | | Maybe someone can confirm this or maybe it's fixed in 7.2
> |
> | The attached patch is for 7.2, but might apply (with some fuzz) to 7.1.x.
> |
> | files:
> | src/backend/commands/copy.c
> | src/bin/psql/copy.c
> |
> | note:
> | Add fstat / S_ISDIR checks to make sure we're not trying to use a directory
> | for COPY TO/FROM.
>
> ...too early in the AM. The proper/complete patch is attached this
> time.
>
> b
>
> --
> "Develop your talent, man, and leave the world something. Records are
> really gifts from people. To think that an artist would love you enough
> to share his music with anyone is a beautiful thing." -- Duane Allman
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Your patch has been added to the PostgreSQL unapplied patches list at:
http://candle.pha.pa.us/cgi-bin/pgpatches
I will try to apply it within the next 48 hours.
---------------------------------------------------------------------------
Brent Verner wrote:
> [2002-01-17 07:08] Brent Verner said:
> | [2002-01-16 17:47] Douglas Trainor said:
> | | I'd like to report a bug in 7.1.3 psql (at least on a 64-bit Alpha Linux box).
> | | Maybe someone can confirm this or maybe it's fixed in 7.2
> |
> | The attached patch is for 7.2, but might apply (with some fuzz) to 7.1.x.
> |
> | files:
> | src/backend/commands/copy.c
> | src/bin/psql/copy.c
> |
> | note:
> | Add fstat / S_ISDIR checks to make sure we're not trying to use a directory
> | for COPY TO/FROM.
>
> ...too early in the AM. The proper/complete patch is attached this
> time.
>
> b
>
> --
> "Develop your talent, man, and leave the world something. Records are
> really gifts from people. To think that an artist would love you enough
> to share his music with anyone is a beautiful thing." -- Duane Allman
[ Attachment, skipping... ]
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo@postgresql.org so that your
> message can get through to the mailing list cleanly
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
OK'ed by Peter. Patch applied. Thanks. --------------------------------------------------------------------------- Brent Verner wrote: > [2002-01-17 07:08] Brent Verner said: > | [2002-01-16 17:47] Douglas Trainor said: > | | I'd like to report a bug in 7.1.3 psql (at least on a 64-bit Alpha Linux box). > | | Maybe someone can confirm this or maybe it's fixed in 7.2 > | > | The attached patch is for 7.2, but might apply (with some fuzz) to 7.1.x. > | > | files: > | src/backend/commands/copy.c > | src/bin/psql/copy.c > | > | note: > | Add fstat / S_ISDIR checks to make sure we're not trying to use a directory > | for COPY TO/FROM. > > ...too early in the AM. The proper/complete patch is attached this > time. > > b -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 Index: src/backend/commands/copy.c =================================================================== RCS file: /var/cvsup/pgsql/src/backend/commands/copy.c,v retrieving revision 1.144 diff -c -r1.144 copy.c *** src/backend/commands/copy.c 4 Dec 2001 21:19:57 -0000 1.144 --- src/backend/commands/copy.c 17 Jan 2002 12:24:17 -0000 *************** *** 326,337 **** } else { fp = AllocateFile(filename, PG_BINARY_R); ! if (fp == NULL) elog(ERROR, "COPY command, running in backend with " "effective uid %d, could not open file '%s' for " "reading. Errno = %s (%d).", (int) geteuid(), filename, strerror(errno), errno); } CopyFrom(rel, binary, oids, fp, delim, null_print); } --- 326,345 ---- } else { + struct stat st; fp = AllocateFile(filename, PG_BINARY_R); ! ! if (fp == NULL) elog(ERROR, "COPY command, running in backend with " "effective uid %d, could not open file '%s' for " "reading. Errno = %s (%d).", (int) geteuid(), filename, strerror(errno), errno); + + fstat(fileno(fp),&st); + if( S_ISDIR(st.st_mode) ){ + fclose(fp); + elog(ERROR,"COPY: %s is a directory.",filename); + } } CopyFrom(rel, binary, oids, fp, delim, null_print); } *************** *** 360,365 **** --- 368,374 ---- else { mode_t oumask; /* Pre-existing umask value */ + struct stat st; /* * Prevent write to relative path ... too easy to shoot *************** *** 378,383 **** --- 387,397 ---- "effective uid %d, could not open file '%s' for " "writing. Errno = %s (%d).", (int) geteuid(), filename, strerror(errno), errno); + fstat(fileno(fp),&st); + if( S_ISDIR(st.st_mode) ){ + fclose(fp); + elog(ERROR,"COPY: %s is a directory.",filename); + } } CopyTo(rel, binary, oids, fp, delim, null_print); } Index: src/bin/psql/copy.c =================================================================== RCS file: /var/cvsup/pgsql/src/bin/psql/copy.c,v retrieving revision 1.19 diff -c -r1.19 copy.c *** src/bin/psql/copy.c 2 Jun 2001 18:25:18 -0000 1.19 --- src/bin/psql/copy.c 17 Jan 2002 12:25:07 -0000 *************** *** 11,16 **** --- 11,17 ---- #include <errno.h> #include <assert.h> #include <signal.h> + #include <sys/stat.h> #ifndef WIN32 #include <unistd.h> /* for isatty */ #else *************** *** 233,238 **** --- 234,240 ---- struct copy_options *options; PGresult *result; bool success; + struct stat st; /* parse options */ options = parse_slash_copy(args); *************** *** 292,298 **** free_copy_options(options); return false; } ! result = PSQLexec(query); switch (PQresultStatus(result)) --- 294,309 ---- free_copy_options(options); return false; } ! /* make sure the specified file is not a directory */ ! fstat(fileno(copystream),&st); ! if( S_ISDIR(st.st_mode) ){ ! fclose(copystream); ! psql_error("%s: cannot COPY TO/FROM a directory\n", ! options->file); ! free_copy_options(options); ! return false; ! } ! result = PSQLexec(query); switch (PQresultStatus(result))
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> {
> + struct stat st;
> fp = AllocateFile(filename, PG_BINARY_R);
> !
> ! if (fp == NULL)
> elog(ERROR, "COPY command, running in backend with "
> "effective uid %d, could not open file '%s' for "
> "reading. Errno = %s (%d).",
> (int) geteuid(), filename, strerror(errno), errno);
> +
> + fstat(fileno(fp),&st);
> + if( S_ISDIR(st.st_mode) ){
> + fclose(fp);
> + elog(ERROR,"COPY: %s is a directory.",filename);
> + }
This coding is WRONG. You do not use fclose() to release a file
opened with AllocateFile.
[ several other instances of same error in patch... ]
regards, tom lane
[2002-02-23 20:27] Tom Lane said:
| Bruce Momjian <pgman@candle.pha.pa.us> writes:
| > {
| > + struct stat st;
| > fp = AllocateFile(filename, PG_BINARY_R);
| > + fclose(fp);
|
| This coding is WRONG. You do not use fclose() to release a file
| opened with AllocateFile.
s/fclose/FreeFile/
hiding in shame,
brent
--
"Develop your talent, man, and leave the world something. Records are
really gifts from people. To think that an artist would love you enough
to share his music with anyone is a beautiful thing." -- Duane Allman
[2002-02-23 20:27] Tom Lane said:
| Bruce Momjian <pgman@candle.pha.pa.us> writes:
| > {
| > + struct stat st;
| > fp = AllocateFile(filename, PG_BINARY_R);
| > + fclose(fp);
|
| This coding is WRONG. You do not use fclose() to release a file
| opened with AllocateFile.
corrected diff attached.
b
--
"Develop your talent, man, and leave the world something. Records are
really gifts from people. To think that an artist would love you enough
to share his music with anyone is a beautiful thing." -- Duane Allman
Attachment
Brent Verner wrote:
> [2002-02-23 20:27] Tom Lane said:
> | Bruce Momjian <pgman@candle.pha.pa.us> writes:
> | > {
> | > + struct stat st;
> | > fp = AllocateFile(filename, PG_BINARY_R);
>
> | > + fclose(fp);
> |
> | This coding is WRONG. You do not use fclose() to release a file
> | opened with AllocateFile.
>
> corrected diff attached.
You are going to feel even more ashamed when you realize you sent a
non-context diff. :-)
I can't apply that. I need diff -c.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Brent Verner <brent@rcfile.org> writes:
> | This coding is WRONG. You do not use fclose() to release a file
> | opened with AllocateFile.
> s/fclose/FreeFile/
Actually, my recommendation is to remove it altogether. The mechanisms
are in place to close allocated files after elog(), so why waste thought
and code space to release them manually?
regards, tom lane
Tom Lane wrote:
> Brent Verner <brent@rcfile.org> writes:
> > | This coding is WRONG. You do not use fclose() to release a file
> > | opened with AllocateFile.
>
> > s/fclose/FreeFile/
>
> Actually, my recommendation is to remove it altogether. The mechanisms
> are in place to close allocated files after elog(), so why waste thought
> and code space to release them manually?
Fix applied. There is a FileFree() just below this in the code:
if (!pipe)
FreeFile(fp);
We don't need the if (!pipe) because this code is in an else of
if(pipe). For clarity, it seems the FreeFile call makes sense.
The psql/copy.c file is fine because it isn't backend code.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Index: src/backend/commands/copy.c
===================================================================
RCS file: /cvsroot/pgsql/src/backend/commands/copy.c,v
retrieving revision 1.146
diff -c -r1.146 copy.c
*** src/backend/commands/copy.c 23 Feb 2002 21:46:02 -0000 1.146
--- src/backend/commands/copy.c 24 Feb 2002 02:28:07 -0000
***************
*** 337,343 ****
fstat(fileno(fp),&st);
if( S_ISDIR(st.st_mode) ){
! fclose(fp);
elog(ERROR,"COPY: %s is a directory.",filename);
}
}
--- 337,343 ----
fstat(fileno(fp),&st);
if( S_ISDIR(st.st_mode) ){
! FreeFile(fp);
elog(ERROR,"COPY: %s is a directory.",filename);
}
}
***************
*** 389,395 ****
(int) geteuid(), filename, strerror(errno), errno);
fstat(fileno(fp),&st);
if( S_ISDIR(st.st_mode) ){
! fclose(fp);
elog(ERROR,"COPY: %s is a directory.",filename);
}
}
--- 389,395 ----
(int) geteuid(), filename, strerror(errno), errno);
fstat(fileno(fp),&st);
if( S_ISDIR(st.st_mode) ){
! FreeFile(fp);
elog(ERROR,"COPY: %s is a directory.",filename);
}
}
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> Actually, my recommendation is to remove it altogether. The mechanisms
>> are in place to close allocated files after elog(), so why waste thought
>> and code space to release them manually?
> Fix applied. There is a FileFree() just below this in the code:
> if (!pipe)
> FreeFile(fp);
> We don't need the if (!pipe) because this code is in an else of
> if(pipe). For clarity, it seems the FreeFile call makes sense.
Huh? That FreeFile is *necessary* because it is not in an elog(ERROR)
path; and by my reading the "if (!pipe)" is needed too.
We do have a fair amount of other code that releases resources just
before doing elog(ERROR); so Brent was just emulating code he saw
elsewhere... but it's a coding habit I think we should move away from.
If the resource in question would be released anyway during error
recovery, then I don't see the value of doing it "by hand"; it just
bloats the backend (and adds potential for error, as in this case).
The only exception I'd make is for the case of releasing a spinlock or
LWLock; it's better to release the lock ASAP so as not to block other
backends longer than necessary.
regards, tom lane
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> >> Actually, my recommendation is to remove it altogether. The mechanisms
> >> are in place to close allocated files after elog(), so why waste thought
> >> and code space to release them manually?
>
> > Fix applied. There is a FileFree() just below this in the code:
> > if (!pipe)
> > FreeFile(fp);
> > We don't need the if (!pipe) because this code is in an else of
^^^^
I should have said "in the _new_ code" above.
> > if(pipe). For clarity, it seems the FreeFile call makes sense.
>
> Huh? That FreeFile is *necessary* because it is not in an elog(ERROR)
> path; and by my reading the "if (!pipe)" is needed too.
Woh, I know we need to keep this code:
if (!pipe)
FreeFile(fp);
I was saying we don't need if (!pipe) in the new FreeFile() we just
added.
> We do have a fair amount of other code that releases resources just
> before doing elog(ERROR); so Brent was just emulating code he saw
> elsewhere... but it's a coding habit I think we should move away from.
> If the resource in question would be released anyway during error
> recovery, then I don't see the value of doing it "by hand"; it just
> bloats the backend (and adds potential for error, as in this case).
> The only exception I'd make is for the case of releasing a spinlock or
> LWLock; it's better to release the lock ASAP so as not to block other
> backends longer than necessary.
Agreed. However, I have never seen it stated that file descriptors are
freeded on elog(ERROR). I certainly didn't know that. If we are going
to remove them, let's do it systematically. Let's state in the
developers FAQ what elog(ERROR) frees automatically (file descriptors,
memory in most contexts, anything else?) and then I can check all the
elog(ERROR) cases and make sure this is done consistently.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I should have said "in the _new_ code" above.
Ah, my mistake. I was looking at the original coding.
> Agreed. However, I have never seen it stated that file descriptors are
> freeded on elog(ERROR). I certainly didn't know that. If we are going
> to remove them, let's do it systematically. Let's state in the
> developers FAQ what elog(ERROR) frees automatically (file descriptors,
> memory in most contexts, anything else?) and then I can check all the
> elog(ERROR) cases and make sure this is done consistently.
As a rule of thumb, *every* transient resource should be freed
automatically on elog(ERROR); if it isn't, what happens if there's an
elog in a called subroutine? If manual releasing of resources in an
error path is actually necessary, that should be regarded as an unsafe
coding practice, IMHO.
There are a small number of places that avoid this problem by not
calling any subroutines at all while holding a resource; the main
example I can think of in 7.2 is spinlocks, which don't have any
auto-release facility because they're not supposed to be held over
any nontrivial span of code anyway. But for most sorts of resources
I'd consider that approach unacceptably fragile.
Again, early release of an LWLock before elog is OK --- it's not
necessary, but there might be a performance justification for it.
But I don't think that argument applies to open files or memory
or other resources used within a single backend. There are or
should be auto-release-on-error mechanisms for all that stuff.
regards, tom lane
PS: The difference between AllocateFile and plain fopen() is exactly
that AllocateFile ensures the file will be closed on elog ...