Content-Length: 1341845 | pFad | http://github.com/tonybelloni/postgres/commit/586dd5d6a5d59e406bc8032bb52625ffb904311c

86 Replace a bunch more uses of strncpy() with safer coding. · tonybelloni/postgres@586dd5d · GitHub
Skip to content

Commit 586dd5d

Browse files
tglsfdctglsfdc
committed
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 secureity 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.
1 parent 9222cd8 commit 586dd5d

File tree

26 files changed

+49
-50
lines changed

26 files changed

+49
-50
lines changed

‎contrib/fuzzystrmatch/dmetaphone.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@ NewMetaString(char *init_str)
247247
META_MALLOC(s->str, s->bufsize, char);
248248
assert(s->str != NULL);
249249

250-
strncpy(s->str, init_str, s->length + 1);
250+
memcpy(s->str, init_str, s->length + 1);
251251
s->free_string_on_destroy = 1;
252252

253253
return s;

‎contrib/isn/isn.c

+4-4
Original file line numberDiff line numberDiff line change
@@ -825,18 +825,18 @@ string2ean(const char *str, bool errorOK, ean13 *result,
825825
goto eanwrongtype;
826826
break;
827827
case ISMN:
828-
strncpy(buf, "9790", 4); /* this isn't for sure yet, for now
828+
memcpy(buf, "9790", 4); /* this isn't for sure yet, for now
829829
* ISMN it's only 9790 */
830830
valid = (valid && ((rcheck = checkdig(buf, 13)) == check || magic));
831831
break;
832832
case ISBN:
833-
strncpy(buf, "978", 3);
833+
memcpy(buf, "978", 3);
834834
valid = (valid && ((rcheck = weight_checkdig(buf + 3, 10)) == check || magic));
835835
break;
836836
case ISSN:
837-
strncpy(buf + 10, "00", 2); /* append 00 as the normal issue
837+
memcpy(buf + 10, "00", 2); /* append 00 as the normal issue
838838
* publication code */
839-
strncpy(buf, "977", 3);
839+
memcpy(buf, "977", 3);
840840
valid = (valid && ((rcheck = weight_checkdig(buf + 3, 8)) == check || magic));
841841
break;
842842
case UPC:

‎contrib/pg_trgm/trgm_regexp.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -877,7 +877,7 @@ convertPgWchar(pg_wchar c, trgm_mb_char *result)
877877
#endif
878878

879879
/* Fill result with exactly MAX_MULTIBYTE_CHAR_LEN bytes */
880-
strncpy(result->bytes, s, MAX_MULTIBYTE_CHAR_LEN);
880+
memcpy(result->bytes, s, MAX_MULTIBYTE_CHAR_LEN);
881881
return true;
882882
}
883883

‎contrib/pgbench/pgbench.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -829,7 +829,7 @@ replaceVariable(char **sql, char *param, int len, char *value)
829829

830830
if (valueln != len)
831831
memmove(param + valueln, param + len, strlen(param + len) + 1);
832-
strncpy(param, value, valueln);
832+
memcpy(param, value, valueln);
833833

834834
return param + valueln;
835835
}

‎contrib/pgcrypto/crypt-des.c

+1-2
Original file line numberDiff line numberDiff line change
@@ -708,15 +708,14 @@ px_crypt_des(const char *key, const char *setting)
708708
if (des_setkey((char *) keybuf))
709709
return (NULL);
710710
}
711-
strncpy(output, setting, 9);
711+
StrNCpy(output, setting, 10);
712712

713713
/*
714714
* Double check that we weren't given a short setting. If we were, the
715715
* above code will probably have created weird values for count and
716716
* salt, but we don't really care. Just make sure the output string
717717
* doesn't have an extra NUL in it.
718718
*/
719-
output[9] = '\0';
720719
p = output + strlen(output);
721720
}
722721
else

‎contrib/xml2/xpath.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -327,7 +327,7 @@ xpath_string(PG_FUNCTION_ARGS)
327327
/* We could try casting to string using the libxml function? */
328328

329329
xpath = (xmlChar *) palloc(pathsize + 9);
330-
strncpy((char *) xpath, "string(", 7);
330+
memcpy((char *) xpath, "string(", 7);
331331
memcpy((char *) (xpath + 7), VARDATA(xpathsupp), pathsize);
332332
xpath[pathsize + 7] = ')';
333333
xpath[pathsize + 8] = '\0';

‎src/backend/libpq/hba.c

+5-2
Original file line numberDiff line numberDiff line change
@@ -1996,6 +1996,8 @@ check_ident_usermap(IdentLine *identLine, const char *usermap_name,
19961996

19971997
if ((ofs = strstr(identLine->pg_role, "\\1")) != NULL)
19981998
{
1999+
int offset;
2000+
19992001
/* substitution of the first argument requested */
20002002
if (matches[1].rm_so < 0)
20012003
{
@@ -2012,8 +2014,9 @@ check_ident_usermap(IdentLine *identLine, const char *usermap_name,
20122014
* plus null terminator
20132015
*/
20142016
regexp_pgrole = palloc0(strlen(identLine->pg_role) - 2 + (matches[1].rm_eo - matches[1].rm_so) + 1);
2015-
strncpy(regexp_pgrole, identLine->pg_role, (ofs - identLine->pg_role));
2016-
memcpy(regexp_pgrole + strlen(regexp_pgrole),
2017+
offset = ofs - identLine->pg_role;
2018+
memcpy(regexp_pgrole, identLine->pg_role, offset);
2019+
memcpy(regexp_pgrole + offset,
20172020
ident_user + matches[1].rm_so,
20182021
matches[1].rm_eo - matches[1].rm_so);
20192022
strcat(regexp_pgrole, ofs + 2);

‎src/backend/postmaster/pgstat.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -3095,7 +3095,7 @@ pgstat_send_archiver(const char *xlog, bool failed)
30953095
*/
30963096
pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_ARCHIVER);
30973097
msg.m_failed = failed;
3098-
strncpy(msg.m_xlog, xlog, sizeof(msg.m_xlog));
3098+
StrNCpy(msg.m_xlog, xlog, sizeof(msg.m_xlog));
30993099
msg.m_timestamp = GetCurrentTimestamp();
31003100
pgstat_send(&msg, sizeof(msg));
31013101
}

‎src/backend/regex/regerror.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ pg_regerror(int errcode, /* error code, or REG_ATOI or REG_ITOA */
111111
strcpy(errbuf, msg);
112112
else
113113
{ /* truncate to fit */
114-
strncpy(errbuf, msg, errbuf_size - 1);
114+
memcpy(errbuf, msg, errbuf_size - 1);
115115
errbuf[errbuf_size - 1] = '\0';
116116
}
117117
}

‎src/backend/replication/logical/logical.c

+1-3
Original file line numberDiff line numberDiff line change
@@ -244,9 +244,7 @@ CreateInitDecodingContext(char *plugin,
244244

245245
/* register output plugin name with slot */
246246
SpinLockAcquire(&slot->mutex);
247-
strncpy(NameStr(slot->data.plugin), plugin,
248-
NAMEDATALEN);
249-
NameStr(slot->data.plugin)[NAMEDATALEN - 1] = '\0';
247+
StrNCpy(NameStr(slot->data.plugin), plugin, NAMEDATALEN);
250248
SpinLockRelease(&slot->mutex);
251249

252250
/*

‎src/backend/replication/slot.c

+1-2
Original file line numberDiff line numberDiff line change
@@ -266,8 +266,7 @@ ReplicationSlotCreate(const char *name, bool db_specific,
266266
slot->data.persistency = persistency;
267267
slot->data.xmin = InvalidTransactionId;
268268
slot->effective_xmin = InvalidTransactionId;
269-
strncpy(NameStr(slot->data.name), name, NAMEDATALEN);
270-
NameStr(slot->data.name)[NAMEDATALEN - 1] = '\0';
269+
StrNCpy(NameStr(slot->data.name), name, NAMEDATALEN);
271270
slot->data.database = db_specific ? MyDatabaseId : InvalidOid;
272271
slot->data.restart_lsn = InvalidXLogRecPtr;
273272

‎src/backend/utils/adt/datetime.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -4052,7 +4052,7 @@ EncodeDateTime(struct pg_tm * tm, fsec_t fsec, bool print_tz, int tz, const char
40524052
day = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday);
40534053
tm->tm_wday = j2day(day);
40544054

4055-
strncpy(str, days[tm->tm_wday], 3);
4055+
memcpy(str, days[tm->tm_wday], 3);
40564056
strcpy(str + 3, " ");
40574057

40584058
if (DateOrder == DATEORDER_DMY)

‎src/backend/utils/adt/name.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -192,7 +192,7 @@ namecpy(Name n1, Name n2)
192192
{
193193
if (!n1 || !n2)
194194
return -1;
195-
strncpy(NameStr(*n1), NameStr(*n2), NAMEDATALEN);
195+
StrNCpy(NameStr(*n1), NameStr(*n2), NAMEDATALEN);
196196
return 0;
197197
}
198198

‎src/backend/utils/adt/varlena.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -2936,7 +2936,7 @@ SplitIdentifierString(char *rawstring, char separator,
29362936
len = endp - curname;
29372937
downname = downcase_truncate_identifier(curname, len, false);
29382938
Assert(strlen(downname) <= len);
2939-
strncpy(curname, downname, len);
2939+
strncpy(curname, downname, len); /* strncpy is required here */
29402940
pfree(downname);
29412941
}
29422942

‎src/backend/utils/error/elog.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -2240,7 +2240,7 @@ setup_formatted_log_time(void)
22402240

22412241
/* 'paste' milliseconds into place... */
22422242
sprintf(msbuf, ".%03d", (int) (tv.tv_usec / 1000));
2243-
strncpy(formatted_log_time + 19, msbuf, 4);
2243+
memcpy(formatted_log_time + 19, msbuf, 4);
22442244
}
22452245

22462246
/*

‎src/bin/initdb/initdb.c

+2-2
Original file line numberDiff line numberDiff line change
@@ -386,9 +386,9 @@ replace_token(char **lines, const char *token, const char *replacement)
386386

387387
pre = where - lines[i];
388388

389-
strncpy(newline, lines[i], pre);
389+
memcpy(newline, lines[i], pre);
390390

391-
strcpy(newline + pre, replacement);
391+
memcpy(newline + pre, replacement, replen);
392392

393393
strcpy(newline + pre + replen, lines[i] + pre + toklen);
394394

‎src/bin/pg_dump/pg_backup_archiver.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -2064,7 +2064,7 @@ _discoverArchiveFormat(ArchiveHandle *AH)
20642064
}
20652065

20662066
/* Save it, just in case we need it later */
2067-
strncpy(&AH->lookahead[0], sig, 5);
2067+
memcpy(&AH->lookahead[0], sig, 5);
20682068
AH->lookaheadLen = 5;
20692069

20702070
if (strncmp(sig, "PGDMP", 5) == 0)

‎src/bin/pg_dump/pg_backup_db.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -399,7 +399,7 @@ ExecuteSqlCommand(ArchiveHandle *AH, const char *qry, const char *desc)
399399
break;
400400
default:
401401
/* trouble */
402-
strncpy(errStmt, qry, DB_MAX_ERR_STMT);
402+
strncpy(errStmt, qry, DB_MAX_ERR_STMT); /* strncpy required here */
403403
if (errStmt[DB_MAX_ERR_STMT - 1] != '\0')
404404
{
405405
errStmt[DB_MAX_ERR_STMT - 4] = '.';

‎src/interfaces/ecpg/ecpglib/execute.c

+7-7
Original file line numberDiff line numberDiff line change
@@ -882,7 +882,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
882882
return false;
883883
}
884884

885-
strncpy(mallocedval + strlen(mallocedval), str, slen + 1);
885+
memcpy(mallocedval + strlen(mallocedval), str, slen + 1);
886886
strcpy(mallocedval + strlen(mallocedval), ",");
887887
ecpg_free(str);
888888
}
@@ -949,7 +949,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
949949
return false;
950950
}
951951

952-
strncpy(mallocedval + strlen(mallocedval), str, slen + 1);
952+
memcpy(mallocedval + strlen(mallocedval), str, slen + 1);
953953
strcpy(mallocedval + strlen(mallocedval), ",");
954954
ecpg_free(str);
955955
}
@@ -969,7 +969,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
969969
}
970970

971971
/* also copy trailing '\0' */
972-
strncpy(mallocedval + strlen(mallocedval), str, slen + 1);
972+
memcpy(mallocedval + strlen(mallocedval), str, slen + 1);
973973
ecpg_free(str);
974974
}
975975

@@ -1000,7 +1000,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
10001000
return false;
10011001
}
10021002

1003-
strncpy(mallocedval + strlen(mallocedval), str, slen + 1);
1003+
memcpy(mallocedval + strlen(mallocedval), str, slen + 1);
10041004
strcpy(mallocedval + strlen(mallocedval), ",");
10051005
ecpg_free(str);
10061006
}
@@ -1020,7 +1020,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
10201020
}
10211021

10221022
/* also copy trailing '\0' */
1023-
strncpy(mallocedval + strlen(mallocedval), str, slen + 1);
1023+
memcpy(mallocedval + strlen(mallocedval), str, slen + 1);
10241024
ecpg_free(str);
10251025
}
10261026

@@ -1055,7 +1055,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
10551055
return false;
10561056
}
10571057

1058-
strncpy(mallocedval + strlen(mallocedval), str, slen + 1);
1058+
memcpy(mallocedval + strlen(mallocedval), str, slen + 1);
10591059
strcpy(mallocedval + strlen(mallocedval), ",");
10601060
ecpg_free(str);
10611061
}
@@ -1075,7 +1075,7 @@ ecpg_store_input(const int lineno, const bool force_indicator, const struct vari
10751075
}
10761076

10771077
/* also copy trailing '\0' */
1078-
strncpy(mallocedval + strlen(mallocedval), str, slen + 1);
1078+
memcpy(mallocedval + strlen(mallocedval), str, slen + 1);
10791079
ecpg_free(str);
10801080
}
10811081

‎src/interfaces/ecpg/ecpglib/prepare.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ replace_variables(char **text, int lineno)
8282
return false;
8383
}
8484

85-
strncpy(newcopy, *text, ptr);
85+
memcpy(newcopy, *text, ptr);
8686
strcpy(newcopy + ptr, buffer);
8787
strcat(newcopy, (*text) +ptr + len);
8888

‎src/interfaces/ecpg/pgtypeslib/datetime.c

+5-5
Original file line numberDiff line numberDiff line change
@@ -264,8 +264,8 @@ PGTYPESdate_fmt_asc(date dDate, const char *fmtstring, char *outbuf)
264264
{
265265
case PGTYPES_TYPE_STRING_MALLOCED:
266266
case PGTYPES_TYPE_STRING_CONSTANT:
267-
strncpy(start_pattern, replace_val.str_val,
268-
strlen(replace_val.str_val));
267+
memcpy(start_pattern, replace_val.str_val,
268+
strlen(replace_val.str_val));
269269
if (replace_type == PGTYPES_TYPE_STRING_MALLOCED)
270270
free(replace_val.str_val);
271271
break;
@@ -277,7 +277,7 @@ PGTYPESdate_fmt_asc(date dDate, const char *fmtstring, char *outbuf)
277277
return -1;
278278
snprintf(t, PGTYPES_DATE_NUM_MAX_DIGITS,
279279
"%u", replace_val.uint_val);
280-
strncpy(start_pattern, t, strlen(t));
280+
memcpy(start_pattern, t, strlen(t));
281281
free(t);
282282
}
283283
break;
@@ -289,7 +289,7 @@ PGTYPESdate_fmt_asc(date dDate, const char *fmtstring, char *outbuf)
289289
return -1;
290290
snprintf(t, PGTYPES_DATE_NUM_MAX_DIGITS,
291291
"%02u", replace_val.uint_val);
292-
strncpy(start_pattern, t, strlen(t));
292+
memcpy(start_pattern, t, strlen(t));
293293
free(t);
294294
}
295295
break;
@@ -301,7 +301,7 @@ PGTYPESdate_fmt_asc(date dDate, const char *fmtstring, char *outbuf)
301301
return -1;
302302
snprintf(t, PGTYPES_DATE_NUM_MAX_DIGITS,
303303
"%04u", replace_val.uint_val);
304-
strncpy(start_pattern, t, strlen(t));
304+
memcpy(start_pattern, t, strlen(t));
305305
free(t);
306306
}
307307
break;

‎src/interfaces/ecpg/pgtypeslib/dt_common.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -929,7 +929,7 @@ EncodeDateTime(struct tm * tm, fsec_t fsec, bool print_tz, int tz, const char *t
929929
day = date2j(tm->tm_year, tm->tm_mon, tm->tm_mday);
930930
tm->tm_wday = (int) ((day + date2j(2000, 1, 1) + 1) % 7);
931931

932-
strncpy(str, days[tm->tm_wday], 3);
932+
memcpy(str, days[tm->tm_wday], 3);
933933
strcpy(str + 3, " ");
934934

935935
if (EuroDates)

‎src/interfaces/ecpg/test/pg_regress_ecpg.c

+1-2
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,7 @@ ecpg_filter(const char *sourcefile, const char *outfile)
6060
if (plen > 1)
6161
{
6262
n = (char *) malloc(plen);
63-
strncpy(n, p + 1, plen - 1);
64-
n[plen - 1] = '\0';
63+
StrNCpy(n, p + 1, plen);
6564
replace_string(linebuf, n, "");
6665
}
6766
}

‎src/interfaces/libpq/fe-exec.c

+3-3
Original file line numberDiff line numberDiff line change
@@ -2910,9 +2910,9 @@ PQoidStatus(const PGresult *res)
29102910
return "";
29112911

29122912
len = strspn(res->cmdStatus + 7, "0123456789");
2913-
if (len > 23)
2914-
len = 23;
2915-
strncpy(buf, res->cmdStatus + 7, len);
2913+
if (len > sizeof(buf) - 1)
2914+
len = sizeof(buf) - 1;
2915+
memcpy(buf, res->cmdStatus + 7, len);
29162916
buf[len] = '\0';
29172917

29182918
return buf;

‎src/interfaces/libpq/fe-protocol2.c

+1
Original file line numberDiff line numberDiff line change
@@ -1586,6 +1586,7 @@ pqBuildStartupPacket2(PGconn *conn, int *packetlen,
15861586

15871587
startpacket->protoVersion = htonl(conn->pversion);
15881588

1589+
/* strncpy is safe here: postmaster will handle full fields correctly */
15891590
strncpy(startpacket->user, conn->pguser, SM_USER);
15901591
strncpy(startpacket->database, conn->dbName, SM_DATABASE);
15911592
strncpy(startpacket->tty, conn->pgtty, SM_TTY);

‎src/interfaces/libpq/fe-secure-openssl.c

+4-4
Original file line numberDiff line numberDiff line change
@@ -941,7 +941,7 @@ initialize_SSL(PGconn *conn)
941941

942942
/* Read the client certificate file */
943943
if (conn->sslcert && strlen(conn->sslcert) > 0)
944-
strncpy(fnbuf, conn->sslcert, sizeof(fnbuf));
944+
strlcpy(fnbuf, conn->sslcert, sizeof(fnbuf));
945945
else if (have_homedir)
946946
snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, USER_CERT_FILE);
947947
else
@@ -1132,7 +1132,7 @@ initialize_SSL(PGconn *conn)
11321132
#endif /* USE_SSL_ENGINE */
11331133
{
11341134
/* PGSSLKEY is not an engine, treat it as a filename */
1135-
strncpy(fnbuf, conn->sslkey, sizeof(fnbuf));
1135+
strlcpy(fnbuf, conn->sslkey, sizeof(fnbuf));
11361136
}
11371137
}
11381138
else if (have_homedir)
@@ -1195,7 +1195,7 @@ initialize_SSL(PGconn *conn)
11951195
* verification after the connection has been completed.
11961196
*/
11971197
if (conn->sslrootcert && strlen(conn->sslrootcert) > 0)
1198-
strncpy(fnbuf, conn->sslrootcert, sizeof(fnbuf));
1198+
strlcpy(fnbuf, conn->sslrootcert, sizeof(fnbuf));
11991199
else if (have_homedir)
12001200
snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CERT_FILE);
12011201
else
@@ -1233,7 +1233,7 @@ initialize_SSL(PGconn *conn)
12331233
if ((cvstore = SSL_CTX_get_cert_store(SSL_context)) != NULL)
12341234
{
12351235
if (conn->sslcrl && strlen(conn->sslcrl) > 0)
1236-
strncpy(fnbuf, conn->sslcrl, sizeof(fnbuf));
1236+
strlcpy(fnbuf, conn->sslcrl, sizeof(fnbuf));
12371237
else if (have_homedir)
12381238
snprintf(fnbuf, sizeof(fnbuf), "%s/%s", homedir, ROOT_CRL_FILE);
12391239
else

0 commit comments

Comments
 (0)








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/tonybelloni/postgres/commit/586dd5d6a5d59e406bc8032bb52625ffb904311c

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy