On 2020-08-03 19:39, Tom Lane wrote:
That's easy to fix, but it's perhaps wondering briefly why it needs to
be zero-padded. hashname() doesn't care, heap_form_tuple() doesn't
care. Does anything care?
We do have an expectation that there are no undefined bytes in values to
be stored on-disk. There's even some code in coerce_type() that will
complain about this:
Okay, here is a new patch with improved implementations of namecpy() and
namestrcpy(). I didn't see any other places that relied on the
zero-filling behavior of strncpy().
While we're here, shouldn't namestrcpy() do some pg_mbcliplen() stuff
like namein()?
Excellent point --- probably so, unless the callers are all truncating
in advance, which I doubt.
I will look into that separately.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 740866001613e9d6aa29f759085ea284c0d41c94 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Tue, 4 Aug 2020 15:16:37 +0200
Subject: [PATCH v2] Replace remaining StrNCpy() by strlcpy()
They are equivalent, and strlcpy() has become the preferred spelling.
Convert over the remaining cases. Remove StrNCpy() as it is now
unused.
---
contrib/pgcrypto/crypt-des.c | 2 +-
src/backend/access/transam/slru.c | 2 +-
src/backend/access/transam/xlogarchive.c | 2 +-
src/backend/catalog/pg_constraint.c | 2 +-
src/backend/commands/indexcmds.c | 2 +-
src/backend/commands/statscmds.c | 2 +-
src/backend/commands/tablecmds.c | 2 +-
src/backend/postmaster/pgstat.c | 2 +-
src/backend/replication/logical/logical.c | 3 +-
src/backend/replication/slot.c | 2 +-
src/backend/utils/adt/formatting.c | 8 ++---
src/backend/utils/adt/name.c | 6 ++--
src/backend/utils/adt/pg_locale.c | 9 ------
src/backend/utils/adt/ruleutils.c | 2 +-
src/common/exec.c | 4 +--
src/include/c.h | 29 -------------------
src/interfaces/ecpg/pgtypeslib/dt_common.c | 4 +--
src/interfaces/ecpg/test/pg_regress_ecpg.c | 2 +-
.../ssl_passphrase_func.c | 2 +-
19 files changed, 26 insertions(+), 61 deletions(-)
diff --git a/contrib/pgcrypto/crypt-des.c b/contrib/pgcrypto/crypt-des.c
index 6efaa609c9..98c30ea122 100644
--- a/contrib/pgcrypto/crypt-des.c
+++ b/contrib/pgcrypto/crypt-des.c
@@ -720,7 +720,7 @@ px_crypt_des(const char *key, const char *setting)
if (des_setkey((char *) keybuf))
return NULL;
}
- StrNCpy(output, setting, 10);
+ strlcpy(output, setting, 10);
/*
* Double check that we weren't given a short setting. If we
were, the
diff --git a/src/backend/access/transam/slru.c
b/src/backend/access/transam/slru.c
index 9e145f1c36..d1dbb43e09 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -252,7 +252,7 @@ SimpleLruInit(SlruCtl ctl, const char *name, int nslots,
int nlsns,
*/
ctl->shared = shared;
ctl->do_fsync = true; /* default behavior */
- StrNCpy(ctl->Dir, subdir, sizeof(ctl->Dir));
+ strlcpy(ctl->Dir, subdir, sizeof(ctl->Dir));
}
/*
diff --git a/src/backend/access/transam/xlogarchive.c
b/src/backend/access/transam/xlogarchive.c
index cdd586fcfb..8f8734dc1d 100644
--- a/src/backend/access/transam/xlogarchive.c
+++ b/src/backend/access/transam/xlogarchive.c
@@ -323,7 +323,7 @@ ExecuteRecoveryCommand(const char *command, const char
*commandName, bool failOn
case 'r':
/* %r: filename of last restartpoint */
sp++;
- StrNCpy(dp, lastRestartPointFname, endp
- dp);
+ strlcpy(dp, lastRestartPointFname, endp
- dp);
dp += strlen(dp);
break;
case '%':
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index fdc63e7dea..6a6b2cb8c0 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -484,7 +484,7 @@ ChooseConstraintName(const char *name1, const char *name2,
conDesc = table_open(ConstraintRelationId, AccessShareLock);
/* try the unmodified label first */
- StrNCpy(modlabel, label, sizeof(modlabel));
+ strlcpy(modlabel, label, sizeof(modlabel));
for (;;)
{
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 2baca12c5f..7819266a63 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -2246,7 +2246,7 @@ ChooseRelationName(const char *name1, const char *name2,
char modlabel[NAMEDATALEN];
/* try the unmodified label first */
- StrNCpy(modlabel, label, sizeof(modlabel));
+ strlcpy(modlabel, label, sizeof(modlabel));
for (;;)
{
diff --git a/src/backend/commands/statscmds.c b/src/backend/commands/statscmds.c
index 974828545c..3057d89d50 100644
--- a/src/backend/commands/statscmds.c
+++ b/src/backend/commands/statscmds.c
@@ -681,7 +681,7 @@ ChooseExtendedStatisticName(const char *name1, const char
*name2,
char modlabel[NAMEDATALEN];
/* try the unmodified label first */
- StrNCpy(modlabel, label, sizeof(modlabel));
+ strlcpy(modlabel, label, sizeof(modlabel));
for (;;)
{
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ac53f79ada..cd989c95e5 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -606,7 +606,7 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
* Truncate relname to appropriate length (probably a waste of time, as
* parser should have done this already).
*/
- StrNCpy(relname, stmt->relation->relname, NAMEDATALEN);
+ strlcpy(relname, stmt->relation->relname, NAMEDATALEN);
/*
* Check consistency of arguments
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 15f92b66c6..73ce944fb1 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -4367,7 +4367,7 @@ pgstat_send_archiver(const char *xlog, bool failed)
*/
pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_ARCHIVER);
msg.m_failed = failed;
- StrNCpy(msg.m_xlog, xlog, sizeof(msg.m_xlog));
+ strlcpy(msg.m_xlog, xlog, sizeof(msg.m_xlog));
msg.m_timestamp = GetCurrentTimestamp();
pgstat_send(&msg, sizeof(msg));
}
diff --git a/src/backend/replication/logical/logical.c
b/src/backend/replication/logical/logical.c
index 05d24b93da..ebc0b7080e 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -39,6 +39,7 @@
#include "replication/snapbuild.h"
#include "storage/proc.h"
#include "storage/procarray.h"
+#include "utils/builtins.h"
#include "utils/memutils.h"
/* data for errcontext callback */
@@ -321,7 +322,7 @@ CreateInitDecodingContext(char *plugin,
/* register output plugin name with slot */
SpinLockAcquire(&slot->mutex);
- StrNCpy(NameStr(slot->data.plugin), plugin, NAMEDATALEN);
+ namestrcpy(&slot->data.plugin, plugin);
SpinLockRelease(&slot->mutex);
if (XLogRecPtrIsInvalid(restart_lsn))
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 57bbb6288c..3dc01b6df2 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -275,7 +275,7 @@ ReplicationSlotCreate(const char *name, bool db_specific,
/* first initialize persistent data */
memset(&slot->data, 0, sizeof(ReplicationSlotPersistentData));
- StrNCpy(NameStr(slot->data.name), name, NAMEDATALEN);
+ namestrcpy(&slot->data.name, name);
slot->data.database = db_specific ? MyDatabaseId : InvalidOid;
slot->data.persistency = persistency;
diff --git a/src/backend/utils/adt/formatting.c
b/src/backend/utils/adt/formatting.c
index 6626438136..9de63686ec 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -3890,7 +3890,7 @@ DCH_cache_getnew(const char *str, bool std)
elog(DEBUG_elog_output, "OLD: '%s' AGE: %d", old->str,
old->age);
#endif
old->valid = false;
- StrNCpy(old->str, str, DCH_CACHE_SIZE + 1);
+ strlcpy(old->str, str, DCH_CACHE_SIZE + 1);
old->age = (++DCHCounter);
/* caller is expected to fill format, then set valid */
return old;
@@ -3904,7 +3904,7 @@ DCH_cache_getnew(const char *str, bool std)
DCHCache[n_DCHCache] = ent = (DCHCacheEntry *)
MemoryContextAllocZero(TopMemoryContext,
sizeof(DCHCacheEntry));
ent->valid = false;
- StrNCpy(ent->str, str, DCH_CACHE_SIZE + 1);
+ strlcpy(ent->str, str, DCH_CACHE_SIZE + 1);
ent->std = std;
ent->age = (++DCHCounter);
/* caller is expected to fill format, then set valid */
@@ -4799,7 +4799,7 @@ NUM_cache_getnew(const char *str)
elog(DEBUG_elog_output, "OLD: \"%s\" AGE: %d", old->str,
old->age);
#endif
old->valid = false;
- StrNCpy(old->str, str, NUM_CACHE_SIZE + 1);
+ strlcpy(old->str, str, NUM_CACHE_SIZE + 1);
old->age = (++NUMCounter);
/* caller is expected to fill format and Num, then set valid */
return old;
@@ -4813,7 +4813,7 @@ NUM_cache_getnew(const char *str)
NUMCache[n_NUMCache] = ent = (NUMCacheEntry *)
MemoryContextAllocZero(TopMemoryContext,
sizeof(NUMCacheEntry));
ent->valid = false;
- StrNCpy(ent->str, str, NUM_CACHE_SIZE + 1);
+ strlcpy(ent->str, str, NUM_CACHE_SIZE + 1);
ent->age = (++NUMCounter);
/* caller is expected to fill format and Num, then set valid */
++n_NUMCache;
diff --git a/src/backend/utils/adt/name.c b/src/backend/utils/adt/name.c
index 64877f67e0..895def56c2 100644
--- a/src/backend/utils/adt/name.c
+++ b/src/backend/utils/adt/name.c
@@ -234,7 +234,7 @@ namecpy(Name n1, const NameData *n2)
{
if (!n1 || !n2)
return -1;
- StrNCpy(NameStr(*n1), NameStr(*n2), NAMEDATALEN);
+ memcpy(n1, n2, NAMEDATALEN);
return 0;
}
@@ -251,7 +251,9 @@ namestrcpy(Name name, const char *str)
{
if (!name || !str)
return -1;
- StrNCpy(NameStr(*name), str, NAMEDATALEN);
+ /* NB: We need to zero-pad the destination. */
+ strncpy(NameStr(*name), str, NAMEDATALEN);
+ NameStr(*name)[NAMEDATALEN-1] = '\0';
return 0;
}
diff --git a/src/backend/utils/adt/pg_locale.c
b/src/backend/utils/adt/pg_locale.c
index 11d05c73ac..07299dbc09 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -75,16 +75,7 @@
#endif
#ifdef WIN32
-/*
- * This Windows file defines StrNCpy. We don't need it here, so we undefine
- * it to keep the compiler quiet, and undefine it again after the file is
- * included, so we don't accidentally use theirs.
- */
-#undef StrNCpy
#include <shlwapi.h>
-#ifdef StrNCpy
-#undef StrNCpy
-#endif
#endif
#define MAX_L10N_DATA 80
diff --git a/src/backend/utils/adt/ruleutils.c
b/src/backend/utils/adt/ruleutils.c
index 2cbcb4b85e..f507e5cd76 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -2489,7 +2489,7 @@ pg_get_userbyid(PG_FUNCTION_ARGS)
if (HeapTupleIsValid(roletup))
{
role_rec = (Form_pg_authid) GETSTRUCT(roletup);
- StrNCpy(NameStr(*result), NameStr(role_rec->rolname),
NAMEDATALEN);
+ namecpy(result, &role_rec->rolname);
ReleaseSysCache(roletup);
}
else
diff --git a/src/common/exec.c b/src/common/exec.c
index f39b0a294b..78bb486f99 100644
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -144,7 +144,7 @@ find_my_exec(const char *argv0, char *retpath)
if (first_dir_separator(argv0) != NULL)
{
if (is_absolute_path(argv0))
- StrNCpy(retpath, argv0, MAXPGPATH);
+ strlcpy(retpath, argv0, MAXPGPATH);
else
join_path_components(retpath, cwd, argv0);
canonicalize_path(retpath);
@@ -184,7 +184,7 @@ find_my_exec(const char *argv0, char *retpath)
if (!endp)
endp = startp + strlen(startp); /* point to end
*/
- StrNCpy(test_path, startp, Min(endp - startp + 1,
MAXPGPATH));
+ strlcpy(test_path, startp, Min(endp - startp + 1,
MAXPGPATH));
if (is_absolute_path(test_path))
join_path_components(retpath, test_path, argv0);
diff --git a/src/include/c.h b/src/include/c.h
index f242e32edb..2c61ca8aa8 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -932,35 +932,6 @@ extern void ExceptionalCondition(const char *conditionName,
*/
#define Abs(x) ((x) >= 0 ? (x) : -(x))
-/*
- * StrNCpy
- * Like standard library function strncpy(), except that result string
- * is guaranteed to be null-terminated --- that is, at most N-1 bytes
- * of the source string will be kept.
- * Also, the macro returns no result (too hard to do that without
- * evaluating the arguments multiple times, which seems worse).
- *
- * BTW: when you need to copy a non-null-terminated string (like a text
- * datum) and add a null, do not do it with StrNCpy(..., len+1). That
- * might seem to work, but it fetches one byte more than there is in the
- * text object. One fine day you'll have a SIGSEGV because there isn't
- * another byte before the end of memory. Don't laugh, we've had real
- * live bug reports from real live users over exactly this mistake.
- * Do it honestly with "memcpy(dst,src,len); dst[len] = '\0';", instead.
- */
-#define StrNCpy(dst,src,len) \
- do \
- { \
- char * _dst = (dst); \
- Size _len = (len); \
-\
- if (_len > 0) \
- { \
- strncpy(_dst, (src), _len); \
- _dst[_len-1] = '\0'; \
- } \
- } while (0)
-
/* Get a bit mask of the bits set in non-long aligned addresses */
#define LONG_ALIGN_MASK (sizeof(long) - 1)
diff --git a/src/interfaces/ecpg/pgtypeslib/dt_common.c
b/src/interfaces/ecpg/pgtypeslib/dt_common.c
index 14cdf2d428..e8a8a0f0ed 100644
--- a/src/interfaces/ecpg/pgtypeslib/dt_common.c
+++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c
@@ -1015,7 +1015,7 @@ abstime2tm(AbsoluteTime _time, int *tzp, struct tm *tm,
char **tzn)
* Copy no more than MAXTZLEN bytes of timezone to tzn,
in case it
* contains an error message, which doesn't fit in the
buffer
*/
- StrNCpy(*tzn, tm->tm_zone, MAXTZLEN + 1);
+ strlcpy(*tzn, tm->tm_zone, MAXTZLEN + 1);
if (strlen(tm->tm_zone) > MAXTZLEN)
tm->tm_isdst = -1;
}
@@ -1033,7 +1033,7 @@ abstime2tm(AbsoluteTime _time, int *tzp, struct tm *tm,
char **tzn)
* Copy no more than MAXTZLEN bytes of timezone to tzn,
in case it
* contains an error message, which doesn't fit in the
buffer
*/
- StrNCpy(*tzn, TZNAME_GLOBAL[tm->tm_isdst], MAXTZLEN +
1);
+ strlcpy(*tzn, TZNAME_GLOBAL[tm->tm_isdst], MAXTZLEN +
1);
if (strlen(TZNAME_GLOBAL[tm->tm_isdst]) > MAXTZLEN)
tm->tm_isdst = -1;
}
diff --git a/src/interfaces/ecpg/test/pg_regress_ecpg.c
b/src/interfaces/ecpg/test/pg_regress_ecpg.c
index 956a599fcb..46b9e78fe5 100644
--- a/src/interfaces/ecpg/test/pg_regress_ecpg.c
+++ b/src/interfaces/ecpg/test/pg_regress_ecpg.c
@@ -63,7 +63,7 @@ ecpg_filter(const char *sourcefile, const char *outfile)
if (plen > 1)
{
n = (char *) malloc(plen);
- StrNCpy(n, p + 1, plen);
+ strlcpy(n, p + 1, plen);
replace_string(linebuf, n, "");
}
}
diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
index 563ff144cc..6b0a3db104 100644
--- a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
+++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
@@ -74,7 +74,7 @@ rot13_passphrase(char *buf, int size, int rwflag, void
*userdata)
{
Assert(ssl_passphrase != NULL);
- StrNCpy(buf, ssl_passphrase, size);
+ strlcpy(buf, ssl_passphrase, size);
for (char *p = buf; *p; p++)
{
char c = *p;
--
2.28.0
