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:

Reply via email to