Skip to content

Commit

Permalink
Configuration: Fix check in nxt_conf_json_parse_value()
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
ac000 committed Feb 29, 2024
1 parent 23e807d commit 8ff606f
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion src/nxt_conf.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down

0 comments on commit 8ff606f

Please sign in to comment.