diff options
author | Tom Lane <tgl@sss.pgh.pa.us> | 2015-01-24 13:05:42 -0500 |
---|---|---|
committer | Tom Lane <tgl@sss.pgh.pa.us> | 2015-01-24 13:05:42 -0500 |
commit | 586dd5d6a5d59e406bc8032bb52625ffb904311c (patch) | |
tree | b47c5e9038b75bd100e507f8ba3c7dc92e50d603 /src | |
parent | 9222cd84b0f227287f65df395d52dc7973a62d29 (diff) | |
download | postgresql-586dd5d6a5d59e406bc8032bb52625ffb904311c.tar.gz postgresql-586dd5d6a5d59e406bc8032bb52625ffb904311c.zip |
Replace a bunch more uses of strncpy() with safer coding.
strncpy() has a well-deserved reputation for being unsafe, so make an
effort to get rid of nearly all occurrences in HEAD.
A large fraction of the remaining uses were passing length less than or
equal to the known strlen() of the source, in which case no null-padding
can occur and the behavior is equivalent to memcpy(), though doubtless
slower and certainly harder to reason about. So just use memcpy() in
these cases.
In other cases, use either StrNCpy() or strlcpy() as appropriate (depending
on whether padding to the full length of the destination buffer seems
useful).
I left a few strncpy() calls alone in the src/timezone/ code, to keep it
in sync with upstream (the IANA tzcode distribution). There are also a
few such calls in ecpg that could possibly do with more analysis.
AFAICT, none of these changes are more than cosmetic, except for the four
occurrences in fe-secure-openssl.c, which are in fact buggy: an overlength
source leads to a non-null-terminated destination buffer and ensuing
misbehavior. These don't seem like security issues, first because no stack
clobber is possible and second because if your values of sslcert etc are
coming from untrusted sources then you've got problems way worse than this.
Still, it's undesirable to have unpredictable behavior for overlength
inputs, so back-patch those four changes to all active branches.
Diffstat (limited to 'src')
-rw-r--r-- | src/backend/libpq/hba.c | 7 | ||||
-rw-r--r-- | src/backend/postmaster/pgstat.c | 2 | ||||
-rw-r--r-- | src/backend/regex/regerror.c | 2 | ||||
-rw-r--r-- | src/backend/replication/logical/logical.c | 4 | ||||
-rw-r--r-- | src/backend/replication/slot.c | 3 | ||||
-rw-r--r-- | src/backend/utils/adt/datetime.c | 2 | ||||
-rw-r--r-- | src/backend/utils/adt/name.c | 2 | ||||
-rw-r--r-- | src/backend/utils/adt/varlena.c | 2 | ||||
-rw-r--r-- | src/backend/utils/error/elog.c | 2 | ||||
-rw-r--r-- | src/bin/initdb/initdb.c | 4 | ||||
-rw-r--r-- | src/bin/pg_dump/pg_backup_archiver.c | 2 | ||||
-rw-r--r-- | src/bin/pg_dump/pg_backup_db.c | 2 | ||||
-rw-r--r-- | src/interfaces/ecpg/ecpglib/execute.c | 14 | ||||
-rw-r--r-- | src/interfaces/ecpg/ecpglib/prepare.c | 2 | ||||
-rw-r--r-- | src/interfaces/ecpg/pgtypeslib/datetime.c | 10 | ||||
-rw-r--r-- | src/interfaces/ecpg/pgtypeslib/dt_common.c | 2 | ||||
-rw-r--r-- | src/interfaces/ecpg/test/pg_regress_ecpg.c | 3 | ||||
-rw-r--r-- | src/interfaces/libpq/fe-exec.c | 6 | ||||
-rw-r--r-- | src/interfaces/libpq/fe-protocol2.c | 1 | ||||
-rw-r--r-- | src/interfaces/libpq/fe-secure-openssl.c | 8 |
20 files changed, 40 insertions, 40 deletions
diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c index f75c4b88be8..9cde6a21ce9 100644 --- a/src/backend/libpq/hba.c +++ b/src/backend/libpq/hba.c @@ -1996,6 +1996,8 @@ check_ident_usermap(IdentLine *identLine, const char *usermap_name, if ((ofs = strstr(identLine->pg_role, "\\1")) != NULL) { + int offset; + /* substitution of the first argument requested */ if (matches[1].rm_so < 0) { @@ -2012,8 +2014,9 @@ check_ident_usermap(IdentLine *identLine, const char *usermap_name, * plus null terminator */ regexp_pgrole = palloc0(strlen(identLine->pg_role) - 2 + (matches[1].rm_eo - matches[1].rm_so) + 1); - strncpy(regexp_pgrole, identLine->pg_role, (ofs - identLine->pg_role)); - memcpy(regexp_pgrole + strlen(regexp_pgrole), + offset = ofs - identLine->pg_role; + memcpy(regexp_pgrole, identLine->pg_role, offset); + memcpy(regexp_pgrole + offset, ident_user + matches[1].rm_so, matches[1].rm_eo - matches[1].rm_so); strcat(regexp_pgrole, ofs + 2); diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index 827ad713b58..268bcd58fd8 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -3095,7 +3095,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)); + StrNCpy(msg.m_xlog, xlog, sizeof(msg.m_xlog)); msg.m_timestamp = GetCurrentTimestamp(); pgstat_send(&msg, sizeof(msg)); } diff --git a/src/backend/regex/regerror.c b/src/backend/regex/regerror.c index f863ee7344f..f2fe696425c 100644 --- a/src/backend/regex/regerror.c +++ b/src/backend/regex/regerror.c @@ -111,7 +111,7 @@ pg_regerror(int errcode, /* error code, or REG_ATOI or REG_ITOA */ strcpy(errbuf, msg); else { /* truncate to fit */ - strncpy(errbuf, msg, errbuf_size - 1); + memcpy(errbuf, msg, errbuf_size - 1); errbuf[errbuf_size - 1] = '\0'; } } diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c index 939831b5576..30baa45383a 100644 --- a/src/backend/replication/logical/logical.c +++ b/src/backend/replication/logical/logical.c @@ -244,9 +244,7 @@ CreateInitDecodingContext(char *plugin, /* register output plugin name with slot */ SpinLockAcquire(&slot->mutex); - strncpy(NameStr(slot->data.plugin), plugin, - NAMEDATALEN); - NameStr(slot->data.plugin)[NAMEDATALEN - 1] = '\0'; + StrNCpy(NameStr(slot->data.plugin), plugin, NAMEDATALEN); SpinLockRelease(&slot->mutex); /* diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c index 5f64e494c5d..dd7ff0f99a2 100644 --- a/src/backend/replication/slot.c +++ b/src/backend/replication/slot.c @@ -266,8 +266,7 @@ ReplicationSlotCreate(const char *name, bool db_specific, slot->data.persistency = persistency; slot->data.xmin = InvalidTransactionId; slot->effective_xmin = InvalidTransactionId; - strncpy(NameStr(slot->data.name), name, NAMEDATALEN); - NameStr(slot->data.name)[NAMEDATALEN - 1] = '\0'; + StrNCpy(NameStr(slot->data.name), name, NAMEDATALEN); slot->data.database = db_specific ? MyDatabaseId : InvalidOid; slot->data.restart_lsn = InvalidXLogRecPtr; diff --git a/src/backend/utils/adt/datetime.c b/src/backend/utils/adt/datetime.c index 3b0402e3cfb..2a44b6e5037 100644 --- a/src/backend/utils/adt/datetime.c +++ b/src/backend/utils/adt/datetime.c @@ -4052,7 +4052,7 @@ EncodeDateTime(struct pg_tm * tm, fsec_t fsec, bool print_tz, int tz, const char day = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday); tm->tm_wday = j2day(day); - strncpy(str, days[tm->tm_wday], 3); + memcpy(str, days[tm->tm_wday], 3); strcpy(str + 3, " "); if (DateOrder == DATEORDER_DMY) diff --git a/src/backend/utils/adt/name.c b/src/backend/utils/adt/name.c index fa13a9a5897..b6c6e393358 100644 --- a/src/backend/utils/adt/name.c +++ b/src/backend/utils/adt/name.c @@ -192,7 +192,7 @@ namecpy(Name n1, Name n2) { if (!n1 || !n2) return -1; - strncpy(NameStr(*n1), NameStr(*n2), NAMEDATALEN); + StrNCpy(NameStr(*n1), NameStr(*n2), NAMEDATALEN); return 0; } diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index cfa192193b4..f48ce4901ca 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -2936,7 +2936,7 @@ SplitIdentifierString(char *rawstring, char separator, len = endp - curname; downname = downcase_truncate_identifier(curname, len, false); Assert(strlen(downname) <= len); - strncpy(curname, downname, len); + strncpy(curname, downname, len); /* strncpy is required here */ pfree(downname); } diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index bee2c92301e..310c5bbffa0 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -2240,7 +2240,7 @@ setup_formatted_log_time(void) /* 'paste' milliseconds into place... */ sprintf(msbuf, ".%03d", (int) (tv.tv_usec / 1000)); - strncpy(formatted_log_time + 19, msbuf, 4); + memcpy(formatted_log_time + 19, msbuf, 4); } /* diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c index 9ac06086e78..18614e7a678 100644 --- a/src/bin/initdb/initdb.c +++ b/src/bin/initdb/initdb.c @@ -386,9 +386,9 @@ replace_token(char **lines, const char *token, const char *replacement) pre = where - lines[i]; - strncpy(newline, lines[i], pre); + memcpy(newline, lines[i], pre); - strcpy(newline + pre, replacement); + memcpy(newline + pre, replacement, replen); strcpy(newline + pre + replen, lines[i] + pre + toklen); diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index 9e6455861d9..f461393dc25 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -2064,7 +2064,7 @@ _discoverArchiveFormat(ArchiveHandle *AH) } /* Save it, just in case we need it later */ - strncpy(&AH->lookahead[0], sig, 5); + memcpy(&AH->lookahead[0], sig, 5); AH->lookaheadLen = 5; if (strncmp(sig, "PGDMP", 5) == 0) diff --git a/src/bin/pg_dump/pg_backup_db.c b/src/bin/pg_dump/pg_backup_db.c index 2c8778db94d..5968c2032a1 100644 --- a/src/bin/pg_dump/pg_backup_db.c +++ b/src/bin/pg_dump/pg_backup_db.c @@ -399,7 +399,7 @@ ExecuteSqlCommand(ArchiveHandle *AH, const char *qry, const char *desc) break; default: /* trouble */ - strncpy(errStmt, qry, DB_MAX_ERR_STMT); + strncpy(errStmt, qry, DB_MAX_ERR_STMT); /* strncpy required here */ if (errStmt[DB_MAX_ERR_STMT - 1] != '\0') { errStmt[DB_MAX_ERR_STMT - 4] = '.'; diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c index a4c7151f9a3..8a3dd759a1a 100644 --- a/src/interfaces/ecpg/ecpglib/execute.c +++ b/src/interfaces/ecpg/ecpglib/execute.c @@ -882,7 +882,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari return false; } - strncpy(mallocedval + strlen(mallocedval), str, slen + 1); + memcpy(mallocedval + strlen(mallocedval), str, slen + 1); strcpy(mallocedval + strlen(mallocedval), ","); ecpg_free(str); } @@ -949,7 +949,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari return false; } - strncpy(mallocedval + strlen(mallocedval), str, slen + 1); + memcpy(mallocedval + strlen(mallocedval), str, slen + 1); strcpy(mallocedval + strlen(mallocedval), ","); ecpg_free(str); } @@ -969,7 +969,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari } /* also copy trailing '\0' */ - strncpy(mallocedval + strlen(mallocedval), str, slen + 1); + memcpy(mallocedval + strlen(mallocedval), str, slen + 1); ecpg_free(str); } @@ -1000,7 +1000,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari return false; } - strncpy(mallocedval + strlen(mallocedval), str, slen + 1); + memcpy(mallocedval + strlen(mallocedval), str, slen + 1); strcpy(mallocedval + strlen(mallocedval), ","); ecpg_free(str); } @@ -1020,7 +1020,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari } /* also copy trailing '\0' */ - strncpy(mallocedval + strlen(mallocedval), str, slen + 1); + memcpy(mallocedval + strlen(mallocedval), str, slen + 1); ecpg_free(str); } @@ -1055,7 +1055,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari return false; } - strncpy(mallocedval + strlen(mallocedval), str, slen + 1); + memcpy(mallocedval + strlen(mallocedval), str, slen + 1); strcpy(mallocedval + strlen(mallocedval), ","); ecpg_free(str); } @@ -1075,7 +1075,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari } /* also copy trailing '\0' */ - strncpy(mallocedval + strlen(mallocedval), str, slen + 1); + memcpy(mallocedval + strlen(mallocedval), str, slen + 1); ecpg_free(str); } diff --git a/src/interfaces/ecpg/ecpglib/prepare.c b/src/interfaces/ecpg/ecpglib/prepare.c index e49ae3763cc..983b242d065 100644 --- a/src/interfaces/ecpg/ecpglib/prepare.c +++ b/src/interfaces/ecpg/ecpglib/prepare.c @@ -82,7 +82,7 @@ replace_variables(char **text, int lineno) return false; } - strncpy(newcopy, *text, ptr); + memcpy(newcopy, *text, ptr); strcpy(newcopy + ptr, buffer); strcat(newcopy, (*text) +ptr + len); diff --git a/src/interfaces/ecpg/pgtypeslib/datetime.c b/src/interfaces/ecpg/pgtypeslib/datetime.c index a271cdd7d15..49cb817a507 100644 --- a/src/interfaces/ecpg/pgtypeslib/datetime.c +++ b/src/interfaces/ecpg/pgtypeslib/datetime.c @@ -264,8 +264,8 @@ PGTYPESdate_fmt_asc(date dDate, const char *fmtstring, char *outbuf) { case PGTYPES_TYPE_STRING_MALLOCED: case PGTYPES_TYPE_STRING_CONSTANT: - strncpy(start_pattern, replace_val.str_val, - strlen(replace_val.str_val)); + memcpy(start_pattern, replace_val.str_val, + strlen(replace_val.str_val)); if (replace_type == PGTYPES_TYPE_STRING_MALLOCED) free(replace_val.str_val); break; @@ -277,7 +277,7 @@ PGTYPESdate_fmt_asc(date dDate, const char *fmtstring, char *outbuf) return -1; snprintf(t, PGTYPES_DATE_NUM_MAX_DIGITS, "%u", replace_val.uint_val); - strncpy(start_pattern, t, strlen(t)); + memcpy(start_pattern, t, strlen(t)); free(t); } break; @@ -289,7 +289,7 @@ PGTYPESdate_fmt_asc(date dDate, const char *fmtstring, char *outbuf) return -1; snprintf(t, PGTYPES_DATE_NUM_MAX_DIGITS, "%02u", replace_val.uint_val); - strncpy(start_pattern, t, strlen(t)); + memcpy(start_pattern, t, strlen(t)); free(t); } break; @@ -301,7 +301,7 @@ PGTYPESdate_fmt_asc(date dDate, const char *fmtstring, char *outbuf) return -1; snprintf(t, PGTYPES_DATE_NUM_MAX_DIGITS, "%04u", replace_val.uint_val); - strncpy(start_pattern, t, strlen(t)); + memcpy(start_pattern, t, strlen(t)); free(t); } break; diff --git a/src/interfaces/ecpg/pgtypeslib/dt_common.c b/src/interfaces/ecpg/pgtypeslib/dt_common.c index 7fdd09b3068..7fe29820cf0 100644 --- a/src/interfaces/ecpg/pgtypeslib/dt_common.c +++ b/src/interfaces/ecpg/pgtypeslib/dt_common.c @@ -929,7 +929,7 @@ EncodeDateTime(struct tm * tm, fsec_t fsec, bool print_tz, int tz, const char *t day = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday); tm->tm_wday = (int) ((day + date2j(2000, 1, 1) + 1) % 7); - strncpy(str, days[tm->tm_wday], 3); + memcpy(str, days[tm->tm_wday], 3); strcpy(str + 3, " "); if (EuroDates) diff --git a/src/interfaces/ecpg/test/pg_regress_ecpg.c b/src/interfaces/ecpg/test/pg_regress_ecpg.c index 16dd4405613..a4d443291dc 100644 --- a/src/interfaces/ecpg/test/pg_regress_ecpg.c +++ b/src/interfaces/ecpg/test/pg_regress_ecpg.c @@ -60,8 +60,7 @@ ecpg_filter(const char *sourcefile, const char *outfile) if (plen > 1) { n = (char *) malloc(plen); - strncpy(n, p + 1, plen - 1); - n[plen - 1] = '\0'; + StrNCpy(n, p + 1, plen); replace_string(linebuf, n, ""); } } diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c index 0d2e59cec5c..691202894fa 100644 --- a/src/interfaces/libpq/fe-exec.c +++ b/src/interfaces/libpq/fe-exec.c @@ -2910,9 +2910,9 @@ PQoidStatus(const PGresult *res) return ""; len = strspn(res->cmdStatus + 7, "0123456789"); - if (len > 23) - len = 23; - strncpy(buf, res->cmdStatus + 7, len); + if (len > sizeof(buf) - 1) + len = sizeof(buf) - 1; + memcpy(buf, res->cmdStatus + 7, len); buf[len] = '\0'; return buf; diff --git a/src/interfaces/libpq/fe-protocol2.c b/src/interfaces/libpq/fe-protocol2.c index 1eeb9766983..eeba7f35047 100644 --- a/src/interfaces/libpq/fe-protocol2.c +++ b/src/interfaces/libpq/fe-protocol2.c @@ -1586,6 +1586,7 @@ pqBuildStartupPacket2(PGconn *conn, int *packetlen, startpacket->protoVersion = htonl(conn->pversion); + /* strncpy is safe here: postmaster will handle full fields correctly */ strncpy(startpacket->user, conn->pguser, SM_USER); strncpy(startpacket->database, conn->dbName, SM_DATABASE); strncpy(startpacket->tty, conn->pgtty, SM_TTY); diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c index d8b5995f5b4..8cdf53ec7f8 100644 --- a/src/interfaces/libpq/fe-secure-openssl.c +++ b/src/interfaces/libpq/fe-secure-openssl.c @@ -941,7 +941,7 @@ initialize_SSL(PGconn *conn) /* Read the client certificate file */ if (conn->sslcert && strlen(conn->sslcert) > 0) - strncpy(fnbuf, conn->sslcert, sizeof(fnbuf)); + strlcpy(fnbuf, conn->sslcert, sizeof(fnbuf)); else if (have_homedir) snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE); else @@ -1132,7 +1132,7 @@ initialize_SSL(PGconn *conn) #endif /* USE_SSL_ENGINE */ { /* PGSSLKEY is not an engine, treat it as a filename */ - strncpy(fnbuf, conn->sslkey, sizeof(fnbuf)); + strlcpy(fnbuf, conn->sslkey, sizeof(fnbuf)); } } else if (have_homedir) @@ -1195,7 +1195,7 @@ initialize_SSL(PGconn *conn) * verification after the connection has been completed. */ if (conn->sslrootcert && strlen(conn->sslrootcert) > 0) - strncpy(fnbuf, conn->sslrootcert, sizeof(fnbuf)); + strlcpy(fnbuf, conn->sslrootcert, sizeof(fnbuf)); else if (have_homedir) snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CERT_FILE); else @@ -1233,7 +1233,7 @@ initialize_SSL(PGconn *conn) if ((cvstore = SSL_CTX_get_cert_store(SSL_context)) != NULL) { if (conn->sslcrl && strlen(conn->sslcrl) > 0) - strncpy(fnbuf, conn->sslcrl, sizeof(fnbuf)); + strlcpy(fnbuf, conn->sslcrl, sizeof(fnbuf)); else if (have_homedir) snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CRL_FILE); else |