Re: strncpy is not a safe version of strcpy

On Wed, Aug 13, 2014 at 10:21:50AM -0400, Tom Lane wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)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