search

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.27k stars 322 forks source link

Fixed some issues found by tests with enabled UndefinedBehaviorSanitizer #1109

Closed andrey-zelenkov closed 3 months ago

ac000 commented 5 months ago

For 505e879 ("Fixed undefined behaviour in left shift of int value")

Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
ac000 commented 5 months ago

For e059674 ("Initialize port_impl only when it is needed")

Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
andrey-zelenkov commented 4 months ago

Replaced length vs 0 checks by string vs NULL where requested. 

% git range-diff eabd7c7c...ca37cf1a
 -:  -------- >  1:  ecd57392 Configuration: Add nxt_conf_get_string_dup()
 -:  -------- >  2:  bb376c68 Simplify, by calling nxt_conf_get_string_dup()
 -:  -------- >  3:  46cef09f Configuration: Don't corrupt abstract socket names
 -:  -------- >  4:  9e986704 Configuration: Fix validation of "processes"
 1:  c9f59f4d =  5:  2a2e2bbb Router: match when pattern and tested string are both zero length
 2:  a51f44ad !  6:  3519c47f NJS: avoiding arithmetic ops with NULL pointer in r->args
    @@ Commit message
         SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_js.c:169:52

         Same fix was introduced in NJS:
    -    http://hg.nginx.org/njs/rev/4fba78789fe4
    +    <http://hg.nginx.org/njs/rev/4fba78789fe4>

      ## src/nxt_http_js.c ##
     @@ src/nxt_http_js.c: static njs_int_t
    @@ src/nxt_http_js.c: static njs_int_t
          njs_value_t *value, njs_value_t *setval, njs_value_t *retval)
      {
     +    u_char              *start;
    -+    uint8_t             length;
          njs_int_t           ret;
          njs_value_t         *args;
          njs_opaque_value_t  val;
    @@ src/nxt_http_js.c: nxt_http_js_ext_get_args(njs_vm_t *vm, njs_object_prop_t *pro

     -    ret = njs_vm_query_string_parse(vm, r->args->start,
     -                                    r->args->start + r->args->length, args);
    -+    length = r->args->length;
    -+    start = (length != 0) ? r->args->start : (u_char *) "";
    -+    ret = njs_vm_query_string_parse(vm, start, start + length, args);
    ++    start = (r->args->start != NULL) ? r->args->start : (u_char *) "";
    ++    ret = njs_vm_query_string_parse(vm, start, start + r->args->length, args);

          if (ret == NJS_ERROR) {
              return NJS_ERROR;
 3:  505e879f =  7:  890ba34b Fixed undefined behaviour in left shift of int value
 4:  fb6725f6 !  8:  88a17771 Avoiding arithmetic ops with NULL pointer in nxt_http_arguments_parse
    @@ Commit message

      ## src/nxt_http_request.c ##
     @@ src/nxt_http_request.c: nxt_http_arguments_parse(nxt_http_request_t *r)
    +         return NULL;
    +     }

    -     r->args_decoded.start = dst_start;
    - 
    -+    if (nxt_slow_path(r->args->length == 0)) {
    -+        r->args_decoded.length = 0;
    ++    if (nxt_slow_path(r->args->start == NULL)) {
     +        goto end;
     +    }
     +
    -     start = r->args->start;
    -     end = start + r->args->length;
    - 
    +     hash = NXT_HTTP_FIELD_HASH_INIT;
    +     name = NULL;
    +     name_length = 0;
     @@ src/nxt_http_request.c: nxt_http_arguments_parse(nxt_http_request_t *r)
              }
          }
 5:  54235875 !  9:  540257e4 Avoiding arithmetic ops with NULL pointer in nxt_port_mmap_get
    @@ src/nxt_port_memory.c: nxt_port_mmap_get(nxt_task_t *task, nxt_port_mmaps_t *mma

          nxt_thread_mutex_lock(&mmaps->mutex);

    -+    if (nxt_slow_path(mmaps->size == 0)) {
    ++    if (nxt_slow_path(mmaps->elts == NULL)) {
     +        goto end;
     +    }
     +
 6:  e0596744 = 10:  4fc83cb7 Initialize port_impl only when it is needed
 7:  eabd7c7c <  -:  -------- Avoiding arithmetic ops with NULL pointer in nxt_unit_mmap_get
 -:  -------- > 11:  ca37cf1a Avoiding arithmetic ops with NULL pointer in nxt_unit_mmap_get
ac000 commented 4 months ago

If you could add my Reviewed-by tags to the commits I've specified, then I can more easily see which things I've already reviewed.

andrey-zelenkov commented 4 months ago

Rebased and added Reviewed-by: Andrew Clayton <a.clayton@nginx.com> where needed. Was not sure about review status of 27b51c8d so preferred to leave it unreviewed.

% git range-diff ca37cf1a...da0802eb
 ...
 1:  2a2e2bbb = 41:  27b51c8d Router: match when pattern and tested string are both zero length
 2:  3519c47f = 42:  58ef7c74 NJS: avoiding arithmetic ops with NULL pointer in r->args
 3:  890ba34b ! 43:  b706f9a0 Fixed undefined behaviour in left shift of int value
    @@ Commit message

         SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_random.c:151:31

    +    Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
    +
      ## src/nxt_random.c ##
     @@ src/nxt_random.c: nxt_random(nxt_random_t *r)
              nxt_random_stir(r);
 4:  88a17771 = 44:  6ae05623 Avoiding arithmetic ops with NULL pointer in nxt_http_arguments_parse
 5:  540257e4 = 45:  c104472d Avoiding arithmetic ops with NULL pointer in nxt_port_mmap_get
 6:  4fc83cb7 ! 46:  2d16d02a Initialize port_impl only when it is needed
    @@ Commit message

         Found by UndefinedBehaviorSanitizer.

    +    Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
    +
      ## src/nxt_unit.c ##
     @@ src/nxt_unit.c: nxt_unit_request_check_response_port(nxt_unit_request_info_t *req,
          pthread_mutex_lock(&lib->mutex);
 7:  ca37cf1a = 47:  da0802eb Avoiding arithmetic ops with NULL pointer in nxt_unit_mmap_get
andrey-zelenkov commented 4 months ago

Rebased, two more Reviewed-by, check against NULL in ec5a29a1:

% git range-diff da0802eb...b82297c4
 -:  -------- >  1:  bca44630 .mailmap: Map Dylan's GitHub address
 -:  -------- >  2:  f2e64475 Wasm-wc: Register a new Wasm component model language module type
 -:  -------- >  3:  f0782722 Wasm-wc: Add core configuration data structure
 -:  -------- >  4:  20ada4b5 Wasm-wc: Core of initial Wasm component model language module support
 -:  -------- >  5:  a9345dd4 Add a .rustfmt.toml file
 -:  -------- >  6:  79c81772 Wasm-wc: Run src/lib.rs through rustfmt
 -:  -------- >  7:  ac3a54d6 Wasm-wc: Improve request buffer handling
 -:  -------- >  8:  98f808af Wasm-wc: Upgrade to wasmtime 17
 -:  -------- >  9:  60eb6c43 Wasm-wc: Allow to use the 'reactor' adaptor again
 -:  -------- > 10:  8d030139 Wasm-wc: Add Cargo.lock
 -:  -------- > 11:  07a0c9a3 Wasm-wc: Wire up the language module to the config system
 -:  -------- > 12:  da44dc00 Fix alignment of wasm options text in auto/help
 -:  -------- > 13:  4e6d7e87 Wasm-wc: Wire it up to the build system
 -:  -------- > 14:  7702293d Docker: Bump rust version to 1.76.0
 -:  -------- > 15:  1297f6f0 Docker: Add wasm-wasi-component to the wasm target
 -:  -------- > 16:  4c558697 Docker: Re-generate Dockerfile.wasm
 -:  -------- > 17:  7883acc6 Tests: Ruby hook tests unstable for version older 3.0
 -:  -------- > 18:  99da2f3c Tests: check for the AddressSanitizer flag during discovery
 -:  -------- > 19:  dbd9d25f Tests: skip some of TLS reconfiguration tests under AddressSanitizer
 -:  -------- > 20:  cabea47d Tests: renamed test_python_procman.py since it's not Python-specific
 -:  -------- > 21:  3f805bc6 Packages: added wasm-wasi-component module packaging for deb-based distros
 -:  -------- > 22:  7a640556 Packages: added wasm-wasi-component module packaging for rpm-based distros
 -:  -------- > 23:  7b13c306 Wasm-wc: Add nxt_unit.o as a dependency in the auto script
 -:  -------- > 24:  d54af163 Wasm-wc: Use the cargo build output as the make target dependency
 -:  -------- > 25:  2f3c7c2c Update third-party java components to their r -:  -------- > 20:  cabea47d Tests: renamed test_python_procman.py since i
t's not Python-specific
 -:  -------- > 21:  3f805bc6 Packages: added wasm-wasi-component module pa
ckaging for deb-based distros
 -:  -------- > 18:  99da2f3c Tests: check for the AddressSanitizer flag du
ring discovery
 -:  -------- > 19:  dbd9d25f Tests: skip some of TLS reconfiguration tests
 under AddressSanitizer
 -:  -------- > 20:  cabea47d Tests: renamed test_python_procman.py since i
t's not Python-specific
 -:  -------- > 21:  3f805bc6 Packages: added wasm-wasi-component module pa
ckaging for deb-based distros
 -:  -------- > 22:  7a640556 Packages: added wasm-wasi-component module pa
ckaging for rpm-based distros
 -:  -------- > 23:  7b13c306 Wasm-wc: Add nxt_unit.o as a dependency in th
e auto script
 -:  -------- > 24:  d54af163 Wasm-wc: Use the cargo build output as the ma
ke target dependency
 -:  -------- > 25:  2f3c7c2c Update third-party java components to their r
ecent versions
 1:  27b51c8d ! 26:  ec5a29a1 Router: match when pattern and tested string are both zero length
    @@ src/nxt_http_route.c: nxt_http_route_pattern(nxt_http_request_t *r, nxt_http_rou
              return 0;
          }

    -+    if (nxt_slow_path(length == 0)) {
    ++    if (nxt_slow_path(start == NULL)) {
     +        return 1;
     +    }
     +
 2:  58ef7c74 ! 27:  65a80df6 NJS: avoiding arithmetic ops with NULL pointer in r->args
    @@ Commit message
         Same fix was introduced in NJS:
         <http://hg.nginx.org/njs/rev/4fba78789fe4>

    +    Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
    +
      ## src/nxt_http_js.c ##
     @@ src/nxt_http_js.c: static njs_int_t
      nxt_http_js_ext_get_args(njs_vm_t *vm, njs_object_prop_t *prop,
 3:  b706f9a0 = 28:  658d5c0a Fixed undefined behaviour in left shift of int value
 4:  6ae05623 = 29:  589b50c1 Avoiding arithmetic ops with NULL pointer in nxt_http_arguments_parse
 5:  c104472d = 30:  7d10bd59 Avoiding arithmetic ops with NULL pointer in nxt_port_mmap_get
 6:  2d16d02a = 31:  4f374c45 Initialize port_impl only when it is needed
 7:  da0802eb ! 32:  b82297c4 Avoiding arithmetic ops with NULL pointer in nxt_unit_mmap_get
    @@ Commit message

         Found by UndefinedBehaviorSanitizer.

    +    Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
    +
      ## src/nxt_unit.c ##
     @@ src/nxt_unit.c: nxt_unit_mmap_get(nxt_unit_ctx_t *ctx, nxt_unit_port_t *port,
andrey-zelenkov commented 4 months ago 
% git range-diff b82297c4...8eb3653f
1:  ec5a29a1 ! 1:  f80edc37 Router: match when pattern and tested string are both zero length
    @@ Commit message

         SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_route.c:2141:17

    +    Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
    +
      ## src/nxt_http_route.c ##
     @@ src/nxt_http_route.c: nxt_http_route_pattern(nxt_http_request_t *r, nxt_http_route_pattern_t *pattern,
              return 0;
2:  65a80df6 = 2:  592fe518 NJS: avoiding arithmetic ops with NULL pointer in r->args
3:  658d5c0a = 3:  915fd982 Fixed undefined behaviour in left shift of int value
4:  589b50c1 = 4:  1dafe541 Avoiding arithmetic ops with NULL pointer in nxt_http_arguments_parse
5:  7d10bd59 ! 5:  213864c3 Avoiding arithmetic ops with NULL pointer in nxt_port_mmap_get
    @@ Commit message
         Can be reproduced by test/test_settings.py::test_settings_send_timeout
         with enabled UndefinedBehaviorSanitizer.

    +    Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
    +
      ## src/nxt_port_memory.c ##
     @@ src/nxt_port_memory.c: nxt_port_mmap_get(nxt_task_t *task, nxt_port_mmaps_t *mmaps, nxt_chunk_id_t *c,

6:  4f374c45 = 6:  e6571b8e Initialize port_impl only when it is needed
7:  b82297c4 = 7:  8eb3653f Avoiding arithmetic ops with NULL pointer in nxt_unit_mmap_get
andrey-zelenkov commented 4 months ago 
% git range-diff 8eb3653f...887fa467
 -:  -------- >  1:  e2cab032 Remove debug from builds and tests
 -:  -------- >  2:  faa7e792 Packages: Pass CFLAGS to compile wasm modules on all packaging targets
 1:  f80edc37 =  3:  2dfba775 Router: match when pattern and tested string are both zero length
 2:  592fe518 =  4:  edb69d54 NJS: avoiding arithmetic ops with NULL pointer in r->args
 3:  915fd982 =  5:  e9c12c21 Fixed undefined behaviour in left shift of int value
 4:  1dafe541 !  6:  5eeaf1f8 Avoiding arithmetic ops with NULL pointer in nxt_http_arguments_parse
    @@ Commit message

         SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_request.c:961:17

    +    Reviewed-by: Andrew Clayton <a.clayton@nginx.com>
    +
      ## src/nxt_http_request.c ##
     @@ src/nxt_http_request.c: nxt_http_arguments_parse(nxt_http_request_t *r)
              return NULL;
 5:  213864c3 =  7:  e14f64b3 Avoiding arithmetic ops with NULL pointer in nxt_port_mmap_get
 6:  e6571b8e =  8:  3431dfea Initialize port_impl only when it is needed
 7:  8eb3653f =  9:  887fa467 Avoiding arithmetic ops with NULL pointer in nxt_unit_mmap_get