Thread: Confusing error message with too-large file in pg_basebackup
Version: 9.4.5 Summary: confusing error message for too-large file failure in pg_basebackup Details: 1. PostgreSQL previously core dumped on this system and left behind a 9gb core file, which was never deleted. 2. Attempted to pg_basebackup the server. 3. Got this error message: pg_basebackup: could not get transaction log end position from server: ERROR: archive member "core" too large for tar format This was very confusing to the user, because they weren't requesting tar format, and even setting -Fp got the same error message. I can only hypothesize that tar is used somewhere under the hood. pg_basebackup doesn't need to work under these circumstances, but maybe we could give a less baffling error message? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On Fri, Nov 20, 2015 at 9:37 AM, Josh Berkus <josh@agliodbs.com> wrote: > pg_basebackup: could not get transaction log end position from server: > ERROR: archive member "core" too large for tar format That's a backend-side error. > This was very confusing to the user, because they weren't requesting tar > format, and even setting -Fp got the same error message. I can only > hypothesize that tar is used somewhere under the hood. Exactly, when a base backup is taken through the replication protocol, backend always sends it in tar format for performance reasons. It is then up to pg_basebackup to decide if the output should be untared or not. > pg_basebackup doesn't need to work under these circumstances, but maybe > we could give a less baffling error message? We would need to let the backend know about the output format expected by the caller of BASE_BACKUP by extending the command in the replication protocol. It does not sound like a good idea to me just to make some potential error messages more verbose. -- Michael
Michael Paquier <michael.paquier@gmail.com> writes:
> On Fri, Nov 20, 2015 at 9:37 AM, Josh Berkus <josh@agliodbs.com> wrote:
>> pg_basebackup: could not get transaction log end position from server:
>> ERROR: archive member "core" too large for tar format
> That's a backend-side error.
>> This was very confusing to the user, because they weren't requesting tar
>> format, and even setting -Fp got the same error message. I can only
>> hypothesize that tar is used somewhere under the hood.
> Exactly, when a base backup is taken through the replication protocol,
> backend always sends it in tar format for performance reasons. It is
> then up to pg_basebackup to decide if the output should be untared or
> not.
It's not unreasonable for pg_basebackup to use tar format, because the
size limitation should not be an issue for files that are expected to
be in a data directory. Leftover core dump files are unexpected :-(.
I wonder if we could put some sort of filter into pg_basebackup so
it would skip this sort of thing.
regards, tom lane
On Fri, Nov 20, 2015 at 1:41 PM, Tom Lane wrote: > It's not unreasonable for pg_basebackup to use tar format, because the > size limitation should not be an issue for files that are expected to > be in a data directory. Leftover core dump files are unexpected :-(. > I wonder if we could put some sort of filter into pg_basebackup so > it would skip this sort of thing. We could try to have some filtering with the core file name for most of the main distribution cases, like "core", or "core*", however with kernel.core_pattern it is easy to set up on a given system a custom core file name format. Without having to call "file" through system(), another way would be to have directly a look at the file type, but this looks unmaintainable to me, look for example here in magic/Magdir/ that keeps a reference of that. That's quite interesting. ftp://ftp.astron.com/pub/file/ Now there is actually the possibility to call directly "file" in the base backup code path as well, and filter the result depending on if "core" shows up... -- Michael
Le 20 nov. 2015 6:26 AM, "Michael Paquier" <michael.paquier@gmail.com> a =C3=A9crit : > > On Fri, Nov 20, 2015 at 1:41 PM, Tom Lane wrote: > > It's not unreasonable for pg_basebackup to use tar format, because the > > size limitation should not be an issue for files that are expected to > > be in a data directory. Leftover core dump files are unexpected :-(. > > I wonder if we could put some sort of filter into pg_basebackup so > > it would skip this sort of thing. > > We could try to have some filtering with the core file name for most > of the main distribution cases, like "core", or "core*", however with > kernel.core_pattern it is easy to set up on a given system a custom > core file name format. > > Without having to call "file" through system(), another way would be > to have directly a look at the file type, but this looks > unmaintainable to me, look for example here in magic/Magdir/ that > keeps a reference of that. That's quite interesting. > ftp://ftp.astron.com/pub/file/ > Now there is actually the possibility to call directly "file" in the > base backup code path as well, and filter the result depending on if > "core" shows up... Looking at the file's size is probably a better idea. As far as I know, PostgreSQL doesn't create files bigger than 1GB, except for log files. I'm not sure about this but I guess pg_basebackup doesn't ship log files. So, looking at the size would work.
On Fri, Nov 20, 2015 at 4:39 PM, Guillaume Lelarge wrote: > Looking at the file's size is probably a better idea. But isn't that already what the backend does? > As far as I know, > PostgreSQL doesn't create files bigger than 1GB, except for log files. In most cases where the default is used, yes. Now this depends as well on --with-segsize. > I'm not sure about this but I guess pg_basebackup doesn't ship log files. So, looking at the size would work. It does fetch files from pg_log. We actually had on hackers not so long ago discussions about authorizing some filtering option in pg_basebackup for partially this purpose. -- Michael
Guillaume Lelarge wrote: > Looking at the file's size is probably a better idea. As far as I know, > PostgreSQL doesn't create files bigger than 1GB, except for log files. I'm > not sure about this but I guess pg_basebackup doesn't ship log files. So, > looking at the size would work. Hmm, so we let configure --with-segsize to change the file size. The configure help says that the limit should be "less than your OS' limit on file size". We don't warn them that this could cause backup problems later on. Should we add a blurb about that somewhere? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Le 20 nov. 2015 1:34 PM, "Alvaro Herrera" <alvherre@2ndquadrant.com> a =C3=A9crit : > > Guillaume Lelarge wrote: > > > Looking at the file's size is probably a better idea. As far as I know, > > PostgreSQL doesn't create files bigger than 1GB, except for log files. I'm > > not sure about this but I guess pg_basebackup doesn't ship log files. So, > > looking at the size would work. > > Hmm, so we let configure --with-segsize to change the file size. The > configure help says that the limit should be "less than your OS' limit > on file size". We don't warn them that this could cause backup > problems later on. Should we add a blurb about that somewhere? > If we do, we should already have done so because of the file size limit in the tar format backup done with pg_dump.
Guillaume Lelarge wrote: > Le 20 nov. 2015 1:34 PM, "Alvaro Herrera" <alvherre@2ndquadrant.com> a > écrit : > > Hmm, so we let configure --with-segsize to change the file size. The > > configure help says that the limit should be "less than your OS' limit > > on file size". We don't warn them that this could cause backup > > problems later on. Should we add a blurb about that somewhere? > > If we do, we should already have done so because of the file size limit in > the tar format backup done with pg_dump. Agreed, please do. I cannot lend you my time machine right now, though, because I'm using it to go to a past conference I missed, so please ask someone else. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Guillaume Lelarge wrote:
>> Le 20 nov. 2015 1:34 PM, "Alvaro Herrera" <alvherre@2ndquadrant.com> a
>> écrit :
>>> Hmm, so we let configure --with-segsize to change the file size. The
>>> configure help says that the limit should be "less than your OS' limit
>>> on file size". We don't warn them that this could cause backup
>>> problems later on. Should we add a blurb about that somewhere?
>> If we do, we should already have done so because of the file size limit in
>> the tar format backup done with pg_dump.
> Agreed, please do. I cannot lend you my time machine right now, though,
> because I'm using it to go to a past conference I missed, so please ask
> someone else.
Um ... the segment size has no effect on pg_dump. It is true that you
can't use pg_dump's -Ft format if the text form of a table's data exceeds
8GB or whatever it is, but it matters not whether the table is segmented
internally. I'm not sure if this limitation is well-documented either,
but it's a completely different issue so far as users are concerned.
regards, tom lane
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Guillaume Lelarge wrote:
>> Looking at the file's size is probably a better idea. As far as I know,
>> PostgreSQL doesn't create files bigger than 1GB, except for log files. I'm
>> not sure about this but I guess pg_basebackup doesn't ship log files. So,
>> looking at the size would work.
> Hmm, so we let configure --with-segsize to change the file size. The
> configure help says that the limit should be "less than your OS' limit
> on file size". We don't warn them that this could cause backup
> problems later on. Should we add a blurb about that somewhere?
Actually ... why don't we get rid of the limit? wikipedia's entry on
tar format says
... only 11 octal digits can be stored. This gives a maximum file size
of 8 gigabytes on archived files. To overcome this limitation, star in
2001 introduced a base-256 coding that is indicated by setting the
high-order bit of the leftmost byte of a numeric field. GNU-tar and
BSD-tar followed this idea.
If that extension is as widespread as this suggests, then following it
when we have a file > 8GB seems like a better answer than failing
entirely. If you try to read the dump with an old tar program, old
pg_restore, etc, it might fail ... but are you really worse off than
if you couldn't make the dump at all?
regards, tom lane
On Fri, 20 Nov 2015 15:20:12 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote: > Actually ... why don't we get rid of the limit? wikipedia's entry on > tar format says > > ... only 11 octal digits can be stored. This gives a maximum file size > of 8 gigabytes on archived files. To overcome this limitation, star in > 2001 introduced a base-256 coding that is indicated by setting the > high-order bit of the leftmost byte of a numeric field. GNU-tar and > BSD-tar followed this idea. > > If that extension is as widespread as this suggests, then following it > when we have a file > 8GB seems like a better answer than failing > entirely. If you try to read the dump with an old tar program, old > pg_restore, etc, it might fail ... but are you really worse off than > if you couldn't make the dump at all? > > regards, tom lane +1 -dg -- David Gould daveg@sonic.net If simplicity worked, the world would be overrun with insects.
On 11/20/2015 12:20 PM, Tom Lane wrote: > Actually ... why don't we get rid of the limit? wikipedia's entry on > tar format says We still shouldn't be dumping core files as part of a pg_basebackup... but then, why was the core file even IN the $PGDATA directory, shouldn't it have been in some other dir, like the postgres daemon user's $HOME ? -- john r pierce, recycling bits in santa cruz
John R Pierce <pierce@hogranch.com> writes:
> We still shouldn't be dumping core files as part of a pg_basebackup...
It'd be reasonable to skip 'em if we can identify 'em reliably. I'm
not sure how reliably we can do that though.
> but then, why was the core file even IN the $PGDATA directory, shouldn't
> it have been in some other dir, like the postgres daemon user's $HOME ?
No, that's standard behavior. (I know of no Unix-oid system that dumps
cores into your $HOME by default; it's normally either $PWD or some
reserved directory like /cores.)
regards, tom lane
On 11/20/2015 2:13 PM, Tom Lane wrote: > It'd be reasonable to skip 'em if we can identify 'em reliably. I'm > not sure how reliably we can do that though. aren't they nearly always named 'core' ? -- john r pierce, recycling bits in santa cruz
John R Pierce <pierce@hogranch.com> writes:
> On 11/20/2015 2:13 PM, Tom Lane wrote:
>> It'd be reasonable to skip 'em if we can identify 'em reliably. I'm
>> not sure how reliably we can do that though.
> aren't they nearly always named 'core' ?
No. Modern systems more often call them something like 'core.<pid>'.
What really makes it messy is that the name is user-configurable on
most Linux kernels, see /proc/sys/kernel/core_pattern.
We could probably get away with excluding anything that matches "*core*",
but it wouldn't be bulletproof.
regards, tom lane
I wrote:
> Actually ... why don't we get rid of the limit? wikipedia's entry on
> tar format says
> ... only 11 octal digits can be stored. This gives a maximum file size
> of 8 gigabytes on archived files. To overcome this limitation, star in
> 2001 introduced a base-256 coding that is indicated by setting the
> high-order bit of the leftmost byte of a numeric field. GNU-tar and
> BSD-tar followed this idea.
> If that extension is as widespread as this suggests, then following it
> when we have a file > 8GB seems like a better answer than failing
> entirely. If you try to read the dump with an old tar program, old
> pg_restore, etc, it might fail ... but are you really worse off than
> if you couldn't make the dump at all?
I looked into the GNU tar sources and confirmed that gtar supports this
concept. (It looks from the GNU sources like they've supported it for
a *really long time*, like since the 90's, in which case wikipedia's
credit to "star" for the idea is full of it.)
Hence, I propose something like the attached (WIP, has no doc
corrections). I've done simple testing on the pg_dump/pg_restore code
path, but not on basebackup --- anyone want to test that?
I'm not sure whether we should treat this as a back-patchable bug fix
or a new feature for HEAD only. If we don't back-patch it, there are
in any case several bugs here that we must fix. In particular, the
existing coding in ReceiveTarFile:
size_t filesz = 0;
...
sscanf(&tarhdr[124], "%11o", (unsigned int *) &filesz);
is utterly, absolutely, completely broken; it'll fail grossly on
any 64-bit big-endian hardware. There are other places with misplaced
faith that "unsigned long" is at least as wide as size_t.
Comments?
regards, tom lane
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 1af011e..6120c8f 100644
*** a/src/backend/replication/basebackup.c
--- b/src/backend/replication/basebackup.c
*************** sendDir(char *path, int basepathlen, boo
*** 1132,1144 ****
/*
- * Maximum file size for a tar member: The limit inherent in the
- * format is 2^33-1 bytes (nearly 8 GB). But we don't want to exceed
- * what we can represent in pgoff_t.
- */
- #define MAX_TAR_MEMBER_FILELEN (((int64) 1 << Min(33, sizeof(pgoff_t)*8 - 1)) - 1)
-
- /*
* Given the member, write the TAR header & send the file.
*
* If 'missing_ok' is true, will not throw an error if the file is not found.
--- 1132,1137 ----
*************** sendFile(char *readfilename, char *tarfi
*** 1166,1180 ****
errmsg("could not open file \"%s\": %m", readfilename)));
}
- /*
- * Some compilers will throw a warning knowing this test can never be true
- * because pgoff_t can't exceed the compared maximum on their platform.
- */
- if (statbuf->st_size > MAX_TAR_MEMBER_FILELEN)
- ereport(ERROR,
- (errmsg("archive member \"%s\" too large for tar format",
- tarfilename)));
-
_tarWriteHeader(tarfilename, NULL, statbuf);
while ((cnt = fread(buf, 1, Min(sizeof(buf), statbuf->st_size - len), fp)) > 0)
--- 1159,1164 ----
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 80de882..8c4dffe 100644
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
*************** ReceiveTarFile(PGconn *conn, PGresult *r
*** 781,787 ****
bool in_tarhdr = true;
bool skip_file = false;
size_t tarhdrsz = 0;
! size_t filesz = 0;
#ifdef HAVE_LIBZ
gzFile ztarfile = NULL;
--- 781,787 ----
bool in_tarhdr = true;
bool skip_file = false;
size_t tarhdrsz = 0;
! pgoff_t filesz = 0;
#ifdef HAVE_LIBZ
gzFile ztarfile = NULL;
*************** ReceiveTarFile(PGconn *conn, PGresult *r
*** 1046,1052 ****
skip_file = (strcmp(&tarhdr[0], "recovery.conf") == 0);
! sscanf(&tarhdr[124], "%11o", (unsigned int *) &filesz);
padding = ((filesz + 511) & ~511) - filesz;
filesz += padding;
--- 1046,1052 ----
skip_file = (strcmp(&tarhdr[0], "recovery.conf") == 0);
! filesz = read_tar_number(&tarhdr[124], 12);
padding = ((filesz + 511) & ~511) - filesz;
filesz += padding;
*************** ReceiveAndUnpackTarFile(PGconn *conn, PG
*** 1139,1145 ****
char current_path[MAXPGPATH];
char filename[MAXPGPATH];
const char *mapped_tblspc_path;
! int current_len_left;
int current_padding = 0;
bool basetablespace;
char *copybuf = NULL;
--- 1139,1145 ----
char current_path[MAXPGPATH];
char filename[MAXPGPATH];
const char *mapped_tblspc_path;
! pgoff_t current_len_left = 0;
int current_padding = 0;
bool basetablespace;
char *copybuf = NULL;
*************** ReceiveAndUnpackTarFile(PGconn *conn, PG
*** 1208,1227 ****
}
totaldone += 512;
! if (sscanf(copybuf + 124, "%11o", ¤t_len_left) != 1)
! {
! fprintf(stderr, _("%s: could not parse file size\n"),
! progname);
! disconnect_and_exit(1);
! }
/* Set permissions on the file */
! if (sscanf(©buf[100], "%07o ", &filemode) != 1)
! {
! fprintf(stderr, _("%s: could not parse file mode\n"),
! progname);
! disconnect_and_exit(1);
! }
/*
* All files are padded up to 512 bytes
--- 1208,1217 ----
}
totaldone += 512;
! current_len_left = read_tar_number(©buf[124], 12);
/* Set permissions on the file */
! filemode = read_tar_number(©buf[100], 8);
/*
* All files are padded up to 512 bytes
diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c
index 532eacc..5fff355 100644
*** a/src/bin/pg_dump/pg_backup_tar.c
--- b/src/bin/pg_dump/pg_backup_tar.c
*************** typedef struct
*** 78,90 ****
ArchiveHandle *AH;
} TAR_MEMBER;
- /*
- * Maximum file size for a tar member: The limit inherent in the
- * format is 2^33-1 bytes (nearly 8 GB). But we don't want to exceed
- * what we can represent in pgoff_t.
- */
- #define MAX_TAR_MEMBER_FILELEN (((int64) 1 << Min(33, sizeof(pgoff_t)*8 - 1)) - 1)
-
typedef struct
{
int hasSeek;
--- 78,83 ----
*************** isValidTarHeader(char *header)
*** 1049,1055 ****
int sum;
int chk = tarChecksum(header);
! sscanf(&header[148], "%8o", &sum);
if (sum != chk)
return false;
--- 1042,1048 ----
int sum;
int chk = tarChecksum(header);
! sum = read_tar_number(&header[148], 8);
if (sum != chk)
return false;
*************** _tarAddFile(ArchiveHandle *AH, TAR_MEMBE
*** 1091,1103 ****
strerror(errno));
fseeko(tmp, 0, SEEK_SET);
- /*
- * Some compilers will throw a warning knowing this test can never be true
- * because pgoff_t can't exceed the compared maximum on their platform.
- */
- if (th->fileLen > MAX_TAR_MEMBER_FILELEN)
- exit_horribly(modulename, "archive member too large for tar format\n");
-
_tarWriteHeader(th);
while ((cnt = fread(buf, 1, sizeof(buf), tmp)) > 0)
--- 1084,1089 ----
*************** _tarGetHeader(ArchiveHandle *AH, TAR_MEM
*** 1225,1232 ****
char tag[100];
int sum,
chk;
! size_t len;
! unsigned long ullen;
pgoff_t hPos;
bool gotBlock = false;
--- 1211,1217 ----
char tag[100];
int sum,
chk;
! pgoff_t len;
pgoff_t hPos;
bool gotBlock = false;
*************** _tarGetHeader(ArchiveHandle *AH, TAR_MEM
*** 1249,1255 ****
/* Calc checksum */
chk = tarChecksum(h);
! sscanf(&h[148], "%8o", &sum);
/*
* If the checksum failed, see if it is a null block. If so, silently
--- 1234,1240 ----
/* Calc checksum */
chk = tarChecksum(h);
! sum = read_tar_number(&h[148], 8);
/*
* If the checksum failed, see if it is a null block. If so, silently
*************** _tarGetHeader(ArchiveHandle *AH, TAR_MEM
*** 1273,1280 ****
}
sscanf(&h[0], "%99s", tag);
! sscanf(&h[124], "%12lo", &ullen);
! len = (size_t) ullen;
{
char buf[100];
--- 1258,1264 ----
}
sscanf(&h[0], "%99s", tag);
! len = read_tar_number(&h[124], 12);
{
char buf[100];
*************** _tarWriteHeader(TAR_MEMBER *th)
*** 1307,1313 ****
{
char h[512];
! tarCreateHeader(h, th->targetFile, NULL, th->fileLen, 0600, 04000, 02000, time(NULL));
/* Now write the completed header. */
if (fwrite(h, 1, 512, th->tarFH) != 512)
--- 1291,1298 ----
{
char h[512];
! tarCreateHeader(h, th->targetFile, NULL, th->fileLen,
! 0600, 04000, 02000, time(NULL));
/* Now write the completed header. */
if (fwrite(h, 1, 512, th->tarFH) != 512)
diff --git a/src/include/pgtar.h b/src/include/pgtar.h
index 906db7c..9c94a58 100644
*** a/src/include/pgtar.h
--- b/src/include/pgtar.h
*************** enum tarError
*** 19,23 ****
TAR_SYMLINK_TOO_LONG
};
! extern enum tarError tarCreateHeader(char *h, const char *filename, const char *linktarget, size_t size, mode_t mode,
uid_tuid, gid_t gid, time_t mtime);
extern int tarChecksum(char *header);
--- 19,25 ----
TAR_SYMLINK_TOO_LONG
};
! extern enum tarError tarCreateHeader(char *h, const char *filename, const char *linktarget,
! pgoff_t size, mode_t mode, uid_t uid, gid_t gid, time_t mtime);
! extern uint64 read_tar_number(const char *s, int len);
extern int tarChecksum(char *header);
diff --git a/src/port/tar.c b/src/port/tar.c
index 72fd4e1..631ff4d 100644
*** a/src/port/tar.c
--- b/src/port/tar.c
***************
*** 3,23 ****
#include <sys/stat.h>
/*
! * Utility routine to print possibly larger than 32 bit integers in a
! * portable fashion. Filled with zeros.
*/
static void
! print_val(char *s, uint64 val, unsigned int base, size_t len)
{
! int i;
!
! for (i = len; i > 0; i--)
{
! int digit = val % base;
! s[i - 1] = '0' + digit;
! val = val / base;
}
}
--- 3,82 ----
#include <sys/stat.h>
/*
! * Print a numeric field in a tar header. The field starts at *s and is of
! * length len; val is the value to be written.
! *
! * Per POSIX, the way to write a number is in octal with leading zeroes and
! * one trailing space (or NUL, but we use space) at the end of the specified
! * field width.
! *
! * However, the given value may not fit in the available space in octal form.
! * If that's true, we use the GNU extension of writing \200 followed by the
! * number in base-256 form (ie, stored in binary MSB-first). (Note: here we
! * support only non-negative numbers, so we don't worry about the GNU rules
! * for handling negative numbers.)
*/
static void
! print_tar_number(char *s, int len, uint64 val)
{
! if (val < (((uint64) 1) << ((len - 1) * 3)))
{
! /* Use octal with trailing space */
! s[--len] = ' ';
! while (len)
! {
! s[--len] = (val & 7) + '0';
! val >>= 3;
! }
! }
! else
! {
! /* Use base-256 with leading \200 */
! s[0] = '\200';
! while (len > 1)
! {
! s[--len] = (val & 255);
! val >>= 8;
! }
! }
! }
!
! /*
! * Read a numeric field in a tar header. The field starts at *s and is of
! * length len.
! *
! * The POSIX-approved format for a number is octal, ending with a space or
! * NUL. However, for values that don't fit, we recognize the GNU extension
! * of \200 followed by the number in base-256 form (ie, stored in binary
! * MSB-first). (Note: here we support only non-negative numbers, so we don't
! * worry about the GNU rules for handling negative numbers.)
! */
! uint64
! read_tar_number(const char *s, int len)
! {
! uint64 result = 0;
!
! if (*s == '\200')
! {
! /* base-256 */
! while (--len)
! {
! result <<= 8;
! result |= (unsigned char) (*++s);
! }
}
+ else
+ {
+ /* octal */
+ while (len-- && *s >= '0' && *s <= '7')
+ {
+ result <<= 3;
+ result |= (*s - '0');
+ s++;
+ }
+ }
+ return result;
}
*************** tarChecksum(char *header)
*** 46,57 ****
/*
* Fill in the buffer pointed to by h with a tar format header. This buffer
! * must always have space for 512 characters, which is a requirement by
* the tar format.
*/
enum tarError
tarCreateHeader(char *h, const char *filename, const char *linktarget,
! size_t size, mode_t mode, uid_t uid, gid_t gid, time_t mtime)
{
if (strlen(filename) > 99)
return TAR_NAME_TOO_LONG;
--- 105,116 ----
/*
* Fill in the buffer pointed to by h with a tar format header. This buffer
! * must always have space for 512 characters, which is a requirement of
* the tar format.
*/
enum tarError
tarCreateHeader(char *h, const char *filename, const char *linktarget,
! pgoff_t size, mode_t mode, uid_t uid, gid_t gid, time_t mtime)
{
if (strlen(filename) > 99)
return TAR_NAME_TOO_LONG;
*************** tarCreateHeader(char *h, const char *fil
*** 84,107 ****
}
/* Mode 8 - this doesn't include the file type bits (S_IFMT) */
! sprintf(&h[100], "%07o ", (int) (mode & 07777));
/* User ID 8 */
! sprintf(&h[108], "%07o ", (int) uid);
/* Group 8 */
! sprintf(&h[116], "%07o ", (int) gid);
! /* File size 12 - 11 digits, 1 space; use print_val for 64 bit support */
if (linktarget != NULL || S_ISDIR(mode))
/* Symbolic link or directory has size zero */
! print_val(&h[124], 0, 8, 11);
else
! print_val(&h[124], size, 8, 11);
! sprintf(&h[135], " ");
/* Mod Time 12 */
! sprintf(&h[136], "%011o ", (int) mtime);
/* Checksum 8 cannot be calculated until we've filled all other fields */
--- 143,165 ----
}
/* Mode 8 - this doesn't include the file type bits (S_IFMT) */
! print_tar_number(&h[100], 8, (mode & 07777));
/* User ID 8 */
! print_tar_number(&h[108], 8, uid);
/* Group 8 */
! print_tar_number(&h[116], 8, gid);
! /* File size 12 */
if (linktarget != NULL || S_ISDIR(mode))
/* Symbolic link or directory has size zero */
! print_tar_number(&h[124], 12, 0);
else
! print_tar_number(&h[124], 12, size);
/* Mod Time 12 */
! print_tar_number(&h[136], 12, mtime);
/* Checksum 8 cannot be calculated until we've filled all other fields */
*************** tarCreateHeader(char *h, const char *fil
*** 134,152 ****
strlcpy(&h[297], "postgres", 32);
/* Major Dev 8 */
! sprintf(&h[329], "%07o ", 0);
/* Minor Dev 8 */
! sprintf(&h[337], "%07o ", 0);
/* Prefix 155 - not used, leave as nulls */
! /*
! * We mustn't overwrite the next field while inserting the checksum.
! * Fortunately, the checksum can't exceed 6 octal digits, so we just write
! * 6 digits, a space, and a null, which is legal per POSIX.
! */
! sprintf(&h[148], "%06o ", tarChecksum(h));
return TAR_OK;
}
--- 192,206 ----
strlcpy(&h[297], "postgres", 32);
/* Major Dev 8 */
! print_tar_number(&h[329], 8, 0);
/* Minor Dev 8 */
! print_tar_number(&h[337], 8, 0);
/* Prefix 155 - not used, leave as nulls */
! /* Finally, compute and insert the checksum */
! print_tar_number(&h[148], 8, tarChecksum(h));
return TAR_OK;
}
On Fri, 20 Nov 2015 19:11:23 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote: > I'm not sure whether we should treat this as a back-patchable bug fix > or a new feature for HEAD only. If we don't back-patch it, there are > in any case several bugs here that we must fix. In particular, the > existing coding in ReceiveTarFile: > > size_t filesz = 0; > ... > sscanf(&tarhdr[124], "%11o", (unsigned int *) &filesz); > > is utterly, absolutely, completely broken; it'll fail grossly on > any 64-bit big-endian hardware. There are other places with misplaced > faith that "unsigned long" is at least as wide as size_t. > > Comments? My vote would be that it should go in 9.5. If it gets back patched then some dumps produced by 9.4.x would not be readable by 9.4.x-1. But no 9.5.x dump is broken by changing it now. -dg -- David Gould 510 282 0869 daveg@sonic.net If simplicity worked, the world would be overrun with insects.
David Gould <daveg@sonic.net> writes:
> My vote would be that it should go in 9.5. If it gets back patched then
> some dumps produced by 9.4.x would not be readable by 9.4.x-1. But no 9.5.x
> dump is broken by changing it now.
The thing is, though, that without the patch 9.4.x would have failed to
produce such a dump at all. Is that really a better outcome? At least
if you have the dump, you have the option to update so you can read it.
With no dump at all, you might be screwed at a most inopportune moment.
(Think "automatic dump script was failing and nobody noticed" ...)
regards, tom lane
On Sat, Nov 21, 2015 at 7:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > John R Pierce <pierce@hogranch.com> writes: >> On 11/20/2015 2:13 PM, Tom Lane wrote: >>> It'd be reasonable to skip 'em if we can identify 'em reliably. I'm >>> not sure how reliably we can do that though. > >> aren't they nearly always named 'core' ? > > No. Modern systems more often call them something like 'core.<pid>'. > What really makes it messy is that the name is user-configurable on > most Linux kernels, see /proc/sys/kernel/core_pattern. > > We could probably get away with excluding anything that matches "*core*", > but it wouldn't be bulletproof. It does not look like a good idea to me. I have no doubts that there are deployments including configuration files with such abbreviations in PGDATA. -- Michael
On Sat, 21 Nov 2015 14:16:56 +0900 Michael Paquier <michael.paquier@gmail.com> wrote: > On Sat, Nov 21, 2015 at 7:34 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > John R Pierce <pierce@hogranch.com> writes: > >> On 11/20/2015 2:13 PM, Tom Lane wrote: > >>> It'd be reasonable to skip 'em if we can identify 'em reliably. I'm > >>> not sure how reliably we can do that though. > > > >> aren't they nearly always named 'core' ? > > > > No. Modern systems more often call them something like 'core.<pid>'. > > What really makes it messy is that the name is user-configurable on > > most Linux kernels, see /proc/sys/kernel/core_pattern. > > > > We could probably get away with excluding anything that matches "*core*", > > but it wouldn't be bulletproof. > > It does not look like a good idea to me. I have no doubts that there > are deployments including configuration files with such abbreviations > in PGDATA. Perhaps matching *core* and size > 100MB or so would cover that. -dg -- David Gould 510 282 0869 daveg@sonic.net If simplicity worked, the world would be overrun with insects.
I wrote:
> I looked into the GNU tar sources and confirmed that gtar supports this
> concept. (It looks from the GNU sources like they've supported it for
> a *really long time*, like since the 90's, in which case wikipedia's
> credit to "star" for the idea is full of it.)
I've now also verified that the "tar" present on OS X, which is bsdtar,
handles the GNU base-256 convention just fine, and has done so at least
since Snow Leopard (released 2009).
> Hence, I propose something like the attached (WIP, has no doc
> corrections). I've done simple testing on the pg_dump/pg_restore code
> path, but not on basebackup --- anyone want to test that?
The attached updated patch fixes the pg_dump docs (pg_basebackup doesn't
address the point, so nothing to fix there), and just for cleanliness'
sake, gets rid of all use of sprintf and sscanf to process tar headers.
I've now tested the basebackup path, and in addition done testing on a
32-bit machine, which caused me to realize that the existing code is
even more broken than I thought. Quite aside from the documented 8GB
limitation, we have these issues:
* tarCreateHeader's file-size argument is size_t, which means that on
32-bit machines it will write a corrupt tar header for file sizes between
4GB and 8GB, even though no error is raised (if pgoff_t is 64 bits, which
it will be on just about any remotely modern platform). This is very
very bad: you could have an unreadable backup and not know it. We
evidently broke this in 9.3 while refactoring the tar-writing logic so
basebackup could use it.
* pg_restore fails on tables of size between 4GB and 8GB on machines
where either size_t or unsigned long is 32 bits, even if the archive was
written by a 64-bit machine and hence is not corrupted by the previous
bug. If you use Windows 64 you don't even need to transport the archive
to a different machine to get bitten by this. This bug goes back a very
long way.
* pg_basebackup will fail if there are files of size between 4GB and 8GB,
even on 64-bit machines, because it tells sscanf to read the file size
as "unsigned int".
* As I noted yesterday, pg_basebackup fails entirely, for any file size,
on 64-bit big-endian machines.
These all seem to me to be must-fix issues even if we didn't want to
back-port use of the base-256 convention. Seeing that we have to deal
with these things, I'm inclined to just back-port the whole patch to
all branches.
regards, tom lane
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 9d84f8b..25b3d96 100644
*** a/doc/src/sgml/ref/pg_dump.sgml
--- b/doc/src/sgml/ref/pg_dump.sgml
*************** PostgreSQL documentation
*** 272,283 ****
<listitem>
<para>
Output a <command>tar</command>-format archive suitable for input
! into <application>pg_restore</application>. The tar-format is
! compatible with the directory-format; extracting a tar-format
archive produces a valid directory-format archive.
! However, the tar-format does not support compression and has a
! limit of 8 GB on the size of individual tables. Also, the relative
! order of table data items cannot be changed during restore.
</para>
</listitem>
</varlistentry>
--- 272,283 ----
<listitem>
<para>
Output a <command>tar</command>-format archive suitable for input
! into <application>pg_restore</application>. The tar format is
! compatible with the directory format: extracting a tar-format
archive produces a valid directory-format archive.
! However, the tar format does not support compression. Also, when
! using tar format the relative order of table data items cannot be
! changed during restore.
</para>
</listitem>
</varlistentry>
*************** CREATE DATABASE foo WITH TEMPLATE templa
*** 1141,1155 ****
</para>
<para>
- Members of tar archives are limited to a size less than 8 GB.
- (This is an inherent limitation of the tar file format.) Therefore
- this format cannot be used if the textual representation of any one table
- exceeds that size. The total size of a tar archive and any of the
- other output formats is not limited, except possibly by the
- operating system.
- </para>
-
- <para>
The dump file produced by <application>pg_dump</application>
does not contain the statistics used by the optimizer to make
query planning decisions. Therefore, it is wise to run
--- 1141,1146 ----
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index 1af011e..6120c8f 100644
*** a/src/backend/replication/basebackup.c
--- b/src/backend/replication/basebackup.c
*************** SendBackupHeader(List *tablespaces)
*** 698,704 ****
}
else
{
! Size len;
len = strlen(ti->oid);
pq_sendint(&buf, len, 4);
--- 698,704 ----
}
else
{
! Size len;
len = strlen(ti->oid);
pq_sendint(&buf, len, 4);
*************** sendDir(char *path, int basepathlen, boo
*** 1132,1144 ****
/*
- * Maximum file size for a tar member: The limit inherent in the
- * format is 2^33-1 bytes (nearly 8 GB). But we don't want to exceed
- * what we can represent in pgoff_t.
- */
- #define MAX_TAR_MEMBER_FILELEN (((int64) 1 << Min(33, sizeof(pgoff_t)*8 - 1)) - 1)
-
- /*
* Given the member, write the TAR header & send the file.
*
* If 'missing_ok' is true, will not throw an error if the file is not found.
--- 1132,1137 ----
*************** sendFile(char *readfilename, char *tarfi
*** 1166,1180 ****
errmsg("could not open file \"%s\": %m", readfilename)));
}
- /*
- * Some compilers will throw a warning knowing this test can never be true
- * because pgoff_t can't exceed the compared maximum on their platform.
- */
- if (statbuf->st_size > MAX_TAR_MEMBER_FILELEN)
- ereport(ERROR,
- (errmsg("archive member \"%s\" too large for tar format",
- tarfilename)));
-
_tarWriteHeader(tarfilename, NULL, statbuf);
while ((cnt = fread(buf, 1, Min(sizeof(buf), statbuf->st_size - len), fp)) > 0)
--- 1159,1164 ----
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
index 80de882..8c4dffe 100644
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
*************** ReceiveTarFile(PGconn *conn, PGresult *r
*** 781,787 ****
bool in_tarhdr = true;
bool skip_file = false;
size_t tarhdrsz = 0;
! size_t filesz = 0;
#ifdef HAVE_LIBZ
gzFile ztarfile = NULL;
--- 781,787 ----
bool in_tarhdr = true;
bool skip_file = false;
size_t tarhdrsz = 0;
! pgoff_t filesz = 0;
#ifdef HAVE_LIBZ
gzFile ztarfile = NULL;
*************** ReceiveTarFile(PGconn *conn, PGresult *r
*** 1046,1052 ****
skip_file = (strcmp(&tarhdr[0], "recovery.conf") == 0);
! sscanf(&tarhdr[124], "%11o", (unsigned int *) &filesz);
padding = ((filesz + 511) & ~511) - filesz;
filesz += padding;
--- 1046,1052 ----
skip_file = (strcmp(&tarhdr[0], "recovery.conf") == 0);
! filesz = read_tar_number(&tarhdr[124], 12);
padding = ((filesz + 511) & ~511) - filesz;
filesz += padding;
*************** ReceiveAndUnpackTarFile(PGconn *conn, PG
*** 1139,1145 ****
char current_path[MAXPGPATH];
char filename[MAXPGPATH];
const char *mapped_tblspc_path;
! int current_len_left;
int current_padding = 0;
bool basetablespace;
char *copybuf = NULL;
--- 1139,1145 ----
char current_path[MAXPGPATH];
char filename[MAXPGPATH];
const char *mapped_tblspc_path;
! pgoff_t current_len_left = 0;
int current_padding = 0;
bool basetablespace;
char *copybuf = NULL;
*************** ReceiveAndUnpackTarFile(PGconn *conn, PG
*** 1208,1227 ****
}
totaldone += 512;
! if (sscanf(copybuf + 124, "%11o", ¤t_len_left) != 1)
! {
! fprintf(stderr, _("%s: could not parse file size\n"),
! progname);
! disconnect_and_exit(1);
! }
/* Set permissions on the file */
! if (sscanf(©buf[100], "%07o ", &filemode) != 1)
! {
! fprintf(stderr, _("%s: could not parse file mode\n"),
! progname);
! disconnect_and_exit(1);
! }
/*
* All files are padded up to 512 bytes
--- 1208,1217 ----
}
totaldone += 512;
! current_len_left = read_tar_number(©buf[124], 12);
/* Set permissions on the file */
! filemode = read_tar_number(©buf[100], 8);
/*
* All files are padded up to 512 bytes
*************** main(int argc, char **argv)
*** 2180,2186 ****
if (replication_slot && !streamwal)
{
fprintf(stderr,
! _("%s: replication slots can only be used with WAL streaming\n"),
progname);
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
progname);
--- 2170,2176 ----
if (replication_slot && !streamwal)
{
fprintf(stderr,
! _("%s: replication slots can only be used with WAL streaming\n"),
progname);
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
progname);
diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c
index 532eacc..c40dfe5 100644
*** a/src/bin/pg_dump/pg_backup_tar.c
--- b/src/bin/pg_dump/pg_backup_tar.c
*************** typedef struct
*** 78,90 ****
ArchiveHandle *AH;
} TAR_MEMBER;
- /*
- * Maximum file size for a tar member: The limit inherent in the
- * format is 2^33-1 bytes (nearly 8 GB). But we don't want to exceed
- * what we can represent in pgoff_t.
- */
- #define MAX_TAR_MEMBER_FILELEN (((int64) 1 << Min(33, sizeof(pgoff_t)*8 - 1)) - 1)
-
typedef struct
{
int hasSeek;
--- 78,83 ----
*************** isValidTarHeader(char *header)
*** 1049,1055 ****
int sum;
int chk = tarChecksum(header);
! sscanf(&header[148], "%8o", &sum);
if (sum != chk)
return false;
--- 1042,1048 ----
int sum;
int chk = tarChecksum(header);
! sum = read_tar_number(&header[148], 8);
if (sum != chk)
return false;
*************** _tarAddFile(ArchiveHandle *AH, TAR_MEMBE
*** 1091,1103 ****
strerror(errno));
fseeko(tmp, 0, SEEK_SET);
- /*
- * Some compilers will throw a warning knowing this test can never be true
- * because pgoff_t can't exceed the compared maximum on their platform.
- */
- if (th->fileLen > MAX_TAR_MEMBER_FILELEN)
- exit_horribly(modulename, "archive member too large for tar format\n");
-
_tarWriteHeader(th);
while ((cnt = fread(buf, 1, sizeof(buf), tmp)) > 0)
--- 1084,1089 ----
*************** _tarGetHeader(ArchiveHandle *AH, TAR_MEM
*** 1222,1232 ****
{
lclContext *ctx = (lclContext *) AH->formatData;
char h[512];
! char tag[100];
int sum,
chk;
! size_t len;
! unsigned long ullen;
pgoff_t hPos;
bool gotBlock = false;
--- 1208,1217 ----
{
lclContext *ctx = (lclContext *) AH->formatData;
char h[512];
! char tag[100 + 1];
int sum,
chk;
! pgoff_t len;
pgoff_t hPos;
bool gotBlock = false;
*************** _tarGetHeader(ArchiveHandle *AH, TAR_MEM
*** 1249,1255 ****
/* Calc checksum */
chk = tarChecksum(h);
! sscanf(&h[148], "%8o", &sum);
/*
* If the checksum failed, see if it is a null block. If so, silently
--- 1234,1240 ----
/* Calc checksum */
chk = tarChecksum(h);
! sum = read_tar_number(&h[148], 8);
/*
* If the checksum failed, see if it is a null block. If so, silently
*************** _tarGetHeader(ArchiveHandle *AH, TAR_MEM
*** 1272,1298 ****
}
}
! sscanf(&h[0], "%99s", tag);
! sscanf(&h[124], "%12lo", &ullen);
! len = (size_t) ullen;
{
! char buf[100];
! snprintf(buf, sizeof(buf), INT64_FORMAT, (int64) hPos);
! ahlog(AH, 3, "TOC Entry %s at %s (length %lu, checksum %d)\n",
! tag, buf, (unsigned long) len, sum);
}
if (chk != sum)
{
! char buf[100];
! snprintf(buf, sizeof(buf), INT64_FORMAT, (int64) ftello(ctx->tarFH));
exit_horribly(modulename,
"corrupt tar header found in %s "
"(expected %d, computed %d) file position %s\n",
! tag, sum, chk, buf);
}
th->targetFile = pg_strdup(tag);
--- 1257,1287 ----
}
}
! /* Name field is 100 bytes, might not be null-terminated */
! strlcpy(tag, &h[0], 100 + 1);
!
! len = read_tar_number(&h[124], 12);
{
! char posbuf[32];
! char lenbuf[32];
! snprintf(posbuf, sizeof(posbuf), UINT64_FORMAT, (uint64) hPos);
! snprintf(lenbuf, sizeof(lenbuf), UINT64_FORMAT, (uint64) len);
! ahlog(AH, 3, "TOC Entry %s at %s (length %s, checksum %d)\n",
! tag, posbuf, lenbuf, sum);
}
if (chk != sum)
{
! char posbuf[32];
! snprintf(posbuf, sizeof(posbuf), UINT64_FORMAT,
! (uint64) ftello(ctx->tarFH));
exit_horribly(modulename,
"corrupt tar header found in %s "
"(expected %d, computed %d) file position %s\n",
! tag, sum, chk, posbuf);
}
th->targetFile = pg_strdup(tag);
*************** _tarWriteHeader(TAR_MEMBER *th)
*** 1307,1313 ****
{
char h[512];
! tarCreateHeader(h, th->targetFile, NULL, th->fileLen, 0600, 04000, 02000, time(NULL));
/* Now write the completed header. */
if (fwrite(h, 1, 512, th->tarFH) != 512)
--- 1296,1303 ----
{
char h[512];
! tarCreateHeader(h, th->targetFile, NULL, th->fileLen,
! 0600, 04000, 02000, time(NULL));
/* Now write the completed header. */
if (fwrite(h, 1, 512, th->tarFH) != 512)
diff --git a/src/include/pgtar.h b/src/include/pgtar.h
index 906db7c..9c94a58 100644
*** a/src/include/pgtar.h
--- b/src/include/pgtar.h
*************** enum tarError
*** 19,23 ****
TAR_SYMLINK_TOO_LONG
};
! extern enum tarError tarCreateHeader(char *h, const char *filename, const char *linktarget, size_t size, mode_t mode,
uid_tuid, gid_t gid, time_t mtime);
extern int tarChecksum(char *header);
--- 19,25 ----
TAR_SYMLINK_TOO_LONG
};
! extern enum tarError tarCreateHeader(char *h, const char *filename, const char *linktarget,
! pgoff_t size, mode_t mode, uid_t uid, gid_t gid, time_t mtime);
! extern uint64 read_tar_number(const char *s, int len);
extern int tarChecksum(char *header);
diff --git a/src/port/tar.c b/src/port/tar.c
index 72fd4e1..52a2113 100644
*** a/src/port/tar.c
--- b/src/port/tar.c
***************
*** 3,23 ****
#include <sys/stat.h>
/*
! * Utility routine to print possibly larger than 32 bit integers in a
! * portable fashion. Filled with zeros.
*/
static void
! print_val(char *s, uint64 val, unsigned int base, size_t len)
{
! int i;
!
! for (i = len; i > 0; i--)
{
! int digit = val % base;
! s[i - 1] = '0' + digit;
! val = val / base;
}
}
--- 3,82 ----
#include <sys/stat.h>
/*
! * Print a numeric field in a tar header. The field starts at *s and is of
! * length len; val is the value to be written.
! *
! * Per POSIX, the way to write a number is in octal with leading zeroes and
! * one trailing space (or NUL, but we use space) at the end of the specified
! * field width.
! *
! * However, the given value may not fit in the available space in octal form.
! * If that's true, we use the GNU extension of writing \200 followed by the
! * number in base-256 form (ie, stored in binary MSB-first). (Note: here we
! * support only non-negative numbers, so we don't worry about the GNU rules
! * for handling negative numbers.)
*/
static void
! print_tar_number(char *s, int len, uint64 val)
{
! if (val < (((uint64) 1) << ((len - 1) * 3)))
{
! /* Use octal with trailing space */
! s[--len] = ' ';
! while (len)
! {
! s[--len] = (val & 7) + '0';
! val >>= 3;
! }
! }
! else
! {
! /* Use base-256 with leading \200 */
! s[0] = '\200';
! while (len > 1)
! {
! s[--len] = (val & 255);
! val >>= 8;
! }
! }
! }
!
! /*
! * Read a numeric field in a tar header. The field starts at *s and is of
! * length len.
! *
! * The POSIX-approved format for a number is octal, ending with a space or
! * NUL. However, for values that don't fit, we recognize the GNU extension
! * of \200 followed by the number in base-256 form (ie, stored in binary
! * MSB-first). (Note: here we support only non-negative numbers, so we don't
! * worry about the GNU rules for handling negative numbers.)
! */
! uint64
! read_tar_number(const char *s, int len)
! {
! uint64 result = 0;
!
! if (*s == '\200')
! {
! /* base-256 */
! while (--len)
! {
! result <<= 8;
! result |= (unsigned char) (*++s);
! }
! }
! else
! {
! /* octal */
! while (len-- && *s >= '0' && *s <= '7')
! {
! result <<= 3;
! result |= (*s - '0');
! s++;
! }
}
+ return result;
}
*************** tarChecksum(char *header)
*** 46,57 ****
/*
* Fill in the buffer pointed to by h with a tar format header. This buffer
! * must always have space for 512 characters, which is a requirement by
* the tar format.
*/
enum tarError
tarCreateHeader(char *h, const char *filename, const char *linktarget,
! size_t size, mode_t mode, uid_t uid, gid_t gid, time_t mtime)
{
if (strlen(filename) > 99)
return TAR_NAME_TOO_LONG;
--- 105,116 ----
/*
* Fill in the buffer pointed to by h with a tar format header. This buffer
! * must always have space for 512 characters, which is a requirement of
* the tar format.
*/
enum tarError
tarCreateHeader(char *h, const char *filename, const char *linktarget,
! pgoff_t size, mode_t mode, uid_t uid, gid_t gid, time_t mtime)
{
if (strlen(filename) > 99)
return TAR_NAME_TOO_LONG;
*************** tarCreateHeader(char *h, const char *fil
*** 59,70 ****
if (linktarget && strlen(linktarget) > 99)
return TAR_SYMLINK_TOO_LONG;
- /*
- * Note: most of the fields in a tar header are not supposed to be
- * null-terminated. We use sprintf, which will write a null after the
- * required bytes; that null goes into the first byte of the next field.
- * This is okay as long as we fill the fields in order.
- */
memset(h, 0, 512); /* assume tar header size */
/* Name 100 */
--- 118,123 ----
*************** tarCreateHeader(char *h, const char *fil
*** 84,129 ****
}
/* Mode 8 - this doesn't include the file type bits (S_IFMT) */
! sprintf(&h[100], "%07o ", (int) (mode & 07777));
/* User ID 8 */
! sprintf(&h[108], "%07o ", (int) uid);
/* Group 8 */
! sprintf(&h[116], "%07o ", (int) gid);
! /* File size 12 - 11 digits, 1 space; use print_val for 64 bit support */
if (linktarget != NULL || S_ISDIR(mode))
/* Symbolic link or directory has size zero */
! print_val(&h[124], 0, 8, 11);
else
! print_val(&h[124], size, 8, 11);
! sprintf(&h[135], " ");
/* Mod Time 12 */
! sprintf(&h[136], "%011o ", (int) mtime);
/* Checksum 8 cannot be calculated until we've filled all other fields */
if (linktarget != NULL)
{
/* Type - Symbolic link */
! sprintf(&h[156], "2");
/* Link Name 100 */
strlcpy(&h[157], linktarget, 100);
}
else if (S_ISDIR(mode))
/* Type - directory */
! sprintf(&h[156], "5");
else
/* Type - regular file */
! sprintf(&h[156], "0");
/* Magic 6 */
! sprintf(&h[257], "ustar");
/* Version 2 */
! sprintf(&h[263], "00");
/* User 32 */
/* XXX: Do we need to care about setting correct username? */
--- 137,185 ----
}
/* Mode 8 - this doesn't include the file type bits (S_IFMT) */
! print_tar_number(&h[100], 8, (mode & 07777));
/* User ID 8 */
! print_tar_number(&h[108], 8, uid);
/* Group 8 */
! print_tar_number(&h[116], 8, gid);
! /* File size 12 */
if (linktarget != NULL || S_ISDIR(mode))
/* Symbolic link or directory has size zero */
! print_tar_number(&h[124], 12, 0);
else
! print_tar_number(&h[124], 12, size);
/* Mod Time 12 */
! print_tar_number(&h[136], 12, mtime);
/* Checksum 8 cannot be calculated until we've filled all other fields */
if (linktarget != NULL)
{
/* Type - Symbolic link */
! h[156] = '2';
/* Link Name 100 */
strlcpy(&h[157], linktarget, 100);
}
else if (S_ISDIR(mode))
+ {
/* Type - directory */
! h[156] = '5';
! }
else
+ {
/* Type - regular file */
! h[156] = '0';
! }
/* Magic 6 */
! strcpy(&h[257], "ustar");
/* Version 2 */
! memcpy(&h[263], "00", 2);
/* User 32 */
/* XXX: Do we need to care about setting correct username? */
*************** tarCreateHeader(char *h, const char *fil
*** 134,152 ****
strlcpy(&h[297], "postgres", 32);
/* Major Dev 8 */
! sprintf(&h[329], "%07o ", 0);
/* Minor Dev 8 */
! sprintf(&h[337], "%07o ", 0);
/* Prefix 155 - not used, leave as nulls */
! /*
! * We mustn't overwrite the next field while inserting the checksum.
! * Fortunately, the checksum can't exceed 6 octal digits, so we just write
! * 6 digits, a space, and a null, which is legal per POSIX.
! */
! sprintf(&h[148], "%06o ", tarChecksum(h));
return TAR_OK;
}
--- 190,204 ----
strlcpy(&h[297], "postgres", 32);
/* Major Dev 8 */
! print_tar_number(&h[329], 8, 0);
/* Minor Dev 8 */
! print_tar_number(&h[337], 8, 0);
/* Prefix 155 - not used, leave as nulls */
! /* Finally, compute and insert the checksum */
! print_tar_number(&h[148], 8, tarChecksum(h));
return TAR_OK;
}
On Sun, Nov 22, 2015 at 5:58 AM, Tom Lane <tgl@sss.pgh.pa.us>wrote: > These all seem to me to be must-fix issues even if we didn't want to > back-port use of the base-256 convention. Seeing that we have to deal > with these things, I'm inclined to just back-port the whole patch to > all branches. > FWIW, patch has been pushed as 00cdd83 and backpatched down to 9.1. -- Michael