diff options
author | Andrew Clayton <a.clayton@nginx.com> | 2024-02-28 20:46:18 +0000 |
---|---|---|
committer | Andrew Clayton <a.clayton@nginx.com> | 2024-02-29 13:05:16 +0000 |
commit | 8ff606fbca688072585325ee5a4ddb56cc034575 (patch) | |
tree | 9c74340840eac04c7b1e45cdb88d0645d86f45d2 | |
parent | 23e807dea3a68ead712cb2eab54fba08e545872b (diff) | |
download | unit-8ff606fbca688072585325ee5a4ddb56cc034575.tar.gz unit-8ff606fbca688072585325ee5a4ddb56cc034575.tar.bz2 |
Configuration: Fix check in nxt_conf_json_parse_value()
If we compile Unit with -Wstrict-overflow=5 (as we do with clang) then
we get the following warning
cc -c -pipe -fPIC -fvisibility=hidden -O0 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wstrict-overflow=5 -Wmissing-prototypes -g -I src -I build/include \
\
\
-o build/src/nxt_conf.o \
-MMD -MF build/src/nxt_conf.dep -MT build/src/nxt_conf.o \
src/nxt_conf.c
src/nxt_conf.c: In function ‘nxt_conf_json_parse_value’:
src/nxt_conf.c:1444:5: warning: assuming signed overflow does not occur when changing X +- C1 cmp C2 to X cmp C2 -+ C1 [-Wstrict-overflow]
1444 | if (nxt_fast_path((ch - '0') <= 9)) {
|
Does this actually cause an issue?... well, yes. Using this minimal test
config to show the problem
{
"listeners": {
"[::1]:8080": {
"pass": --100
}
}
}
With the above if () statement that triggers the warning, my assumption
here is that we only want a digit now. '0' - '9'.
ch is a u_char, however if ch is any character with an ASCII code < 48
('0') e.g if ch is '-' (45) then we get 45 - 48 = -3, through arithmetic
conversion, which makes the if () statement true (when it shouldn't) then
at some point we get the following error returned from the controller
{
"error": "Memory allocation failed."
}
Instead of the expected
{
"error": "Invalid JSON.",
"detail": "A valid JSON value is expected here. It must be either a literal (null, true, or false), a number, a string (in double quotes \"\"), an array (with brackets []), or an object (with braces {}).",
"location": {
"offset": 234,
"line": 15,
"column": 27
}
}
Casting the result of (ch - '0') to u_char resolves this issue, this
makes the above calculation come out as 253 (relying on unsigned integer
wraparound) which was probably the intended way for it to work.
Reviewed-by: Zhidao Hong <z.hong@f5.com>
Signed-off-by: Andrew Clayton <a.clayton@nginx.com>
-rw-r--r-- | src/nxt_conf.c | 2 |
1 files changed, 1 insertions, 1 deletions
diff --git a/src/nxt_conf.c b/src/nxt_conf.c index 008cb968..9ae25172 100644 --- a/src/nxt_conf.c +++ b/src/nxt_conf.c @@ -1441,7 +1441,7 @@ nxt_conf_json_parse_value(nxt_mp_t *mp, nxt_conf_value_t *value, u_char *start, goto error; } - if (nxt_fast_path((ch - '0') <= 9)) { + if (nxt_fast_path((u_char)(ch - '0') <= 9)) { p = nxt_conf_json_parse_number(mp, value, start, end, error); if (nxt_slow_path(p == NULL)) { |