summaryrefslogtreecommitdiff
path: root/src/redis-cli.c
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 /src/redis-cli.c
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.
Diffstat (limited to 'src/redis-cli.c')
-rw-r--r--src/redis-cli.c37
1 files changed, 19 insertions, 18 deletions
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 */