nginx / unit

NGINX Unit - universal web app server - a lightweight and versatile open source server that simplifies the application stack by natively executing application code across eight different programming language runtimes.
https://unit.nginx.org
Apache License 2.0
5.26k stars 322 forks source link

Fix check in nxt_conf_json_parse_value() #1164

Closed ac000 closed 4 months ago

ac000 commented 4 months ago

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.

ac000 commented 4 months ago
$ git range-diff 02da5d35...27e0f22f
1:  02da5d35 ! 1:  27e0f22f Fix check in nxt_conf_json_parse_value()
    @@ Metadata
     Author: Andrew Clayton <a.clayton@nginx.com>

      ## Commit message ##
    -    Fix check in nxt_conf_json_parse_value()
    +    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
    @@ Commit message
         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: Reviewed-by: Zhidao Hong <z.hong@f5.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

      ## src/nxt_conf.c ##
ac000 commented 4 months ago

Fix copy paste snafu

$ git range-diff 27e0f22f...8ff606fb
1:  27e0f22f ! 1:  8ff606fb Configuration: Fix check in nxt_conf_json_parse_value()
    @@ Commit message
         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: Reviewed-by: Zhidao Hong <z.hong@f5.com>
    +    Reviewed-by: Zhidao Hong <z.hong@f5.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

      ## src/nxt_conf.c ##