From: Tom Lane Date: Sat, 24 Jan 2015 18:05:42 +0000 (-0500) Subject: Replace a bunch more uses of strncpy() with safer coding. X-Git-Tag: REL9_5_ALPHA1~873 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=586dd5d6a5d59e406bc8032bb52625ffb904311c;p=postgresql 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. --- diff --git a/contrib/fuzzystrmatch/dmetaphone.c b/contrib/fuzzystrmatch/dmetaphone.c index b1f8b78d3b..7c8457e734 100644 --- a/contrib/fuzzystrmatch/dmetaphone.c +++ b/contrib/fuzzystrmatch/dmetaphone.c @@ -247,7 +247,7 @@ NewMetaString(char *init_str) META_MALLOC(s->str, s->bufsize, char); assert(s->str != NULL); - strncpy(s->str, init_str, s->length + 1); + memcpy(s->str, init_str, s->length + 1); s->free_string_on_destroy = 1; return s; diff --git a/contrib/isn/isn.c b/contrib/isn/isn.c index 903b9378d8..5fbd253491 100644 --- a/contrib/isn/isn.c +++ b/contrib/isn/isn.c @@ -825,18 +825,18 @@ string2ean(const char *str, bool errorOK, ean13 *result, goto eanwrongtype; break; case ISMN: - strncpy(buf, "9790", 4); /* this isn't for sure yet, for now + memcpy(buf, "9790", 4); /* this isn't for sure yet, for now * ISMN it's only 9790 */ valid = (valid && ((rcheck = checkdig(buf, 13)) == check || magic)); break; case ISBN: - strncpy(buf, "978", 3); + memcpy(buf, "978", 3); valid = (valid && ((rcheck = weight_checkdig(buf + 3, 10)) == check || magic)); break; case ISSN: - strncpy(buf + 10, "00", 2); /* append 00 as the normal issue + memcpy(buf + 10, "00", 2); /* append 00 as the normal issue * publication code */ - strncpy(buf, "977", 3); + memcpy(buf, "977", 3); valid = (valid && ((rcheck = weight_checkdig(buf + 3, 8)) == check || magic)); break; case UPC: diff --git a/contrib/pg_trgm/trgm_regexp.c b/contrib/pg_trgm/trgm_regexp.c index 529e1dbfe9..a91e6186ba 100644 --- a/contrib/pg_trgm/trgm_regexp.c +++ b/contrib/pg_trgm/trgm_regexp.c @@ -877,7 +877,7 @@ convertPgWchar(pg_wchar c, trgm_mb_char *result) #endif /* Fill result with exactly MAX_MULTIBYTE_CHAR_LEN bytes */ - strncpy(result->bytes, s, MAX_MULTIBYTE_CHAR_LEN); + memcpy(result->bytes, s, MAX_MULTIBYTE_CHAR_LEN); return true; } diff --git a/contrib/pgbench/pgbench.c b/contrib/pgbench/pgbench.c index 25616ceff6..ddd11a09c5 100644 --- a/contrib/pgbench/pgbench.c +++ b/contrib/pgbench/pgbench.c @@ -829,7 +829,7 @@ replaceVariable(char **sql, char *param, int len, char *value) if (valueln != len) memmove(param + valueln, param + len, strlen(param + len) + 1); - strncpy(param, value, valueln); + memcpy(param, value, valueln); return param + valueln; } diff --git a/contrib/pgcrypto/crypt-des.c b/contrib/pgcrypto/crypt-des.c index 4ed44beeff..b43141fed5 100644 --- a/contrib/pgcrypto/crypt-des.c +++ b/contrib/pgcrypto/crypt-des.c @@ -708,7 +708,7 @@ px_crypt_des(const char *key, const char *setting) if (des_setkey((char *) keybuf)) return (NULL); } - strncpy(output, setting, 9); + StrNCpy(output, setting, 10); /* * Double check that we weren't given a short setting. If we were, the @@ -716,7 +716,6 @@ px_crypt_des(const char *key, const char *setting) * salt, but we don't really care. Just make sure the output string * doesn't have an extra NUL in it. */ - output[9] = '\0'; p = output + strlen(output); } else diff --git a/contrib/xml2/xpath.c b/contrib/xml2/xpath.c index f57b81302f..655c5322cd 100644 --- a/contrib/xml2/xpath.c +++ b/contrib/xml2/xpath.c @@ -327,7 +327,7 @@ xpath_string(PG_FUNCTION_ARGS) /* We could try casting to string using the libxml function? */ xpath = (xmlChar *) palloc(pathsize + 9); - strncpy((char *) xpath, "string(", 7); + memcpy((char *) xpath, "string(", 7); memcpy((char *) (xpath + 7), VARDATA(xpathsupp), pathsize); xpath[pathsize + 7] = ')'; xpath[pathsize + 8] = '\0'; diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c index f75c4b88be..9cde6a21ce 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 827ad713b5..268bcd58fd 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 f863ee7344..f2fe696425 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 939831b557..30baa45383 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 5f64e494c5..dd7ff0f99a 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 3b0402e3cf..2a44b6e503 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 fa13a9a589..b6c6e39335 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 cfa192193b..f48ce4901c 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 bee2c92301..310c5bbffa 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 9ac06086e7..18614e7a67 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 9e6455861d..f461393dc2 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 2c8778db94..5968c2032a 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 a4c7151f9a..8a3dd759a1 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 e49ae3763c..983b242d06 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 a271cdd7d1..49cb817a50 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 7fdd09b306..7fe29820cf 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 16dd440561..a4d443291d 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 0d2e59cec5..691202894f 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 1eeb976698..eeba7f3504 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 d8b5995f5b..8cdf53ec7f 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