summaryrefslogtreecommitdiff
path: root/src/util.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/util.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/util.c')
-rw-r--r--src/util.c81
1 files changed, 49 insertions, 32 deletions
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
+
+