On Wed, Aug 13, 2014 at 10:21:50AM -0400, Tom Lane wrote: > Kevin Grittner <kgri...@ymail.com> writes: > > I am concerned that failure to check for truncation could allow > > deletion of unexpected files or directories. > > I believe that we deal with this by the expedient of checking the lengths > of tablespace paths in advance, when the tablespace is created.
The files under scrutiny here are not located in a tablespace. Even if they were, isn't the length of $PGDATA/pg_tblspc the important factor? $PGDATA can change between runs if the DBA moves the data directory or reaches it via different symlinks, so any DDL-time defense would be incomplete. > > Some might consider it overkill, but I tend to draw a pretty hard > > line on deleting or executing random files, even if the odds seem > > to be that the mangled name won't find a match. Granted, those > > problems exist now, but without checking for truncation it seems to > > me that we're just deleting *different* incorrect filenames, not > > really fixing the problem. I share your (Kevin's) discomfort with our use of strlcpy(). I wouldn't mind someone replacing most strlcpy()/snprintf() calls with calls to wrappers that ereport(ERROR) on truncation. Though as reliability problems go, this one has been minor. David's specific patch has no concrete problem: On Wed, Aug 13, 2014 at 10:26:01PM +1200, David Rowley wrote: > --- a/contrib/pg_archivecleanup/pg_archivecleanup.c > +++ b/contrib/pg_archivecleanup/pg_archivecleanup.c > @@ -108,7 +108,7 @@ CleanupPriorWALFiles(void) > { > while (errno = 0, (xlde = readdir(xldir)) != NULL) > { > - strncpy(walfile, xlde->d_name, MAXPGPATH); > + strlcpy(walfile, xlde->d_name, MAXPGPATH); The code proceeds to check strlen(walfile) == XLOG_DATA_FNAME_LEN, so a long name can't trick it. > TrimExtension(walfile, additional_ext); > > /* > diff --git a/src/backend/access/transam/xlogarchive.c > b/src/backend/access/transam/xlogarchive.c > index 37745dc..0c9498a 100644 > --- a/src/backend/access/transam/xlogarchive.c > +++ b/src/backend/access/transam/xlogarchive.c > @@ -459,7 +459,7 @@ KeepFileRestoredFromArchive(char *path, char *xlogfname) > xlogfpath, oldpath))); > } > #else > - strncpy(oldpath, xlogfpath, MAXPGPATH); > + strlcpy(oldpath, xlogfpath, MAXPGPATH); This one never overflows, because it's copying from one MAXPGPATH buffer to another. Plain strcpy() would be fine, too. -- Noah Misch EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers