summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorranshid <88133677+ranshid@users.noreply.github.com>2022-07-18 10:56:26 +0300
committerGitHub <noreply@github.com>2022-07-18 10:56:26 +0300
commiteacca729a55501508c434bab30c2432e58728aee (patch)
tree2efbd90ac59743bf39ef8f7b2ee9ba9e9f1c1e39
parent82b82035553cdbaf81983f91e0402edc8de764ab (diff)
downloadredis-eacca729a55501508c434bab30c2432e58728aee.tar.gz
Avoid using unsafe C functions (#10932)
replace use of: sprintf --> snprintf strcpy/strncpy --> redis_strlcpy strcat/strncat --> redis_strlcat **why are we making this change?** Much of the code uses some unsafe variants or deprecated buffer handling functions. While most cases are probably not presenting any issue on the known path programming errors and unterminated strings might lead to potential buffer overflows which are not covered by tests. **As part of this PR we change** 1. added implementation for redis_strlcpy and redis_strlcat based on the strl implementation: https://linux.die.net/man/3/strl 2. change all occurrences of use of sprintf with use of snprintf 3. change occurrences of use of strcpy/strncpy with redis_strlcpy 4. change occurrences of use of strcat/strncat with redis_strlcat 5. change the behavior of ll2string/ull2string/ld2string so that it will always place null termination ('\0') on the output buffer in the first index. this was done in order to make the use of these functions more safe in cases were the user will not check the output returned by them (for example in rdbRemoveTempFile) 6. we added a compiler directive to issue a deprecation error in case a use of sprintf/strcpy/strcat is found during compilation which will result in error during compile time. However keep in mind that since the deprecation attribute is not supported on all compilers, this is expected to fail during push workflows. **NOTE:** while this is only an initial milestone. We might also consider using the *_s implementation provided by the C11 Extensions (however not yet widly supported). I would also suggest to start looking at static code analyzers to track unsafe use cases. For example LLVM clang checker supports security.insecureAPI.DeprecatedOrUnsafeBufferHandling which can help locate unsafe function usage. https://clang.llvm.org/docs/analyzer/checkers.html#security-insecureapi-deprecatedorunsafebufferhandling-c The main reason not to onboard it at this stage is that the alternative excepted by clang is to use the C11 extensions which are not always supported by stdlib.
-rw-r--r--src/Makefile8
-rw-r--r--src/anet.c5
-rw-r--r--src/cluster.c6
-rw-r--r--src/config.c9
-rw-r--r--src/config.h10
-rw-r--r--src/dict.c2
-rw-r--r--src/endianconv.c6
-rw-r--r--src/listpack.c8
-rw-r--r--src/module.c5
-rw-r--r--src/networking.c2
-rw-r--r--src/rdb.c8
-rw-r--r--src/redis-cli.c37
-rw-r--r--src/server.c36
-rw-r--r--src/strl.c86
-rw-r--r--src/util.c81
-rw-r--r--src/util.h3
-rw-r--r--src/ziplist.c22
-rw-r--r--src/zmalloc.c2
-rw-r--r--tests/modules/basics.c4
19 files changed, 227 insertions, 113 deletions
diff --git a/src/Makefile b/src/Makefile
index e4f7d9068..097ef2454 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -31,7 +31,7 @@ ifneq (,$(findstring FreeBSD,$(uname_S)))
STD+=-Wno-c11-extensions
endif
endif
-WARN=-Wall -W -Wno-missing-field-initializers
+WARN=-Wall -W -Wno-missing-field-initializers -Werror=deprecated-declarations
OPT=$(OPTIMIZATION)
# Detect if the compiler supports C11 _Atomic.
@@ -316,11 +316,11 @@ endif
REDIS_SERVER_NAME=redis-server$(PROG_SUFFIX)
REDIS_SENTINEL_NAME=redis-sentinel$(PROG_SUFFIX)
-REDIS_SERVER_OBJ=adlist.o quicklist.o ae.o anet.o dict.o server.o sds.o zmalloc.o lzf_c.o lzf_d.o pqsort.o zipmap.o sha1.o ziplist.o release.o networking.o util.o object.o db.o replication.o rdb.o t_string.o t_list.o t_set.o t_zset.o t_hash.o config.o aof.o pubsub.o multi.o debug.o sort.o intset.o syncio.o cluster.o crc16.o endianconv.o slowlog.o eval.o bio.o rio.o rand.o memtest.o syscheck.o crcspeed.o crc64.o bitops.o sentinel.o notify.o setproctitle.o blocked.o hyperloglog.o latency.o sparkline.o redis-check-rdb.o redis-check-aof.o geo.o lazyfree.o module.o evict.o expire.o geohash.o geohash_helper.o childinfo.o defrag.o siphash.o rax.o t_stream.o listpack.o localtime.o lolwut.o lolwut5.o lolwut6.o acl.o tracking.o connection.o tls.o sha256.o timeout.o setcpuaffinity.o monotonic.o mt19937-64.o resp_parser.o call_reply.o script_lua.o script.o functions.o function_lua.o commands.o
+REDIS_SERVER_OBJ=adlist.o quicklist.o ae.o anet.o dict.o server.o sds.o zmalloc.o lzf_c.o lzf_d.o pqsort.o zipmap.o sha1.o ziplist.o release.o networking.o util.o object.o db.o replication.o rdb.o t_string.o t_list.o t_set.o t_zset.o t_hash.o config.o aof.o pubsub.o multi.o debug.o sort.o intset.o syncio.o cluster.o crc16.o endianconv.o slowlog.o eval.o bio.o rio.o rand.o memtest.o syscheck.o crcspeed.o crc64.o bitops.o sentinel.o notify.o setproctitle.o blocked.o hyperloglog.o latency.o sparkline.o redis-check-rdb.o redis-check-aof.o geo.o lazyfree.o module.o evict.o expire.o geohash.o geohash_helper.o childinfo.o defrag.o siphash.o rax.o t_stream.o listpack.o localtime.o lolwut.o lolwut5.o lolwut6.o acl.o tracking.o connection.o tls.o sha256.o timeout.o setcpuaffinity.o monotonic.o mt19937-64.o resp_parser.o call_reply.o script_lua.o script.o functions.o function_lua.o commands.o strl.o
REDIS_CLI_NAME=redis-cli$(PROG_SUFFIX)
-REDIS_CLI_OBJ=anet.o adlist.o dict.o redis-cli.o zmalloc.o release.o ae.o redisassert.o crcspeed.o crc64.o siphash.o crc16.o monotonic.o cli_common.o mt19937-64.o
+REDIS_CLI_OBJ=anet.o adlist.o dict.o redis-cli.o zmalloc.o release.o ae.o redisassert.o crcspeed.o crc64.o siphash.o crc16.o monotonic.o cli_common.o mt19937-64.o strl.o
REDIS_BENCHMARK_NAME=redis-benchmark$(PROG_SUFFIX)
-REDIS_BENCHMARK_OBJ=ae.o anet.o redis-benchmark.o adlist.o dict.o zmalloc.o redisassert.o release.o crcspeed.o crc64.o siphash.o crc16.o monotonic.o cli_common.o mt19937-64.o
+REDIS_BENCHMARK_OBJ=ae.o anet.o redis-benchmark.o adlist.o dict.o zmalloc.o redisassert.o release.o crcspeed.o crc64.o siphash.o crc16.o monotonic.o cli_common.o mt19937-64.o strl.o
REDIS_CHECK_RDB_NAME=redis-check-rdb$(PROG_SUFFIX)
REDIS_CHECK_AOF_NAME=redis-check-aof$(PROG_SUFFIX)
ALL_SOURCES=$(sort $(patsubst %.o,%.c,$(REDIS_SERVER_OBJ) $(REDIS_CLI_OBJ) $(REDIS_BENCHMARK_OBJ)))
diff --git a/src/anet.c b/src/anet.c
index 4ea201df5..753f2fe42 100644
--- a/src/anet.c
+++ b/src/anet.c
@@ -48,6 +48,7 @@
#include "anet.h"
#include "config.h"
+#include "util.h"
#define UNUSED(x) (void)(x)
@@ -388,7 +389,7 @@ int anetUnixGenericConnect(char *err, const char *path, int flags)
return ANET_ERR;
sa.sun_family = AF_LOCAL;
- strncpy(sa.sun_path,path,sizeof(sa.sun_path)-1);
+ redis_strlcpy(sa.sun_path,path,sizeof(sa.sun_path));
if (flags & ANET_CONNECT_NONBLOCK) {
if (anetNonBlock(err,s) != ANET_OK) {
close(s);
@@ -497,7 +498,7 @@ int anetUnixServer(char *err, char *path, mode_t perm, int backlog)
memset(&sa,0,sizeof(sa));
sa.sun_family = AF_LOCAL;
- strncpy(sa.sun_path,path,sizeof(sa.sun_path)-1);
+ redis_strlcpy(sa.sun_path,path,sizeof(sa.sun_path));
if (anetListen(err,s,(struct sockaddr*)&sa,sizeof(sa),backlog) == ANET_ERR)
return ANET_ERR;
if (perm)
diff --git a/src/cluster.c b/src/cluster.c
index ae353e1dd..09c388013 100644
--- a/src/cluster.c
+++ b/src/cluster.c
@@ -577,8 +577,7 @@ void clusterUpdateMyselfIp(void) {
* duplicating the string. This way later we can check if
* the address really changed. */
prev_ip = zstrdup(prev_ip);
- strncpy(myself->ip,server.cluster_announce_ip,NET_IP_STR_LEN-1);
- myself->ip[NET_IP_STR_LEN-1] = '\0';
+ redis_strlcpy(myself->ip,server.cluster_announce_ip,NET_IP_STR_LEN);
} else {
myself->ip[0] = '\0'; /* Force autodetection. */
}
@@ -2807,8 +2806,7 @@ void clusterBuildMessageHdr(clusterMsg *hdr, int type) {
* first byte is zero, they'll do auto discovery. */
memset(hdr->myip,0,NET_IP_STR_LEN);
if (server.cluster_announce_ip) {
- strncpy(hdr->myip,server.cluster_announce_ip,NET_IP_STR_LEN-1);
- hdr->myip[NET_IP_STR_LEN-1] = '\0';
+ redis_strlcpy(hdr->myip,server.cluster_announce_ip,NET_IP_STR_LEN);
}
/* Handle cluster-announce-[tls-|bus-]port. */
diff --git a/src/config.c b/src/config.c
index 115446c67..0ffe65a61 100644
--- a/src/config.c
+++ b/src/config.c
@@ -1971,8 +1971,7 @@ static int enumConfigSet(standardConfig *config, sds *argv, int argc, const char
}
sdsrange(enumerr,0,-3); /* Remove final ", ". */
- strncpy(loadbuf, enumerr, LOADBUF_SIZE);
- loadbuf[LOADBUF_SIZE - 1] = '\0';
+ redis_strlcpy(loadbuf, enumerr, LOADBUF_SIZE);
sdsfree(enumerr);
*err = loadbuf;
@@ -2499,7 +2498,7 @@ static int updateMaxclients(const char **err) {
adjustOpenFilesLimit();
if (server.maxclients != new_maxclients) {
static char msg[128];
- sprintf(msg, "The operating system is not able to handle the specified number of clients, try with %d", server.maxclients);
+ snprintf(msg, sizeof(msg), "The operating system is not able to handle the specified number of clients, try with %d", server.maxclients);
*err = msg;
return 0;
}
@@ -2896,7 +2895,7 @@ static sds getConfigLatencyTrackingInfoPercentilesOutputOption(standardConfig *c
sds buf = sdsempty();
for (int j = 0; j < server.latency_tracking_info_percentiles_len; j++) {
char fbuf[128];
- size_t len = sprintf(fbuf, "%f", server.latency_tracking_info_percentiles[j]);
+ size_t len = snprintf(fbuf, sizeof(fbuf), "%f", server.latency_tracking_info_percentiles[j]);
len = trimDoubleString(fbuf, len);
buf = sdscatlen(buf, fbuf, len);
if (j != server.latency_tracking_info_percentiles_len-1)
@@ -2918,7 +2917,7 @@ void rewriteConfigLatencyTrackingInfoPercentilesOutputOption(standardConfig *con
} else {
for (int j = 0; j < server.latency_tracking_info_percentiles_len; j++) {
char fbuf[128];
- size_t len = sprintf(fbuf, " %f", server.latency_tracking_info_percentiles[j]);
+ size_t len = snprintf(fbuf, sizeof(fbuf), " %f", server.latency_tracking_info_percentiles[j]);
len = trimDoubleString(fbuf, len);
line = sdscatlen(line, fbuf, len);
}
diff --git a/src/config.h b/src/config.h
index 6baa8bd0f..dafbf15a9 100644
--- a/src/config.h
+++ b/src/config.h
@@ -309,4 +309,14 @@ int pthread_setname_np(const char *name);
void setcpuaffinity(const char *cpulist);
#endif
+/* deprecate unsafe functions
+ *
+ * NOTE: We do not use the poison pragma since it
+ * will error on stdlib definitions in files as well*/
+#if (__GNUC__ && __GNUC__ >= 4) && !defined __APPLE__
+int sprintf(char *str, const char *format, ...) __attribute__((deprecated("please avoid use of unsafe C functions. prefer use of snprintf instead")));
+char *strcpy(char *restrict dest, const char *src) __attribute__((deprecated("please avoid use of unsafe C functions. prefer use of redis_strlcpy instead")));
+char *strcat(char *restrict dest, const char *restrict src) __attribute__((deprecated("please avoid use of unsafe C functions. prefer use of redis_strlcat instead")));
+#endif
+
#endif
diff --git a/src/dict.c b/src/dict.c
index cb6850027..e96a1ed2b 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -1210,7 +1210,7 @@ char *stringFromLongLong(long long value) {
int len;
char *s;
- len = sprintf(buf,"%lld",value);
+ len = snprintf(buf,sizeof(buf),"%lld",value);
s = zmalloc(len+1);
memcpy(s, buf, len);
s[len] = '\0';
diff --git a/src/endianconv.c b/src/endianconv.c
index 56f3d738a..8eb6b2228 100644
--- a/src/endianconv.c
+++ b/src/endianconv.c
@@ -112,15 +112,15 @@ int endianconvTest(int argc, char *argv[], int flags) {
UNUSED(argv);
UNUSED(flags);
- sprintf(buf,"ciaoroma");
+ snprintf(buf,sizeof(buf),"ciaoroma");
memrev16(buf);
printf("%s\n", buf);
- sprintf(buf,"ciaoroma");
+ snprintf(buf,sizeof(buf),"ciaoroma");
memrev32(buf);
printf("%s\n", buf);
- sprintf(buf,"ciaoroma");
+ snprintf(buf,sizeof(buf),"ciaoroma");
memrev64(buf);
printf("%s\n", buf);
diff --git a/src/listpack.c b/src/listpack.c
index 833c9709e..7fe86d297 100644
--- a/src/listpack.c
+++ b/src/listpack.c
@@ -1967,7 +1967,7 @@ int listpackTest(int argc, char *argv[], int flags) {
char buf[32];
int i,len;
for (i = 0; i < 1000; i++) {
- len = sprintf(buf, "%d", i);
+ len = snprintf(buf, sizeof(buf), "%d", i);
lp = lpAppend(lp, (unsigned char*)buf, len);
}
for (i = 0; i < 1000; i++) {
@@ -2271,13 +2271,13 @@ int listpackTest(int argc, char *argv[], int flags) {
} else {
switch(rand() % 3) {
case 0:
- buflen = sprintf(buf,"%lld",(0LL + rand()) >> 20);
+ buflen = snprintf(buf,sizeof(buf),"%lld",(0LL + rand()) >> 20);
break;
case 1:
- buflen = sprintf(buf,"%lld",(0LL + rand()));
+ buflen = snprintf(buf,sizeof(buf),"%lld",(0LL + rand()));
break;
case 2:
- buflen = sprintf(buf,"%lld",(0LL + rand()) << 20);
+ buflen = snprintf(buf,sizeof(buf),"%lld",(0LL + rand()) << 20);
break;
default:
assert(NULL);
diff --git a/src/module.c b/src/module.c
index 3ea8433f8..8e14f7c5d 100644
--- a/src/module.c
+++ b/src/module.c
@@ -8095,7 +8095,7 @@ int RM_GetClusterNodeInfo(RedisModuleCtx *ctx, const char *id, char *ip, char *m
return REDISMODULE_ERR;
}
- if (ip) strncpy(ip,node->ip,NET_IP_STR_LEN);
+ if (ip) redis_strlcpy(ip,node->ip,NET_IP_STR_LEN);
if (master_id) {
/* If the information is not available, the function will set the
@@ -11468,8 +11468,7 @@ int moduleVerifyConfigName(sds name) {
static char configerr[CONFIG_ERR_SIZE];
static void propagateErrorString(RedisModuleString *err_in, const char **err) {
if (err_in) {
- strncpy(configerr, err_in->ptr, CONFIG_ERR_SIZE);
- configerr[CONFIG_ERR_SIZE - 1] = '\0';
+ redis_strlcpy(configerr, err_in->ptr, CONFIG_ERR_SIZE);
decrRefCount(err_in);
*err = configerr;
}
diff --git a/src/networking.c b/src/networking.c
index b04eec4a6..8e68300c2 100644
--- a/src/networking.c
+++ b/src/networking.c
@@ -799,7 +799,7 @@ void setDeferredAggregateLen(client *c, void *node, long length, char prefix) {
}
char lenstr[128];
- size_t lenstr_len = sprintf(lenstr, "%c%ld\r\n", prefix, length);
+ size_t lenstr_len = snprintf(lenstr, sizeof(lenstr), "%c%ld\r\n", prefix, length);
setDeferredReply(c, node, lenstr, lenstr_len);
}
diff --git a/src/rdb.c b/src/rdb.c
index b53fbdb21..79c156f18 100644
--- a/src/rdb.c
+++ b/src/rdb.c
@@ -1508,10 +1508,10 @@ void rdbRemoveTempFile(pid_t childpid, int from_signal) {
char pid[32];
/* Generate temp rdb file name using async-signal safe functions. */
- int pid_len = ll2string(pid, sizeof(pid), childpid);
- strcpy(tmpfile, "temp-");
- strncpy(tmpfile+5, pid, pid_len);
- strcpy(tmpfile+5+pid_len, ".rdb");
+ ll2string(pid, sizeof(pid), childpid);
+ redis_strlcpy(tmpfile, "temp-", sizeof(tmpfile));
+ redis_strlcat(tmpfile, pid, sizeof(tmpfile));
+ redis_strlcat(tmpfile, ".rdb", sizeof(tmpfile));
if (from_signal) {
/* bg_unlink is not async-signal-safe, but in this case we don't really
diff --git a/src/redis-cli.c b/src/redis-cli.c
index a1a258f46..5f3bc78d4 100644
--- a/src/redis-cli.c
+++ b/src/redis-cli.c
@@ -294,6 +294,7 @@ static long getLongInfoField(char *info, char *field);
/*------------------------------------------------------------------------------
* Utility functions
*--------------------------------------------------------------------------- */
+size_t redis_strlcpy(char *dst, const char *src, size_t dsize);
static void cliPushHandler(void *, void *);
@@ -3264,7 +3265,7 @@ static int clusterManagerCheckRedisReply(clusterManagerNode *n,
if (is_err) {
if (err != NULL) {
*err = zmalloc((r->len + 1) * sizeof(char));
- strcpy(*err, r->str);
+ redis_strlcpy(*err, r->str,(r->len + 1));
} else CLUSTER_MANAGER_PRINT_REPLY_ERROR(n, r->str);
}
return 0;
@@ -3424,7 +3425,7 @@ static redisReply *clusterManagerGetNodeRedisInfo(clusterManagerNode *node,
if (info->type == REDIS_REPLY_ERROR) {
if (err != NULL) {
*err = zmalloc((info->len + 1) * sizeof(char));
- strcpy(*err, info->str);
+ redis_strlcpy(*err, info->str,(info->len + 1));
}
freeReplyObject(info);
return NULL;
@@ -4001,7 +4002,7 @@ static int clusterManagerSetSlot(clusterManagerNode *node1,
success = 0;
if (err != NULL) {
*err = zmalloc((reply->len + 1) * sizeof(char));
- strcpy(*err, reply->str);
+ redis_strlcpy(*err, reply->str,(reply->len + 1));
} else CLUSTER_MANAGER_PRINT_REPLY_ERROR(node1, reply->str);
goto cleanup;
}
@@ -4275,7 +4276,7 @@ static int clusterManagerMigrateKeysInSlot(clusterManagerNode *source,
success = 0;
if (err != NULL) {
*err = zmalloc((reply->len + 1) * sizeof(char));
- strcpy(*err, reply->str);
+ redis_strlcpy(*err, reply->str,(reply->len + 1));
CLUSTER_MANAGER_PRINT_REPLY_ERROR(source, *err);
}
goto next;
@@ -4387,7 +4388,7 @@ static int clusterManagerMigrateKeysInSlot(clusterManagerNode *source,
if (migrate_reply != NULL) {
if (err) {
*err = zmalloc((migrate_reply->len + 1) * sizeof(char));
- strcpy(*err, migrate_reply->str);
+ redis_strlcpy(*err, migrate_reply->str, (migrate_reply->len + 1));
}
printf("\n");
CLUSTER_MANAGER_PRINT_REPLY_ERROR(source,
@@ -4504,7 +4505,7 @@ static int clusterManagerFlushNodeConfig(clusterManagerNode *node, char **err) {
if (reply == NULL || (is_err = (reply->type == REDIS_REPLY_ERROR))) {
if (is_err && err != NULL) {
*err = zmalloc((reply->len + 1) * sizeof(char));
- strcpy(*err, reply->str);
+ redis_strlcpy(*err, reply->str, (reply->len + 1));
}
success = 0;
/* If the cluster did not already joined it is possible that
@@ -8557,7 +8558,7 @@ static long getLongInfoField(char *info, char *field) {
/* Convert number of bytes into a human readable string of the form:
* 100B, 2G, 100M, 4K, and so forth. */
-void bytesToHuman(char *s, long long n) {
+void bytesToHuman(char *s, size_t size, long long n) {
double d;
if (n < 0) {
@@ -8567,17 +8568,17 @@ void bytesToHuman(char *s, long long n) {
}
if (n < 1024) {
/* Bytes */
- sprintf(s,"%lldB",n);
+ snprintf(s,size,"%lldB",n);
return;
} else if (n < (1024*1024)) {
d = (double)n/(1024);
- sprintf(s,"%.2fK",d);
+ snprintf(s,size,"%.2fK",d);
} else if (n < (1024LL*1024*1024)) {
d = (double)n/(1024*1024);
- sprintf(s,"%.2fM",d);
+ snprintf(s,size,"%.2fM",d);
} else if (n < (1024LL*1024*1024*1024)) {
d = (double)n/(1024LL*1024*1024);
- sprintf(s,"%.2fG",d);
+ snprintf(s,size,"%.2fG",d);
}
}
@@ -8610,38 +8611,38 @@ static void statMode(void) {
for (j = 0; j < 20; j++) {
long k;
- sprintf(buf,"db%d:keys",j);
+ snprintf(buf,sizeof(buf),"db%d:keys",j);
k = getLongInfoField(reply->str,buf);
if (k == LONG_MIN) continue;
aux += k;
}
- sprintf(buf,"%ld",aux);
+ snprintf(buf,sizeof(buf),"%ld",aux);
printf("%-11s",buf);
/* Used memory */
aux = getLongInfoField(reply->str,"used_memory");
- bytesToHuman(buf,aux);
+ bytesToHuman(buf,sizeof(buf),aux);
printf("%-8s",buf);
/* Clients */
aux = getLongInfoField(reply->str,"connected_clients");
- sprintf(buf,"%ld",aux);
+ snprintf(buf,sizeof(buf),"%ld",aux);
printf(" %-8s",buf);
/* Blocked (BLPOPPING) Clients */
aux = getLongInfoField(reply->str,"blocked_clients");
- sprintf(buf,"%ld",aux);
+ snprintf(buf,sizeof(buf),"%ld",aux);
printf("%-8s",buf);
/* Requests */
aux = getLongInfoField(reply->str,"total_commands_processed");
- sprintf(buf,"%ld (+%ld)",aux,requests == 0 ? 0 : aux-requests);
+ snprintf(buf,sizeof(buf),"%ld (+%ld)",aux,requests == 0 ? 0 : aux-requests);
printf("%-19s",buf);
requests = aux;
/* Connections */
aux = getLongInfoField(reply->str,"total_connections_received");
- sprintf(buf,"%ld",aux);
+ snprintf(buf,sizeof(buf),"%ld",aux);
printf(" %-12s",buf);
/* Children */
diff --git a/src/server.c b/src/server.c
index de32ff847..3910c6614 100644
--- a/src/server.c
+++ b/src/server.c
@@ -5024,30 +5024,30 @@ NULL
/* Convert an amount of bytes into a human readable string in the form
* of 100B, 2G, 100M, 4K, and so forth. */
-void bytesToHuman(char *s, unsigned long long n) {
+void bytesToHuman(char *s, size_t size, unsigned long long n) {
double d;
if (n < 1024) {
/* Bytes */
- sprintf(s,"%lluB",n);
+ snprintf(s,size,"%lluB",n);
} else if (n < (1024*1024)) {
d = (double)n/(1024);
- sprintf(s,"%.2fK",d);
+ snprintf(s,size,"%.2fK",d);
} else if (n < (1024LL*1024*1024)) {
d = (double)n/(1024*1024);
- sprintf(s,"%.2fM",d);
+ snprintf(s,size,"%.2fM",d);
} else if (n < (1024LL*1024*1024*1024)) {
d = (double)n/(1024LL*1024*1024);
- sprintf(s,"%.2fG",d);
+ snprintf(s,size,"%.2fG",d);
} else if (n < (1024LL*1024*1024*1024*1024)) {
d = (double)n/(1024LL*1024*1024*1024);
- sprintf(s,"%.2fT",d);
+ snprintf(s,size,"%.2fT",d);
} else if (n < (1024LL*1024*1024*1024*1024*1024)) {
d = (double)n/(1024LL*1024*1024*1024*1024);
- sprintf(s,"%.2fP",d);
+ snprintf(s,size,"%.2fP",d);
} else {
/* Let's hope we never need this */
- sprintf(s,"%lluB",n);
+ snprintf(s,size,"%lluB",n);
}
}
@@ -5056,8 +5056,8 @@ sds fillPercentileDistributionLatencies(sds info, const char* histogram_name, st
info = sdscatfmt(info,"latency_percentiles_usec_%s:",histogram_name);
for (int j = 0; j < server.latency_tracking_info_percentiles_len; j++) {
char fbuf[128];
- size_t len = sprintf(fbuf, "%f", server.latency_tracking_info_percentiles[j]);
- len = trimDoubleString(fbuf, len);
+ size_t len = snprintf(fbuf, sizeof(fbuf), "%f", server.latency_tracking_info_percentiles[j]);
+ trimDoubleString(fbuf, len);
info = sdscatprintf(info,"p%s=%.3f", fbuf,
((double)hdr_value_at_percentile(histogram,server.latency_tracking_info_percentiles[j]))/1000.0f);
if (j != server.latency_tracking_info_percentiles_len-1)
@@ -5359,14 +5359,14 @@ sds genRedisInfoString(dict *section_dict, int all_sections, int everything) {
if (zmalloc_used > server.stat_peak_memory)
server.stat_peak_memory = zmalloc_used;
- bytesToHuman(hmem,zmalloc_used);
- bytesToHuman(peak_hmem,server.stat_peak_memory);
- bytesToHuman(total_system_hmem,total_system_mem);
- bytesToHuman(used_memory_lua_hmem,memory_lua);
- bytesToHuman(used_memory_vm_total_hmem,memory_functions + memory_lua);
- bytesToHuman(used_memory_scripts_hmem,mh->lua_caches + mh->functions_caches);
- bytesToHuman(used_memory_rss_hmem,server.cron_malloc_stats.process_rss);
- bytesToHuman(maxmemory_hmem,server.maxmemory);
+ bytesToHuman(hmem,sizeof(hmem),zmalloc_used);
+ bytesToHuman(peak_hmem,sizeof(peak_hmem),server.stat_peak_memory);
+ bytesToHuman(total_system_hmem,sizeof(total_system_hmem),total_system_mem);
+ bytesToHuman(used_memory_lua_hmem,sizeof(used_memory_lua_hmem),memory_lua);
+ bytesToHuman(used_memory_vm_total_hmem,sizeof(used_memory_vm_total_hmem),memory_functions + memory_lua);
+ bytesToHuman(used_memory_scripts_hmem,sizeof(used_memory_scripts_hmem),mh->lua_caches + mh->functions_caches);
+ bytesToHuman(used_memory_rss_hmem,sizeof(used_memory_rss_hmem),server.cron_malloc_stats.process_rss);
+ bytesToHuman(maxmemory_hmem,sizeof(maxmemory_hmem),server.maxmemory);
if (sections++) info = sdscat(info,"\r\n");
info = sdscatprintf(info,
diff --git a/src/strl.c b/src/strl.c
new file mode 100644
index 000000000..f73cf796c
--- /dev/null
+++ b/src/strl.c
@@ -0,0 +1,86 @@
+/*
+ * Copyright (c) 1998, 2015 Todd C. Miller <millert@openbsd.org>
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+#include <string.h>
+
+/*
+ * Copy string src to buffer dst of size dsize. At most dsize-1
+ * chars will be copied. Always NUL terminates (unless dsize == 0).
+ * Returns strlen(src); if retval >= dsize, truncation occurred.
+ */
+size_t
+redis_strlcpy(char *dst, const char *src, size_t dsize)
+{
+ const char *osrc = src;
+ size_t nleft = dsize;
+
+ /* Copy as many bytes as will fit. */
+ if (nleft != 0) {
+ while (--nleft != 0) {
+ if ((*dst++ = *src++) == '\0')
+ break;
+ }
+ }
+
+ /* Not enough room in dst, add NUL and traverse rest of src. */
+ if (nleft == 0) {
+ if (dsize != 0)
+ *dst = '\0'; /* NUL-terminate dst */
+ while (*src++)
+ ;
+ }
+
+ return(src - osrc - 1); /* count does not include NUL */
+}
+
+/*
+ * Appends src to string dst of size dsize (unlike strncat, dsize is the
+ * full size of dst, not space left). At most dsize-1 characters
+ * will be copied. Always NUL terminates (unless dsize <= strlen(dst)).
+ * Returns strlen(src) + MIN(dsize, strlen(initial dst)).
+ * If retval >= dsize, truncation occurred.
+ */
+size_t
+redis_strlcat(char *dst, const char *src, size_t dsize)
+{
+ const char *odst = dst;
+ const char *osrc = src;
+ size_t n = dsize;
+ size_t dlen;
+
+ /* Find the end of dst and adjust bytes left but don't go past end. */
+ while (n-- != 0 && *dst != '\0')
+ dst++;
+ dlen = dst - odst;
+ n = dsize - dlen;
+
+ if (n-- == 0)
+ return(dlen + strlen(src));
+ while (*src != '\0') {
+ if (n != 0) {
+ *dst++ = *src;
+ n--;
+ }
+ src++;
+ }
+ *dst = '\0';
+
+ return(dlen + (src - osrc)); /* count does not include NUL */
+}
+
+
+
+
+
diff --git a/src/util.c b/src/util.c
index 4b534e9a6..092814251 100644
--- a/src/util.c
+++ b/src/util.c
@@ -330,7 +330,7 @@ int ll2string(char *dst, size_t dstlen, long long svalue) {
value = ((unsigned long long) LLONG_MAX)+1;
}
if (dstlen < 2)
- return 0;
+ goto err;
negative = 1;
dst[0] = '-';
dst++;
@@ -343,6 +343,12 @@ int ll2string(char *dst, size_t dstlen, long long svalue) {
int length = ull2string(dst, dstlen, value);
if (length == 0) return 0;
return length + negative;
+
+err:
+ /* force add Null termination */
+ if (dstlen > 0)
+ dst[0] = '\0';
+ return 0;
}
/* Convert a unsigned long long into a string. Returns the number of
@@ -363,7 +369,7 @@ int ull2string(char *dst, size_t dstlen, unsigned long long value) {
/* Check length. */
uint32_t length = digits10(value);
- if (length >= dstlen) return 0;
+ if (length >= dstlen) goto err;;
/* Null term. */
uint32_t next = length - 1;
@@ -384,8 +390,12 @@ int ull2string(char *dst, size_t dstlen, unsigned long long value) {
dst[next] = digits[i + 1];
dst[next - 1] = digits[i];
}
-
return length;
+err:
+ /* force add Null termination */
+ if (dstlen > 0)
+ dst[0] = '\0';
+ return 0;
}
/* Convert a string into a long long. Returns 1 if the string could be parsed
@@ -643,7 +653,7 @@ int ld2string(char *buf, size_t len, long double value, ld2string_mode mode) {
if (isinf(value)) {
/* Libc in odd systems (Hi Solaris!) will format infinite in a
* different way, so better to handle it in an explicit way. */
- if (len < 5) return 0; /* No room. 5 is "-inf\0" */
+ if (len < 5) goto err; /* No room. 5 is "-inf\0" */
if (value > 0) {
memcpy(buf,"inf",3);
l = 3;
@@ -655,11 +665,11 @@ int ld2string(char *buf, size_t len, long double value, ld2string_mode mode) {
switch (mode) {
case LD_STR_AUTO:
l = snprintf(buf,len,"%.17Lg",value);
- if (l+1 > len) return 0; /* No room. */
+ if (l+1 > len) goto err;; /* No room. */
break;
case LD_STR_HEX:
l = snprintf(buf,len,"%La",value);
- if (l+1 > len) return 0; /* No room. */
+ if (l+1 > len) goto err; /* No room. */
break;
case LD_STR_HUMAN:
/* We use 17 digits precision since with 128 bit floats that precision
@@ -668,7 +678,7 @@ int ld2string(char *buf, size_t len, long double value, ld2string_mode mode) {
* decimal numbers will be represented in a way that when converted
* back into a string are exactly the same as what the user typed.) */
l = snprintf(buf,len,"%.17Lf",value);
- if (l+1 > len) return 0; /* No room. */
+ if (l+1 > len) goto err; /* No room. */
/* Now remove trailing zeroes after the '.' */
if (strchr(buf,'.') != NULL) {
char *p = buf+l-1;
@@ -683,11 +693,16 @@ int ld2string(char *buf, size_t len, long double value, ld2string_mode mode) {
l = 1;
}
break;
- default: return 0; /* Invalid mode. */
+ default: goto err; /* Invalid mode. */
}
}
buf[l] = '\0';
return l;
+err:
+ /* force add Null termination */
+ if (len > 0)
+ buf[0] = '\0';
+ return 0;
}
/* Get random bytes, attempts to get an initial seed from /dev/urandom and
@@ -980,53 +995,53 @@ static void test_string2ll(void) {
long long v;
/* May not start with +. */
- strcpy(buf,"+1");
+ redis_strlcpy(buf,"+1",sizeof(buf));
assert(string2ll(buf,strlen(buf),&v) == 0);
/* Leading space. */
- strcpy(buf," 1");
+ redis_strlcpy(buf," 1",sizeof(buf));
assert(string2ll(buf,strlen(buf),&v) == 0);
/* Trailing space. */
- strcpy(buf,"1 ");
+ redis_strlcpy(buf,"1 ",sizeof(buf));
assert(string2ll(buf,strlen(buf),&v) == 0);
/* May not start with 0. */
- strcpy(buf,"01");
+ redis_strlcpy(buf,"01",sizeof(buf));
assert(string2ll(buf,strlen(buf),&v) == 0);
- strcpy(buf,"-1");
+ redis_strlcpy(buf,"-1",sizeof(buf));
assert(string2ll(buf,strlen(buf),&v) == 1);
assert(v == -1);
- strcpy(buf,"0");
+ redis_strlcpy(buf,"0",sizeof(buf));
assert(string2ll(buf,strlen(buf),&v) == 1);
assert(v == 0);
- strcpy(buf,"1");
+ redis_strlcpy(buf,"1",sizeof(buf));
assert(string2ll(buf,strlen(buf),&v) == 1);
assert(v == 1);
- strcpy(buf,"99");
+ redis_strlcpy(buf,"99",sizeof(buf));
assert(string2ll(buf,strlen(buf),&v) == 1);
assert(v == 99);
- strcpy(buf,"-99");
+ redis_strlcpy(buf,"-99",sizeof(buf));
assert(string2ll(buf,strlen(buf),&v) == 1);
assert(v == -99);
- strcpy(buf,"-9223372036854775808");
+ redis_strlcpy(buf,"-9223372036854775808",sizeof(buf));
assert(string2ll(buf,strlen(buf),&v) == 1);
assert(v == LLONG_MIN);
- strcpy(buf,"-9223372036854775809"); /* overflow */
+ redis_strlcpy(buf,"-9223372036854775809",sizeof(buf)); /* overflow */
assert(string2ll(buf,strlen(buf),&v) == 0);
- strcpy(buf,"9223372036854775807");
+ redis_strlcpy(buf,"9223372036854775807",sizeof(buf));
assert(string2ll(buf,strlen(buf),&v) == 1);
assert(v == LLONG_MAX);
- strcpy(buf,"9223372036854775808"); /* overflow */
+ redis_strlcpy(buf,"9223372036854775808",sizeof(buf)); /* overflow */
assert(string2ll(buf,strlen(buf),&v) == 0);
}
@@ -1035,46 +1050,46 @@ static void test_string2l(void) {
long v;
/* May not start with +. */
- strcpy(buf,"+1");
+ redis_strlcpy(buf,"+1",sizeof(buf));
assert(string2l(buf,strlen(buf),&v) == 0);
/* May not start with 0. */
- strcpy(buf,"01");
+ redis_strlcpy(buf,"01",sizeof(buf));
assert(string2l(buf,strlen(buf),&v) == 0);
- strcpy(buf,"-1");
+ redis_strlcpy(buf,"-1",sizeof(buf));
assert(string2l(buf,strlen(buf),&v) == 1);
assert(v == -1);
- strcpy(buf,"0");
+ redis_strlcpy(buf,"0",sizeof(buf));
assert(string2l(buf,strlen(buf),&v) == 1);
assert(v == 0);
- strcpy(buf,"1");
+ redis_strlcpy(buf,"1",sizeof(buf));
assert(string2l(buf,strlen(buf),&v) == 1);
assert(v == 1);
- strcpy(buf,"99");
+ redis_strlcpy(buf,"99",sizeof(buf));
assert(string2l(buf,strlen(buf),&v) == 1);
assert(v == 99);
- strcpy(buf,"-99");
+ redis_strlcpy(buf,"-99",sizeof(buf));
assert(string2l(buf,strlen(buf),&v) == 1);
assert(v == -99);
#if LONG_MAX != LLONG_MAX
- strcpy(buf,"-2147483648");
+ redis_strlcpy(buf,"-2147483648",sizeof(buf));
assert(string2l(buf,strlen(buf),&v) == 1);
assert(v == LONG_MIN);
- strcpy(buf,"-2147483649"); /* overflow */
+ redis_strlcpy(buf,"-2147483649",sizeof(buf)); /* overflow */
assert(string2l(buf,strlen(buf),&v) == 0);
- strcpy(buf,"2147483647");
+ redis_strlcpy(buf,"2147483647",sizeof(buf));
assert(string2l(buf,strlen(buf),&v) == 1);
assert(v == LONG_MAX);
- strcpy(buf,"2147483648"); /* overflow */
+ redis_strlcpy(buf,"2147483648",sizeof(buf)); /* overflow */
assert(string2l(buf,strlen(buf),&v) == 0);
#endif
}
@@ -1132,3 +1147,5 @@ int utilTest(int argc, char **argv, int flags) {
return 0;
}
#endif
+
+
diff --git a/src/util.h b/src/util.h
index 3cf6c3e98..e009354e8 100644
--- a/src/util.h
+++ b/src/util.h
@@ -87,6 +87,9 @@ int fileExist(char *filename);
sds makePath(char *path, char *filename);
int fsyncFileDir(const char *filename);
+size_t redis_strlcpy(char *dst, const char *src, size_t dsize);
+size_t redis_strlcat(char *dst, const char *src, size_t dsize);
+
#ifdef REDIS_TEST
int utilTest(int argc, char **argv, int flags);
#endif
diff --git a/src/ziplist.c b/src/ziplist.c
index 36556050c..c9b954e0b 100644
--- a/src/ziplist.c
+++ b/src/ziplist.c
@@ -1707,17 +1707,17 @@ static unsigned char *createIntList() {
unsigned char *zl = ziplistNew();
char buf[32];
- sprintf(buf, "100");
+ snprintf(buf, sizeof(buf), "100");
zl = ziplistPush(zl, (unsigned char*)buf, strlen(buf), ZIPLIST_TAIL);
- sprintf(buf, "128000");
+ snprintf(buf, sizeof(buf), "128000");
zl = ziplistPush(zl, (unsigned char*)buf, strlen(buf), ZIPLIST_TAIL);
- sprintf(buf, "-100");
+ snprintf(buf, sizeof(buf), "-100");
zl = ziplistPush(zl, (unsigned char*)buf, strlen(buf), ZIPLIST_HEAD);
- sprintf(buf, "4294967296");
+ snprintf(buf, sizeof(buf), "4294967296");
zl = ziplistPush(zl, (unsigned char*)buf, strlen(buf), ZIPLIST_HEAD);
- sprintf(buf, "non integer");
+ snprintf(buf, sizeof(buf), "non integer");
zl = ziplistPush(zl, (unsigned char*)buf, strlen(buf), ZIPLIST_TAIL);
- sprintf(buf, "much much longer non integer");
+ snprintf(buf,sizeof(buf), "much much longer non integer");
zl = ziplistPush(zl, (unsigned char*)buf, strlen(buf), ZIPLIST_TAIL);
return zl;
}
@@ -2232,7 +2232,7 @@ int ziplistTest(int argc, char **argv, int flags) {
char buf[32];
int i,len;
for (i = 0; i < 1000; i++) {
- len = sprintf(buf,"%d",i);
+ len = snprintf(buf,sizeof(buf),"%d",i);
zl = ziplistPush(zl,(unsigned char*)buf,len,ZIPLIST_TAIL);
}
for (i = 0; i < 1000; i++) {
@@ -2379,13 +2379,13 @@ int ziplistTest(int argc, char **argv, int flags) {
} else {
switch(rand() % 3) {
case 0:
- buflen = sprintf(buf,"%lld",(0LL + rand()) >> 20);
+ buflen = snprintf(buf,sizeof(buf),"%lld",(0LL + rand()) >> 20);
break;
case 1:
- buflen = sprintf(buf,"%lld",(0LL + rand()));
+ buflen = snprintf(buf,sizeof(buf),"%lld",(0LL + rand()));
break;
case 2:
- buflen = sprintf(buf,"%lld",(0LL + rand()) << 20);
+ buflen = snprintf(buf,sizeof(buf),"%lld",(0LL + rand()) << 20);
break;
default:
assert(NULL);
@@ -2414,7 +2414,7 @@ int ziplistTest(int argc, char **argv, int flags) {
assert(ziplistGet(p,&sstr,&slen,&sval));
if (sstr == NULL) {
- buflen = sprintf(buf,"%lld",sval);
+ buflen = snprintf(buf,sizeof(buf),"%lld",sval);
} else {
buflen = slen;
memcpy(buf,sstr,buflen);
diff --git a/src/zmalloc.c b/src/zmalloc.c
index dc9d7cbc4..6d02604f4 100644
--- a/src/zmalloc.c
+++ b/src/zmalloc.c
@@ -609,7 +609,7 @@ int jemalloc_purge() {
unsigned narenas = 0;
size_t sz = sizeof(unsigned);
if (!je_mallctl("arenas.narenas", &narenas, &sz, NULL, 0)) {
- sprintf(tmp, "arena.%d.purge", narenas);
+ snprintf(tmp, sizeof(tmp), "arena.%d.purge", narenas);
if (!je_mallctl(tmp, NULL, 0, NULL, 0))
return 0;
}
diff --git a/tests/modules/basics.c b/tests/modules/basics.c
index 9dbb5a9d4..55b36fa5e 100644
--- a/tests/modules/basics.c
+++ b/tests/modules/basics.c
@@ -283,8 +283,8 @@ int TestCallResp3Double(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
double d = RedisModule_CallReplyDouble(reply);
/* we compare strings, since comparing doubles directly can fail in various architectures, e.g. 32bit */
char got[30], expected[30];
- sprintf(got, "%.17g", d);
- sprintf(expected, "%.17g", 3.141);
+ snprintf(got, sizeof(got), "%.17g", d);
+ snprintf(expected, sizeof(expected), "%.17g", 3.141);
if (strcmp(got, expected) != 0) goto fail;
RedisModule_ReplyWithSimpleString(ctx,"OK");
return REDISMODULE_OK;