From 5324b1e386c33e25450f8aab57930abd88bb8f39 Mon Sep 17 00:00:00 2001 From: Yi Jin Date: Wed, 19 Dec 2018 13:26:27 -0800 Subject: [PATCH 1/8] add support for setting compression level through environment variable ZSTD_CLEVEL --- programs/zstdcli.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/programs/zstdcli.c b/programs/zstdcli.c index e69661d5..9737ae02 100644 --- a/programs/zstdcli.c +++ b/programs/zstdcli.c @@ -28,6 +28,7 @@ #include "platform.h" /* IS_CONSOLE, PLATFORM_POSIX_VERSION */ #include "util.h" /* UTIL_HAS_CREATEFILELIST, UTIL_createFileList */ #include /* fprintf(), stdin, stdout, stderr */ +#include /* getenv */ #include /* strcmp, strlen */ #include /* errno */ #include "fileio.h" /* stdinmark, stdoutmark, ZSTD_EXTENSION */ @@ -452,6 +453,28 @@ static void printVersion(void) #endif } +/* Environment variables for parameter setting */ +#define ENV_CLEVEL "ZSTD_CLEVEL" + +/* functions that pick up environment variables */ +int init_cLevel() { + const char *env = getenv(ENV_CLEVEL); + if (env) { + int sign = 1; + if (*env == '-') { + sign = -1; + env++; + } else if (*env == '+') { + env++; + } + + if ((*env>='0') && (*env<='9')) + return sign * readU32FromChar(&env); + } + + return ZSTDCLI_CLEVEL_DEFAULT; +} + typedef enum { zom_compress, zom_decompress, zom_test, zom_bench, zom_train, zom_list } zstd_operation_mode; #define CLEAN_RETURN(i) { operationResult = (i); goto _end; } @@ -493,7 +516,7 @@ int main(int argCount, const char* argv[]) size_t blockSize = 0; zstd_operation_mode operation = zom_compress; ZSTD_compressionParameters compressionParams; - int cLevel = ZSTDCLI_CLEVEL_DEFAULT; + int cLevel = init_cLevel(); int cLevelLast = -1000000000; unsigned recursive = 0; unsigned memLimit = 0; From 26a9ae3f5f0f8a563ce108e757b327b0dd9beae7 Mon Sep 17 00:00:00 2001 From: Yi Jin Date: Wed, 19 Dec 2018 16:45:42 -0800 Subject: [PATCH 2/8] refactor readU32FromChar(...), improve init_cLevel(...), and add env var ZSTD_CLEVEL tests --- programs/zstdcli.c | 57 ++++++++++++++++++++++++++++++++-------------- tests/playTests.sh | 12 ++++++++++ 2 files changed, 52 insertions(+), 17 deletions(-) diff --git a/programs/zstdcli.c b/programs/zstdcli.c index 9737ae02..d892c6aa 100644 --- a/programs/zstdcli.c +++ b/programs/zstdcli.c @@ -234,32 +234,44 @@ static void errorOut(const char* msg) DISPLAY("%s \n", msg); exit(1); } -/*! readU32FromChar() : - * @return : unsigned integer value read from input in `char` format. +/*! _readU32FromChar() : + * @return 0 if success, and store the result in *value. * allows and interprets K, KB, KiB, M, MB and MiB suffix. * Will also modify `*stringPtr`, advancing it to position where it stopped reading. - * Note : function will exit() program if digit sequence overflows */ -static unsigned readU32FromChar(const char** stringPtr) + * @return 1 if an overflow error occurs */ +static int _readU32FromChar(const char** stringPtr, unsigned* value) { - const char errorMsg[] = "error: numeric value too large"; + static unsigned const max = (((unsigned)(-1)) / 10) - 1; unsigned result = 0; while ((**stringPtr >='0') && (**stringPtr <='9')) { - unsigned const max = (((unsigned)(-1)) / 10) - 1; - if (result > max) errorOut(errorMsg); + if (result > max) return 1; // overflow error result *= 10, result += **stringPtr - '0', (*stringPtr)++ ; } if ((**stringPtr=='K') || (**stringPtr=='M')) { unsigned const maxK = ((unsigned)(-1)) >> 10; - if (result > maxK) errorOut(errorMsg); + if (result > maxK) return 1; // overflow error result <<= 10; if (**stringPtr=='M') { - if (result > maxK) errorOut(errorMsg); + if (result > maxK) return 1; // overflow error result <<= 10; } (*stringPtr)++; /* skip `K` or `M` */ if (**stringPtr=='i') (*stringPtr)++; if (**stringPtr=='B') (*stringPtr)++; } + *value = result; + return 0; +} + +/*! readU32FromChar() : + * @return : unsigned integer value read from input in `char` format. + * allows and interprets K, KB, KiB, M, MB and MiB suffix. + * Will also modify `*stringPtr`, advancing it to position where it stopped reading. + * Note : function will exit() program if digit sequence overflows */ +static unsigned readU32FromChar(const char** stringPtr) { + static const char errorMsg[] = "error: numeric value too large"; + unsigned result; + if (_readU32FromChar(stringPtr, &result)) { errorOut(errorMsg); } return result; } @@ -458,18 +470,28 @@ static void printVersion(void) /* functions that pick up environment variables */ int init_cLevel() { - const char *env = getenv(ENV_CLEVEL); + const char* const env = getenv(ENV_CLEVEL); if (env) { + const char *ptr = env; int sign = 1; - if (*env == '-') { + if (*ptr == '-') { sign = -1; - env++; - } else if (*env == '+') { - env++; + ptr++; + } else if (*ptr == '+') { + ptr++; } - if ((*env>='0') && (*env<='9')) - return sign * readU32FromChar(&env); + if ((*ptr>='0') && (*ptr<='9')) { + unsigned absLevel; + if (_readU32FromChar(&ptr, &absLevel)) { + DISPLAYLEVEL(2, "Ignore environment variable %s=%s: numeric value too large\n", ENV_CLEVEL, env); + return ZSTDCLI_CLEVEL_DEFAULT; + } else if (*ptr == 0) { + return sign * absLevel; + } + } + + DISPLAYLEVEL(2, "Ignore environment variable %s=%s: not a valid integer value\n", ENV_CLEVEL, env); } return ZSTDCLI_CLEVEL_DEFAULT; @@ -490,6 +512,8 @@ typedef enum { zom_compress, zom_decompress, zom_test, zom_bench, zom_train, zom int main(int argCount, const char* argv[]) { + g_displayOut = stderr; + int argNb, followLinks = 0, forceStdout = 0, @@ -550,7 +574,6 @@ int main(int argCount, const char* argv[]) (void)memLimit; /* not used when ZSTD_NODECOMPRESS set */ if (filenameTable==NULL) { DISPLAY("zstd: %s \n", strerror(errno)); exit(1); } filenameTable[0] = stdinmark; - g_displayOut = stderr; programName = lastNameFromPath(programName); #ifdef ZSTD_MULTITHREAD nbWorkers = 1; diff --git a/tests/playTests.sh b/tests/playTests.sh index 7758f46e..cb4c6d5a 100755 --- a/tests/playTests.sh +++ b/tests/playTests.sh @@ -122,6 +122,18 @@ $ZSTD --fast=5000000000 -f tmp && die "too large numeric value : must fail" $ZSTD -c --fast=0 tmp > $INTOVOID && die "--fast must not accept value 0" $ECHO "test : too large numeric argument" $ZSTD --fast=9999999999 -f tmp && die "should have refused numeric value" +$ECHO "test : set compression level with environment variable ZSTD_CLEVEL" +ZSTD_CLEVEL=12 $ZSTD -f tmp # positive compression level +ZSTD_CLEVEL=-12 $ZSTD -f tmp # negative compression level +ZSTD_CLEVEL=+12 $ZSTD -f tmp # valid: verbose '+' sign +ZSTD_CLEVEL= $ZSTD -f tmp # empty env var, warn and revert to default setting +ZSTD_CLEVEL=- $ZSTD -f tmp # malformed env var, warn and revert to default setting +ZSTD_CLEVEL=a $ZSTD -f tmp # malformed env var, warn and revert to default setting +ZSTD_CLEVEL=+a $ZSTD -f tmp # malformed env var, warn and revert to default setting +ZSTD_CLEVEL=3a7 $ZSTD -f tmp # malformed env var, warn and revert to default setting +ZSTD_CLEVEL=50000000000 $ZSTD -f tmp # numeric value too large, warn and revert to default setting +$ECHO "test : override ZSTD_CLEVEL with command line option" +ZSTD_CLEVEL=12 $ZSTD --fast=3 -f tmp # overridden by command line option $ECHO "test : compress to stdout" $ZSTD tmp -c > tmpCompressed $ZSTD tmp --stdout > tmpCompressed # long command format From 29c7d82390bc052bd8d40b1801d96d1655bcc5d8 Mon Sep 17 00:00:00 2001 From: Yi Jin Date: Wed, 19 Dec 2018 17:06:56 -0800 Subject: [PATCH 3/8] add a section on restricted support of environment variables to README.md --- programs/README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/programs/README.md b/programs/README.md index ca9056ea..baf90e88 100644 --- a/programs/README.md +++ b/programs/README.md @@ -172,6 +172,10 @@ Benchmark arguments : --priority=rt : set process priority to real-time ``` +#### Restricted usage of Environment Variables +Using environment variables to set compression/decompression parameters has security implications. Therefore, +we intentionally restrict its usage. Currently, only `ZSTD_CLEVEL` is supported for setting compression level. +If the value of `ZSTD_CLEVEL` is not a valid integer, it will be ignored with a warning message. #### Long distance matching mode The long distance matching mode, enabled with `--long`, is designed to improve From cdc7bbf8b22204c6e78db5302461a82a160f2358 Mon Sep 17 00:00:00 2001 From: Yi Jin Date: Wed, 19 Dec 2018 17:09:54 -0800 Subject: [PATCH 4/8] edit README.md --- programs/README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/programs/README.md b/programs/README.md index baf90e88..afbebaa1 100644 --- a/programs/README.md +++ b/programs/README.md @@ -176,6 +176,7 @@ Benchmark arguments : Using environment variables to set compression/decompression parameters has security implications. Therefore, we intentionally restrict its usage. Currently, only `ZSTD_CLEVEL` is supported for setting compression level. If the value of `ZSTD_CLEVEL` is not a valid integer, it will be ignored with a warning message. +Note that command line options will override corresponding environment variable settings. #### Long distance matching mode The long distance matching mode, enabled with `--long`, is designed to improve From 0700335f5789b78e010cf2781d461982e3a397a9 Mon Sep 17 00:00:00 2001 From: Yi Jin Date: Wed, 19 Dec 2018 17:38:28 -0800 Subject: [PATCH 5/8] change int init_cLevel() to int init_cLevel(void) --- programs/zstdcli.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/programs/zstdcli.c b/programs/zstdcli.c index d892c6aa..4270bc1f 100644 --- a/programs/zstdcli.c +++ b/programs/zstdcli.c @@ -469,7 +469,7 @@ static void printVersion(void) #define ENV_CLEVEL "ZSTD_CLEVEL" /* functions that pick up environment variables */ -int init_cLevel() { +int init_cLevel(void) { const char* const env = getenv(ENV_CLEVEL); if (env) { const char *ptr = env; @@ -541,6 +541,7 @@ int main(int argCount, const char* argv[]) zstd_operation_mode operation = zom_compress; ZSTD_compressionParameters compressionParams; int cLevel = init_cLevel(); + printf("init cLevel = %d\n", cLevel); int cLevelLast = -1000000000; unsigned recursive = 0; unsigned memLimit = 0; @@ -1107,6 +1108,7 @@ int main(int argCount, const char* argv[]) if (adaptMin > cLevel) cLevel = adaptMin; if (adaptMax < cLevel) cLevel = adaptMax; + printf("final cLevel = %d\n", cLevel); if ((filenameIdx==1) && outFileName) operationResult = FIO_compressFilename(outFileName, filenameTable[0], dictFileName, cLevel, compressionParams); else From 30ffc24ad7b763166e4583f234fc3688944a7225 Mon Sep 17 00:00:00 2001 From: Yi Jin Date: Wed, 19 Dec 2018 17:49:04 -0800 Subject: [PATCH 6/8] fix the code ahead of declaration issue --- programs/zstdcli.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/programs/zstdcli.c b/programs/zstdcli.c index 4270bc1f..33b200ef 100644 --- a/programs/zstdcli.c +++ b/programs/zstdcli.c @@ -512,8 +512,6 @@ typedef enum { zom_compress, zom_decompress, zom_test, zom_bench, zom_train, zom int main(int argCount, const char* argv[]) { - g_displayOut = stderr; - int argNb, followLinks = 0, forceStdout = 0, @@ -540,8 +538,7 @@ int main(int argCount, const char* argv[]) size_t blockSize = 0; zstd_operation_mode operation = zom_compress; ZSTD_compressionParameters compressionParams; - int cLevel = init_cLevel(); - printf("init cLevel = %d\n", cLevel); + int cLevel; int cLevelLast = -1000000000; unsigned recursive = 0; unsigned memLimit = 0; @@ -575,6 +572,8 @@ int main(int argCount, const char* argv[]) (void)memLimit; /* not used when ZSTD_NODECOMPRESS set */ if (filenameTable==NULL) { DISPLAY("zstd: %s \n", strerror(errno)); exit(1); } filenameTable[0] = stdinmark; + g_displayOut = stderr; + cLevel = init_cLevel(); programName = lastNameFromPath(programName); #ifdef ZSTD_MULTITHREAD nbWorkers = 1; @@ -1108,7 +1107,6 @@ int main(int argCount, const char* argv[]) if (adaptMin > cLevel) cLevel = adaptMin; if (adaptMax < cLevel) cLevel = adaptMax; - printf("final cLevel = %d\n", cLevel); if ((filenameIdx==1) && outFileName) operationResult = FIO_compressFilename(outFileName, filenameTable[0], dictFileName, cLevel, compressionParams); else From f7b1841b6f758bd88eab3710c77cf36471a344e0 Mon Sep 17 00:00:00 2001 From: Yi Jin Date: Wed, 19 Dec 2018 17:56:45 -0800 Subject: [PATCH 7/8] make init_cLevel(...) static --- programs/zstdcli.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/programs/zstdcli.c b/programs/zstdcli.c index 33b200ef..eca7438f 100644 --- a/programs/zstdcli.c +++ b/programs/zstdcli.c @@ -469,7 +469,7 @@ static void printVersion(void) #define ENV_CLEVEL "ZSTD_CLEVEL" /* functions that pick up environment variables */ -int init_cLevel(void) { +static int init_cLevel(void) { const char* const env = getenv(ENV_CLEVEL); if (env) { const char *ptr = env; From 9b2d70885019cf1ec9c1688079a1dba19c68df96 Mon Sep 17 00:00:00 2001 From: Yi Jin Date: Wed, 19 Dec 2018 23:41:18 -0800 Subject: [PATCH 8/8] change function name: _readU32FromChar() -> readU32FromCharChecked() --- programs/zstdcli.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/programs/zstdcli.c b/programs/zstdcli.c index eca7438f..64dad110 100644 --- a/programs/zstdcli.c +++ b/programs/zstdcli.c @@ -234,12 +234,12 @@ static void errorOut(const char* msg) DISPLAY("%s \n", msg); exit(1); } -/*! _readU32FromChar() : +/*! readU32FromCharChecked() : * @return 0 if success, and store the result in *value. * allows and interprets K, KB, KiB, M, MB and MiB suffix. * Will also modify `*stringPtr`, advancing it to position where it stopped reading. * @return 1 if an overflow error occurs */ -static int _readU32FromChar(const char** stringPtr, unsigned* value) +static int readU32FromCharChecked(const char** stringPtr, unsigned* value) { static unsigned const max = (((unsigned)(-1)) / 10) - 1; unsigned result = 0; @@ -271,7 +271,7 @@ static int _readU32FromChar(const char** stringPtr, unsigned* value) static unsigned readU32FromChar(const char** stringPtr) { static const char errorMsg[] = "error: numeric value too large"; unsigned result; - if (_readU32FromChar(stringPtr, &result)) { errorOut(errorMsg); } + if (readU32FromCharChecked(stringPtr, &result)) { errorOut(errorMsg); } return result; } @@ -483,15 +483,15 @@ static int init_cLevel(void) { if ((*ptr>='0') && (*ptr<='9')) { unsigned absLevel; - if (_readU32FromChar(&ptr, &absLevel)) { - DISPLAYLEVEL(2, "Ignore environment variable %s=%s: numeric value too large\n", ENV_CLEVEL, env); + if (readU32FromCharChecked(&ptr, &absLevel)) { + DISPLAYLEVEL(2, "Ignore environment variable setting %s=%s: numeric value too large\n", ENV_CLEVEL, env); return ZSTDCLI_CLEVEL_DEFAULT; } else if (*ptr == 0) { return sign * absLevel; } } - DISPLAYLEVEL(2, "Ignore environment variable %s=%s: not a valid integer value\n", ENV_CLEVEL, env); + DISPLAYLEVEL(2, "Ignore environment variable setting %s=%s: not a valid integer value\n", ENV_CLEVEL, env); } return ZSTDCLI_CLEVEL_DEFAULT;