Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What competitive advantages? #6

Closed
uasan opened this issue Sep 7, 2017 · 32 comments
Closed

What competitive advantages? #6

uasan opened this issue Sep 7, 2017 · 32 comments

Comments

@uasan
Copy link

uasan commented Sep 7, 2017

Please, describe in more detail in readme, your advantages in comparison with similar Application servers.

For example PHP:

  1. Is it faster than PHP-FPM?
  2. Is it better scaled than PHP-FPM?
  3. Is it planned that the php script will work in the daemon mode, do not reset the state after HTTP request executed?
  4. Is planned implement Websocket protocol?

Thanks.

@stevenh
Copy link

stevenh commented Sep 7, 2017

Same for go it seems to be adding an extra layer without adding any real benefit that I can see?

The only thing seems to be dynamic reconfiguration which can already be done with just straight nginx.

Am I missing something?

@k4ml
Copy link

k4ml commented Sep 7, 2017

What I can see now is the Application model and the JSON API which remind me to Webfaction application model - https://docs.webfaction.com/xmlrpc-api/apiref.html#applications

The application model and the http API would allow us to build something like webfaction hosting panel.

@vovanec
Copy link

vovanec commented Sep 8, 2017

Hi, I want to understand the benefit of using Unit. In case of Python, how it's better than just running e.g. multiple tornado backends on supervisord/monit/whatever and having nginx server in front of it?

@docteurklein
Copy link

docteurklein commented Sep 12, 2017

I'll use this issue to dump my early benchmarks (not saying it's correct and definitive, but I still think it worths sharing).
Maybe my config is not optimal, and in that case it's great in the sense it might open a discussion on how to make it better.

Anyway, I tried to compare an nginx(epoll,multi_accept on) + php-fpm(dynamic pool at max-workers=70) with a nginx-unit php application with 70 workers, serving the same php application (complicated real-life symfony application with pdo_mysql and redis access and stuff).

I used wrk to bench it (4 threads, 10 connections during 20s), here are the results:

nginx+fpm:

 % wrk --timeout=20 --latency -H "Authorization: Bearer $TOKEN" -t4 -c10 -d20s 'http://127.0.0.1:8080/en/contents'
Running 20s test @ http://127.0.0.1:8080/en/contents
  4 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency    53.20ms   22.25ms 146.78ms   65.08%
    Req/Sec    37.55     11.16    80.00     63.38%
  Latency Distribution
     50%   59.99ms
     75%   66.66ms
     90%   74.78ms
     99%   99.82ms
  3006 requests in 20.02s, 441.43MB read
Requests/sec:    150.14
Transfer/sec:     22.05MB

nginx-unit php app

 % wrk --timeout=20 --latency -H "Authorization: Bearer $TOKEN" -t4 -c10 -d20s 'http://127.0.0.1:8300/en/contents'
Running 20s test @ http://127.0.0.1:8300/en/contents
  4 threads and 10 connections
  Thread Stats   Avg      Stdev     Max   +/- Stdev
    Latency   108.47ms   15.14ms 247.84ms   74.57%
    Req/Sec    18.44      4.31    30.00     79.47%
  Latency Distribution
     50%  106.93ms
     75%  116.66ms
     90%  125.13ms
     99%  142.14ms
  1467 requests in 20.01s, 215.16MB read
  Socket errors: connect 0, read 1467, write 0, timeout 0
Requests/sec:     73.30
Transfer/sec:     10.75MB

I noticed some strange CPU usage (1 core was 100% used by unit after the bench was finished, even with 0 traffic).

Hope it helps! I know benchmarks are are often bad, I'm not here to open a war :) just sharing.

PS:

the nginx-unit config:

{
    "listeners": {
        "*:8300": {
            "application": "api"
        }
    },
    "applications": {
        "api": {
            "type": "php",
            "workers": 70,
            "root": "/usr/src/app/web",
            "index": "app.php",
            "script": "app.php"
        }
    }
}

fpm config:

[global]
daemonize = no
error_log = /proc/self/fd/2

[www]
listen = [::]:9000
access.log = /proc/self/fd/2
clear_env = no
catch_workers_output = yes

pm = dynamic
pm.max_children = 70
pm.start_servers = 40
pm.min_spare_servers = 40
pm.max_spare_servers = 50
;pm.max_requests = 500

pm.status_path = /fpm_status
ping.path = /ping

nginx config


user  nginx;
worker_processes auto;

error_log  /dev/stderr debug;
pid        /var/run/nginx.pid;

events {
    worker_connections  4096;
    multi_accept        on;
    use                 epoll;
}

worker_rlimit_nofile 40000;

http {

access_log    /dev/stdout;
include       /etc/nginx/mime.types;
default_type  application/octet-stream;

log_format  proxy  '[$time_local] response_time: $upstream_response_time - status: $upstream_status: "$request"';

sendfile        on;
tcp_nopush      on;
tcp_nodelay     on;

keepalive_requests 100000;

send_timeout            60;
client_header_timeout   60;
client_body_timeout     60;

proxy_send_timeout      60;
proxy_connect_timeout   60;
proxy_read_timeout      60;

fastcgi_send_timeout    60;
fastcgi_connect_timeout 60;
fastcgi_read_timeout    60;



reset_timedout_connection on;

gzip              on;
gzip_vary         on;
gzip_min_length   1000;
gzip_proxied      expired no-cache no-store private auth;
gzip_types        text/plain text/css text/xml text/javascript application/javascript application/json application/xml;
gzip_disable      "MSIE [1-6]\.";

open_file_cache          max=2000 inactive=20s;
open_file_cache_valid    60s;
open_file_cache_min_uses 5;
open_file_cache_errors   off;

proxy_http_version  1.1;
proxy_redirect      off;
proxy_set_header    X-Forwarded-For $remote_addr;
proxy_set_header    Connection "";
proxy_buffering     off;
proxy_buffer_size   128k;
proxy_buffers       100 128k;

fastcgi_buffer_size            128k;
fastcgi_buffers                256 16k;
fastcgi_busy_buffers_size      256k;
fastcgi_temp_file_write_size   256k;
fastcgi_keep_conn              on;

    upstream fpm {
        server api_php_fpm:9000;
        keepalive 128;
    }

    server {

        listen 80 default_server;

        root /usr/src/app/web;

        location /ping {
            include fastcgi_params;
            fastcgi_pass fpm;
        }

        location / {
            add_header 'Access-Control-Allow-Origin' '*';

            try_files $uri /app.php$is_args$args;
        }

        location ~ ^/(app|app_dev|config)\.php(/|$) {

            access_log /dev/stdout proxy;

            include fastcgi_params;
            fastcgi_param SCRIPT_FILENAME /usr/src/app/web/app.php;
            fastcgi_pass fpm;
        }
    }
}

@VBart VBart assigned VBart and unassigned VBart Sep 12, 2017
@thresheek thresheek assigned thresheek and unassigned thresheek Sep 12, 2017
@nshadrin
Copy link
Contributor

Responding to OP:

Competitive advantages are coming in the form of:

  • Consistent application delivery stack regardless of the language used. That is the ability to run PHP, Python, Go (and other languages later) with the same HTTP stack and configuration methods.

  • Dynamic application configuration. Unit does not reload on changes. Only the parts that require restarting (such as application workers) - are restarted on demand. This greatly saves resources and gives you the ability to reconfigure as frequently as you like.

@nshadrin
Copy link
Contributor

Regarding performance testing:
Currently it's too early to for the performance tests. Unit is still in beta, and it has some early features and early versions of the code in many places.
Configuration for tuning is also not available yet.

Be sure, it will be optimized closer to the stable release.

I encourage the community to work with us on optimizing the code, and treat the current performance tests as experiments only.

@VBart
Copy link
Contributor

VBart commented Sep 12, 2017

@docteurklein Actually, the architecture is the best of the best from a performance point of view. There are no compromises. But on the current development stage the code isn't in a good enough shape to show impressive numbers.

@docteurklein
Copy link

docteurklein commented Sep 13, 2017

yes, the embedded php SAPI seems a good idea and could be a huge win from a performance POV I guess.
I really like the idea behind unit, and it could be a good replacement for the nginx+fpm stack.
Again, I was just sharing some numbers (as the OP asked in point 1) and some default config to help people who might later need it.

@MrHamel
Copy link

MrHamel commented Sep 19, 2017

@docteurklein I feel your benchmarks are invalid because you are comparing apples to oranges. You benched raw unitd to nginx+fpm. Your comparisons need to be nginx+unitd and nginx+fpm, or straight fpm and unitd.

@stevenh
Copy link

stevenh commented Sep 19, 2017

While its not quite the same I doubt adding an extra nginx layer in front of unitd will improve throughput?

Also you can't benchmark fpm with standard tools as its fastcgi based not http.

@docteurklein
Copy link

@techhelper1 I tend to agree with @stevenh since unitd is meant to replace nginx for the http layer, and based on http://unit.nginx.org/docs-integration-with-nginx.html, nginx should only be used to handle static file serving.

@OO00O0O
Copy link

OO00O0O commented Oct 6, 2017

@docteurklein So, nginx for static, unit for dynamic? Like how I'm gonna run Symfony website with all images and stuff? I will still need nginx, or unit will have some "static content serving". If it does, will we won't need nginx/apache for simple wordpress sites?

@docteurklein
Copy link

ATM, I think you'll need nginx for static assets only, and a proxy_pass to unitd for the rest. I don't know if unitd plans static assets serving at all.

@docteurklein
Copy link

Maybe an alternative solution would be to make a unitd go app that does only static serving: https://gowebexamples.com/static-files/

never tried tho.

@VBart
Copy link
Contributor

VBart commented Oct 6, 2017

@BunnyHolder Yes, static content serving is planned.
Currently, you can utilize nginx or/and cdn for that.

@uasan
Copy link
Author

uasan commented Oct 6, 2017

HTTP-caching for application responses in Unit, is planned implement?

@VBart
Copy link
Contributor

VBart commented Oct 6, 2017

Not for the nearest future, but I would say that in general we are working on reducing complexity of web technology stack and therefore reducing number of different components that you have to deal with.

@OO00O0O
Copy link

OO00O0O commented Oct 12, 2017

@VBart so idea is to do http server with compilers as plugins? Like: apt install unit unit-php7, and i'm good to go, or I have install php separately? Can config be in project? It would be nice to have ability to register config: nginx-unit register /some/path/to/project/unit.json ProjectName && nginx-unit start ProjectName

@VBart
Copy link
Contributor

VBart commented Oct 16, 2017

apt install unit unit-php7, and i'm good to go, or I have install php separately?

apt handles all the dependencies automatically, so you don't have to install php separately.

Can config be in project?

Yes. It's up to you where and how to keep your configuration.

It would be nice to have ability to register config: nginx-unit register /some/path/to/project/unit.json ProjectName && nginx-unit start ProjectName

Since Unit has a simple RESTful JSON API, then it can be just a curl command, or a php script in your application that you run to register itself.

@mdesantis
Copy link

Hello! I'm interested about Websockets support

@VBart
Copy link
Contributor

VBart commented Apr 24, 2018

@stevenh
Copy link

stevenh commented Apr 24, 2018

Improvement from uwsgi but whats the benefit for go, you still need nginx in front of that so seems like just another layer to get in the way?

@nshadrin
Copy link
Contributor

The reasons to use Unit for a Go app would be related to consistency and ease of management across your various app implementations.

  • You will use the same provisioning and deployment for your Python app as you will do for a Go app or any other app.
  • Your HTTP operation will be the same for all your apps, and it simplifies troubleshooting.
  • You can decouple the network connectivity and configuration from the app code in the same way for all apps.
  • You can manage your Go app dynamically, remotely, with zero downtime. Upgrade the versions of the app, try its operation with various traffic patterns, stop, start, use several versions at the same time for canary or blue/green deployments, etc.

@mdesantis
Copy link

mdesantis commented Apr 24, 2018 via email

@VBart
Copy link
Contributor

VBart commented Apr 24, 2018

@mdesantis Support for WebSockets is planned. It's a complex feature, so it's hard to specify exact ETA until R&D will be done. You can help to shape it out by proposing a good interface for working with WebSockets in Unit.

andrey-zelenkov added a commit that referenced this issue Feb 21, 2024
These tests cause router crash when run with AddressSanitizer:

=================================================================
==77196==ERROR: AddressSanitizer: heap-use-after-free on address 0x60c000079340 at pc 0x55d56b132d4b bp 0x7f8cc7f346b0 sp 0x7f8cc7f346a0
READ of size 1 at 0x60c000079340 thread T1
    #0 0x55d56b132d4a in nxt_openssl_conn_io_shutdown src/nxt_openssl.c:1466
    #1 0x55d56b0f6a25 in nxt_h1p_closing src/nxt_h1proto.c:2069
    #2 0x55d56b1009a6 in nxt_h1p_shutdown src/nxt_h1proto.c:2038
    #3 0x55d56b1014c3 in nxt_h1p_request_close src/nxt_h1proto.c:1718
    #4 0x55d56b1045c0 in nxt_http_request_close_handler src/nxt_http_request.c:864
    #5 0x55d56b104988 in nxt_http_request_done src/nxt_http_request.c:795
    #6 0x55d56b0ba0c3 in nxt_event_engine_start src/nxt_event_engine.c:542
    #7 0x55d56b0dcac2 in nxt_router_thread_start src/nxt_router.c:3645
    #8 0x55d56b0b421b in nxt_thread_trampoline src/nxt_thread.c:126
    #9 0x7f8ccab95ac2  (/lib/x86_64-linux-gnu/libc.so.6+0x94ac2)
    #10 0x7f8ccac2784f  (/lib/x86_64-linux-gnu/libc.so.6+0x12684f)
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Feb 22, 2024
Otherwise, undefined behaviour will be triggered.

Can be reproduced by test/test_routing.py::test_routes_match_host_empty
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_route.c:2141:17: runtime error: applying zero offset to null pointer
    #0 0x100562588 in nxt_http_route_test_rule nxt_http_route.c:2091
    nginx#1 0x100564ed8 in nxt_http_route_handler nxt_http_route.c:1574
    nginx#2 0x10055188c in nxt_http_request_action nxt_http_request.c:570
    nginx#3 0x10052b1a0 in nxt_h1p_request_body_read nxt_h1proto.c:998
    nginx#4 0x100449c38 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#5 0x100436828 in nxt_thread_trampoline nxt_thread.c:126
    nginx#6 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    nginx#7 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_route.c:2141:17
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Feb 22, 2024
Can be reproduced by test/test_rewrite.py::test_rewrite_njs
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_js.c:169:52: runtime error: applying zero offset to null pointer
    #0 0x10255b044 in nxt_http_js_ext_get_args nxt_http_js.c:169
    nginx#1 0x102598ad0 in njs_value_property njs_value.c:1175
    nginx#2 0x10259c2c8 in njs_vm_object_prop njs_vm.c:1398
    nginx#3 0x102559d74 in nxt_js_call nxt_js.c:445
    nginx#4 0x1023c0da0 in nxt_tstr_query nxt_tstr.c:276
    nginx#5 0x102516ec4 in nxt_http_rewrite nxt_http_rewrite.c:56
    nginx#6 0x1024fd86c in nxt_http_request_action nxt_http_request.c:565
    nginx#7 0x1024d71b0 in nxt_h1p_request_body_read nxt_h1proto.c:998
    nginx#8 0x1023f5c48 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#9 0x1023e2838 in nxt_thread_trampoline nxt_thread.c:126
    nginx#10 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    nginx#11 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_js.c:169:52

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

Reviewed-by: Andrew Clayton <[email protected]>
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Feb 22, 2024
Found by UndefinedBehaviorSanitizer:

src/nxt_random.c:151:31: runtime error: left shift of 140 by 24 places cannot be represented in type 'int'
    #0 0x104f78968 in nxt_random nxt_random.c:151
    nginx#1 0x104f58a98 in nxt_shm_open nxt_port_memory.c:377
    nginx#2 0x10503e24c in nxt_controller_conf_send nxt_controller.c:617
    nginx#3 0x105041154 in nxt_controller_process_request nxt_controller.c:1109
    nginx#4 0x104fcdc48 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#5 0x104f27254 in main nxt_main.c:35
    nginx#6 0x180fbd0dc  (<unknown module>)

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

Reviewed-by: Andrew Clayton <[email protected]>
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Feb 22, 2024
Can be reproduced by test/test_variables.py::test_variables_dynamic_arguments
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_request.c:961:17: runtime error: applying zero offset to null pointer
    #0 0x1050d95a4 in nxt_http_arguments_parse nxt_http_request.c:961
    nginx#1 0x105102bf8 in nxt_http_var_arg nxt_http_variables.c:621
    nginx#2 0x104f95d74 in nxt_var_interpreter nxt_var.c:507
    nginx#3 0x104f98c98 in nxt_tstr_query nxt_tstr.c:265
    nginx#4 0x1050abfd8 in nxt_router_access_log_writer nxt_router_access_log.c:194
    nginx#5 0x1050d81f4 in nxt_http_request_close_handler nxt_http_request.c:838
    nginx#6 0x104fcdc48 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#7 0x104fba838 in nxt_thread_trampoline nxt_thread.c:126
    nginx#8 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    nginx#9 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_request.c:961:17
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Feb 22, 2024
Otherwise, undefined behaviour will be triggered.

Can be reproduced by test/test_routing.py::test_routes_match_host_empty
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_route.c:2141:17: runtime error: applying zero offset to null pointer
    #0 0x100562588 in nxt_http_route_test_rule nxt_http_route.c:2091
    nginx#1 0x100564ed8 in nxt_http_route_handler nxt_http_route.c:1574
    nginx#2 0x10055188c in nxt_http_request_action nxt_http_request.c:570
    nginx#3 0x10052b1a0 in nxt_h1p_request_body_read nxt_h1proto.c:998
    nginx#4 0x100449c38 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#5 0x100436828 in nxt_thread_trampoline nxt_thread.c:126
    nginx#6 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    nginx#7 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

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

Reviewed-by: Andrew Clayton <[email protected]>
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Feb 22, 2024
Can be reproduced by test/test_rewrite.py::test_rewrite_njs
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_js.c:169:52: runtime error: applying zero offset to null pointer
    #0 0x10255b044 in nxt_http_js_ext_get_args nxt_http_js.c:169
    nginx#1 0x102598ad0 in njs_value_property njs_value.c:1175
    nginx#2 0x10259c2c8 in njs_vm_object_prop njs_vm.c:1398
    nginx#3 0x102559d74 in nxt_js_call nxt_js.c:445
    nginx#4 0x1023c0da0 in nxt_tstr_query nxt_tstr.c:276
    nginx#5 0x102516ec4 in nxt_http_rewrite nxt_http_rewrite.c:56
    nginx#6 0x1024fd86c in nxt_http_request_action nxt_http_request.c:565
    nginx#7 0x1024d71b0 in nxt_h1p_request_body_read nxt_h1proto.c:998
    nginx#8 0x1023f5c48 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#9 0x1023e2838 in nxt_thread_trampoline nxt_thread.c:126
    nginx#10 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    nginx#11 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_js.c:169:52

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

Reviewed-by: Andrew Clayton <[email protected]>
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Feb 22, 2024
Found by UndefinedBehaviorSanitizer:

src/nxt_random.c:151:31: runtime error: left shift of 140 by 24 places cannot be represented in type 'int'
    #0 0x104f78968 in nxt_random nxt_random.c:151
    nginx#1 0x104f58a98 in nxt_shm_open nxt_port_memory.c:377
    nginx#2 0x10503e24c in nxt_controller_conf_send nxt_controller.c:617
    nginx#3 0x105041154 in nxt_controller_process_request nxt_controller.c:1109
    nginx#4 0x104fcdc48 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#5 0x104f27254 in main nxt_main.c:35
    nginx#6 0x180fbd0dc  (<unknown module>)

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

Reviewed-by: Andrew Clayton <[email protected]>
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Feb 22, 2024
Can be reproduced by test/test_variables.py::test_variables_dynamic_arguments
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_request.c:961:17: runtime error: applying zero offset to null pointer
    #0 0x1050d95a4 in nxt_http_arguments_parse nxt_http_request.c:961
    nginx#1 0x105102bf8 in nxt_http_var_arg nxt_http_variables.c:621
    nginx#2 0x104f95d74 in nxt_var_interpreter nxt_var.c:507
    nginx#3 0x104f98c98 in nxt_tstr_query nxt_tstr.c:265
    nginx#4 0x1050abfd8 in nxt_router_access_log_writer nxt_router_access_log.c:194
    nginx#5 0x1050d81f4 in nxt_http_request_close_handler nxt_http_request.c:838
    nginx#6 0x104fcdc48 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#7 0x104fba838 in nxt_thread_trampoline nxt_thread.c:126
    nginx#8 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    nginx#9 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_request.c:961:17
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Feb 23, 2024
Otherwise, undefined behaviour will be triggered.

Can be reproduced by test/test_routing.py::test_routes_match_host_empty
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_route.c:2141:17: runtime error: applying zero offset to null pointer
    #0 0x100562588 in nxt_http_route_test_rule nxt_http_route.c:2091
    nginx#1 0x100564ed8 in nxt_http_route_handler nxt_http_route.c:1574
    nginx#2 0x10055188c in nxt_http_request_action nxt_http_request.c:570
    nginx#3 0x10052b1a0 in nxt_h1p_request_body_read nxt_h1proto.c:998
    nginx#4 0x100449c38 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#5 0x100436828 in nxt_thread_trampoline nxt_thread.c:126
    nginx#6 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    nginx#7 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

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

Reviewed-by: Andrew Clayton <[email protected]>
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Feb 23, 2024
Can be reproduced by test/test_rewrite.py::test_rewrite_njs
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_js.c:169:52: runtime error: applying zero offset to null pointer
    #0 0x10255b044 in nxt_http_js_ext_get_args nxt_http_js.c:169
    nginx#1 0x102598ad0 in njs_value_property njs_value.c:1175
    nginx#2 0x10259c2c8 in njs_vm_object_prop njs_vm.c:1398
    nginx#3 0x102559d74 in nxt_js_call nxt_js.c:445
    nginx#4 0x1023c0da0 in nxt_tstr_query nxt_tstr.c:276
    nginx#5 0x102516ec4 in nxt_http_rewrite nxt_http_rewrite.c:56
    nginx#6 0x1024fd86c in nxt_http_request_action nxt_http_request.c:565
    nginx#7 0x1024d71b0 in nxt_h1p_request_body_read nxt_h1proto.c:998
    nginx#8 0x1023f5c48 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#9 0x1023e2838 in nxt_thread_trampoline nxt_thread.c:126
    nginx#10 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    nginx#11 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_js.c:169:52

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

Reviewed-by: Andrew Clayton <[email protected]>
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Feb 23, 2024
Found by UndefinedBehaviorSanitizer:

src/nxt_random.c:151:31: runtime error: left shift of 140 by 24 places cannot be represented in type 'int'
    #0 0x104f78968 in nxt_random nxt_random.c:151
    nginx#1 0x104f58a98 in nxt_shm_open nxt_port_memory.c:377
    nginx#2 0x10503e24c in nxt_controller_conf_send nxt_controller.c:617
    nginx#3 0x105041154 in nxt_controller_process_request nxt_controller.c:1109
    nginx#4 0x104fcdc48 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#5 0x104f27254 in main nxt_main.c:35
    nginx#6 0x180fbd0dc  (<unknown module>)

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

Reviewed-by: Andrew Clayton <[email protected]>
andrey-zelenkov added a commit to andrey-zelenkov/unit that referenced this issue Feb 23, 2024
Can be reproduced by test/test_variables.py::test_variables_dynamic_arguments
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_request.c:961:17: runtime error: applying zero offset to null pointer
    #0 0x1050d95a4 in nxt_http_arguments_parse nxt_http_request.c:961
    nginx#1 0x105102bf8 in nxt_http_var_arg nxt_http_variables.c:621
    nginx#2 0x104f95d74 in nxt_var_interpreter nxt_var.c:507
    nginx#3 0x104f98c98 in nxt_tstr_query nxt_tstr.c:265
    nginx#4 0x1050abfd8 in nxt_router_access_log_writer nxt_router_access_log.c:194
    nginx#5 0x1050d81f4 in nxt_http_request_close_handler nxt_http_request.c:838
    nginx#6 0x104fcdc48 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#7 0x104fba838 in nxt_thread_trampoline nxt_thread.c:126
    nginx#8 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    nginx#9 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

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

Reviewed-by: Andrew Clayton <[email protected]>
andrey-zelenkov added a commit that referenced this issue Feb 27, 2024
These tests cause router crash when run with AddressSanitizer:

=================================================================
==77196==ERROR: AddressSanitizer: heap-use-after-free on address 0x60c000079340 at pc 0x55d56b132d4b bp 0x7f8cc7f346b0 sp 0x7f8cc7f346a0
READ of size 1 at 0x60c000079340 thread T1
    #0 0x55d56b132d4a in nxt_openssl_conn_io_shutdown src/nxt_openssl.c:1466
    #1 0x55d56b0f6a25 in nxt_h1p_closing src/nxt_h1proto.c:2069
    #2 0x55d56b1009a6 in nxt_h1p_shutdown src/nxt_h1proto.c:2038
    #3 0x55d56b1014c3 in nxt_h1p_request_close src/nxt_h1proto.c:1718
    #4 0x55d56b1045c0 in nxt_http_request_close_handler src/nxt_http_request.c:864
    #5 0x55d56b104988 in nxt_http_request_done src/nxt_http_request.c:795
    #6 0x55d56b0ba0c3 in nxt_event_engine_start src/nxt_event_engine.c:542
    #7 0x55d56b0dcac2 in nxt_router_thread_start src/nxt_router.c:3645
    #8 0x55d56b0b421b in nxt_thread_trampoline src/nxt_thread.c:126
    #9 0x7f8ccab95ac2  (/lib/x86_64-linux-gnu/libc.so.6+0x94ac2)
    #10 0x7f8ccac2784f  (/lib/x86_64-linux-gnu/libc.so.6+0x12684f)
andrey-zelenkov added a commit that referenced this issue Mar 11, 2024
Otherwise, undefined behaviour will be triggered.

Can be reproduced by test/test_routing.py::test_routes_match_host_empty
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_route.c:2141:17: runtime error: applying zero offset to null pointer
    #0 0x100562588 in nxt_http_route_test_rule nxt_http_route.c:2091
    #1 0x100564ed8 in nxt_http_route_handler nxt_http_route.c:1574
    #2 0x10055188c in nxt_http_request_action nxt_http_request.c:570
    #3 0x10052b1a0 in nxt_h1p_request_body_read nxt_h1proto.c:998
    #4 0x100449c38 in nxt_event_engine_start nxt_event_engine.c:542
    #5 0x100436828 in nxt_thread_trampoline nxt_thread.c:126
    #6 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    #7 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

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

Reviewed-by: Andrew Clayton <[email protected]>
andrey-zelenkov added a commit that referenced this issue Mar 11, 2024
Can be reproduced by test/test_rewrite.py::test_rewrite_njs
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_js.c:169:52: runtime error: applying zero offset to null pointer
    #0 0x10255b044 in nxt_http_js_ext_get_args nxt_http_js.c:169
    #1 0x102598ad0 in njs_value_property njs_value.c:1175
    #2 0x10259c2c8 in njs_vm_object_prop njs_vm.c:1398
    #3 0x102559d74 in nxt_js_call nxt_js.c:445
    #4 0x1023c0da0 in nxt_tstr_query nxt_tstr.c:276
    #5 0x102516ec4 in nxt_http_rewrite nxt_http_rewrite.c:56
    #6 0x1024fd86c in nxt_http_request_action nxt_http_request.c:565
    #7 0x1024d71b0 in nxt_h1p_request_body_read nxt_h1proto.c:998
    #8 0x1023f5c48 in nxt_event_engine_start nxt_event_engine.c:542
    #9 0x1023e2838 in nxt_thread_trampoline nxt_thread.c:126
    #10 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    #11 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_js.c:169:52

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

Reviewed-by: Andrew Clayton <[email protected]>
andrey-zelenkov added a commit that referenced this issue Mar 11, 2024
Found by UndefinedBehaviorSanitizer:

src/nxt_random.c:151:31: runtime error: left shift of 140 by 24 places cannot be represented in type 'int'
    #0 0x104f78968 in nxt_random nxt_random.c:151
    #1 0x104f58a98 in nxt_shm_open nxt_port_memory.c:377
    #2 0x10503e24c in nxt_controller_conf_send nxt_controller.c:617
    #3 0x105041154 in nxt_controller_process_request nxt_controller.c:1109
    #4 0x104fcdc48 in nxt_event_engine_start nxt_event_engine.c:542
    #5 0x104f27254 in main nxt_main.c:35
    #6 0x180fbd0dc  (<unknown module>)

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

Reviewed-by: Andrew Clayton <[email protected]>
andrey-zelenkov added a commit that referenced this issue Mar 11, 2024
Can be reproduced by test/test_variables.py::test_variables_dynamic_arguments
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_request.c:961:17: runtime error: applying zero offset to null pointer
    #0 0x1050d95a4 in nxt_http_arguments_parse nxt_http_request.c:961
    #1 0x105102bf8 in nxt_http_var_arg nxt_http_variables.c:621
    #2 0x104f95d74 in nxt_var_interpreter nxt_var.c:507
    #3 0x104f98c98 in nxt_tstr_query nxt_tstr.c:265
    #4 0x1050abfd8 in nxt_router_access_log_writer nxt_router_access_log.c:194
    #5 0x1050d81f4 in nxt_http_request_close_handler nxt_http_request.c:838
    #6 0x104fcdc48 in nxt_event_engine_start nxt_event_engine.c:542
    #7 0x104fba838 in nxt_thread_trampoline nxt_thread.c:126
    #8 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    #9 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

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

Reviewed-by: Andrew Clayton <[email protected]>
pkillarjun pushed a commit to pkillarjun/unit that referenced this issue May 29, 2024
These tests cause router crash when run with AddressSanitizer:

=================================================================
==77196==ERROR: AddressSanitizer: heap-use-after-free on address 0x60c000079340 at pc 0x55d56b132d4b bp 0x7f8cc7f346b0 sp 0x7f8cc7f346a0
READ of size 1 at 0x60c000079340 thread T1
    #0 0x55d56b132d4a in nxt_openssl_conn_io_shutdown src/nxt_openssl.c:1466
    nginx#1 0x55d56b0f6a25 in nxt_h1p_closing src/nxt_h1proto.c:2069
    nginx#2 0x55d56b1009a6 in nxt_h1p_shutdown src/nxt_h1proto.c:2038
    nginx#3 0x55d56b1014c3 in nxt_h1p_request_close src/nxt_h1proto.c:1718
    nginx#4 0x55d56b1045c0 in nxt_http_request_close_handler src/nxt_http_request.c:864
    nginx#5 0x55d56b104988 in nxt_http_request_done src/nxt_http_request.c:795
    nginx#6 0x55d56b0ba0c3 in nxt_event_engine_start src/nxt_event_engine.c:542
    nginx#7 0x55d56b0dcac2 in nxt_router_thread_start src/nxt_router.c:3645
    nginx#8 0x55d56b0b421b in nxt_thread_trampoline src/nxt_thread.c:126
    nginx#9 0x7f8ccab95ac2  (/lib/x86_64-linux-gnu/libc.so.6+0x94ac2)
    nginx#10 0x7f8ccac2784f  (/lib/x86_64-linux-gnu/libc.so.6+0x12684f)
pkillarjun pushed a commit to pkillarjun/unit that referenced this issue May 29, 2024
Otherwise, undefined behaviour will be triggered.

Can be reproduced by test/test_routing.py::test_routes_match_host_empty
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_route.c:2141:17: runtime error: applying zero offset to null pointer
    #0 0x100562588 in nxt_http_route_test_rule nxt_http_route.c:2091
    nginx#1 0x100564ed8 in nxt_http_route_handler nxt_http_route.c:1574
    nginx#2 0x10055188c in nxt_http_request_action nxt_http_request.c:570
    nginx#3 0x10052b1a0 in nxt_h1p_request_body_read nxt_h1proto.c:998
    nginx#4 0x100449c38 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#5 0x100436828 in nxt_thread_trampoline nxt_thread.c:126
    nginx#6 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    nginx#7 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

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

Reviewed-by: Andrew Clayton <[email protected]>
pkillarjun pushed a commit to pkillarjun/unit that referenced this issue May 29, 2024
Can be reproduced by test/test_rewrite.py::test_rewrite_njs
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_js.c:169:52: runtime error: applying zero offset to null pointer
    #0 0x10255b044 in nxt_http_js_ext_get_args nxt_http_js.c:169
    nginx#1 0x102598ad0 in njs_value_property njs_value.c:1175
    nginx#2 0x10259c2c8 in njs_vm_object_prop njs_vm.c:1398
    nginx#3 0x102559d74 in nxt_js_call nxt_js.c:445
    nginx#4 0x1023c0da0 in nxt_tstr_query nxt_tstr.c:276
    nginx#5 0x102516ec4 in nxt_http_rewrite nxt_http_rewrite.c:56
    nginx#6 0x1024fd86c in nxt_http_request_action nxt_http_request.c:565
    nginx#7 0x1024d71b0 in nxt_h1p_request_body_read nxt_h1proto.c:998
    nginx#8 0x1023f5c48 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#9 0x1023e2838 in nxt_thread_trampoline nxt_thread.c:126
    nginx#10 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    nginx#11 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior src/nxt_http_js.c:169:52

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

Reviewed-by: Andrew Clayton <[email protected]>
pkillarjun pushed a commit to pkillarjun/unit that referenced this issue May 29, 2024
Found by UndefinedBehaviorSanitizer:

src/nxt_random.c:151:31: runtime error: left shift of 140 by 24 places cannot be represented in type 'int'
    #0 0x104f78968 in nxt_random nxt_random.c:151
    nginx#1 0x104f58a98 in nxt_shm_open nxt_port_memory.c:377
    nginx#2 0x10503e24c in nxt_controller_conf_send nxt_controller.c:617
    nginx#3 0x105041154 in nxt_controller_process_request nxt_controller.c:1109
    nginx#4 0x104fcdc48 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#5 0x104f27254 in main nxt_main.c:35
    nginx#6 0x180fbd0dc  (<unknown module>)

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

Reviewed-by: Andrew Clayton <[email protected]>
pkillarjun pushed a commit to pkillarjun/unit that referenced this issue May 29, 2024
Can be reproduced by test/test_variables.py::test_variables_dynamic_arguments
with enabled UndefinedBehaviorSanitizer:

src/nxt_http_request.c:961:17: runtime error: applying zero offset to null pointer
    #0 0x1050d95a4 in nxt_http_arguments_parse nxt_http_request.c:961
    nginx#1 0x105102bf8 in nxt_http_var_arg nxt_http_variables.c:621
    nginx#2 0x104f95d74 in nxt_var_interpreter nxt_var.c:507
    nginx#3 0x104f98c98 in nxt_tstr_query nxt_tstr.c:265
    nginx#4 0x1050abfd8 in nxt_router_access_log_writer nxt_router_access_log.c:194
    nginx#5 0x1050d81f4 in nxt_http_request_close_handler nxt_http_request.c:838
    nginx#6 0x104fcdc48 in nxt_event_engine_start nxt_event_engine.c:542
    nginx#7 0x104fba838 in nxt_thread_trampoline nxt_thread.c:126
    nginx#8 0x18133e030 in _pthread_start+0x84 (libsystem_pthread.dylib:arm64e+0x7030)
    nginx#9 0x181338e38 in thread_start+0x4 (libsystem_pthread.dylib:arm64e+0x1e38)

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

Reviewed-by: Andrew Clayton <[email protected]>
ac000 pushed a commit to ac000/unit that referenced this issue Jun 25, 2024
This issue was found with oss-fuzz.

  ==18420==WARNING: MemorySanitizer: use-of-uninitialized-value
	      #0 0x55dd798a5797 in nxt_vsprintf unit/src/nxt_sprintf.c:163:31
	      #1 0x55dd798d5bdb in nxt_conf_vldt_error unit/src/nxt_conf_validation.c:1525:11
	      #2 0x55dd798dd4cd in nxt_conf_vldt_var unit/src/nxt_conf_validation.c:1560:16
	      #3 0x55dd798dd4cd in nxt_conf_vldt_if unit/src/nxt_conf_validation.c:1592:16
	      nginx#4 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
	      nginx#5 0x55dd798d6f84 in nxt_conf_vldt_access_log unit/src/nxt_conf_validation.c:3426:11
	      nginx#6 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
	      nginx#7 0x55dd798d47bd in nxt_conf_validate unit/src/nxt_conf_validation.c:1421:11
	      nginx#8 0x55dd79871c82 in LLVMFuzzerTestOneInput unit/fuzzing/nxt_json_fuzz.c:67:5
	      nginx#9 0x55dd79770620 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
	      nginx#10 0x55dd7975adb4 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327:6
	      nginx#11 0x55dd7976084a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862:9
	      nginx#12 0x55dd7978cc42 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
	      nginx#13 0x7e8192213082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
	      nginx#14 0x55dd7975188d in _start

	    Uninitialized value was created by an allocation of 'error.i' in the stack frame
	      #0 0x55dd798dd42b in nxt_conf_vldt_var unit/src/nxt_conf_validation.c:1557:5
	      #1 0x55dd798dd42b in nxt_conf_vldt_if unit/src/nxt_conf_validation.c:1592:16

The issue was in nxt_tstr_test() where we create an error message with
nxt_sprintf(), where this error message is then later used with the
'%s' format specifier which expects a nul-terminated string, but by
default nxt_sprintf() doesn't nul-terminate, you must use the '%Z'
specifier to signify a '\0' at the end of the string.

Link: <https://github.com/google/oss-fuzz>
Link: <https://oss-fuzz.com/testcase-detail/5545917827055616>
Co-developed-by: Zhidao HONG <[email protected]>
Signed-off-by: Zhidao HONG <[email protected]>
Reviewed-by: Andrew Clayton <[email protected]>
Signed-off-by: Arjun <[email protected]>
[ Commit message/subject - Andrew ]
Signed-off-by: Andrew Clayton <[email protected]>
ac000 pushed a commit to ac000/unit that referenced this issue Jun 25, 2024
This issue was found with oss-fuzz.

  ==18420==WARNING: MemorySanitizer: use-of-uninitialized-value
	      #0 0x55dd798a5797 in nxt_vsprintf unit/src/nxt_sprintf.c:163:31
	      #1 0x55dd798d5bdb in nxt_conf_vldt_error unit/src/nxt_conf_validation.c:1525:11
	      #2 0x55dd798dd4cd in nxt_conf_vldt_var unit/src/nxt_conf_validation.c:1560:16
	      #3 0x55dd798dd4cd in nxt_conf_vldt_if unit/src/nxt_conf_validation.c:1592:16
	      nginx#4 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
	      nginx#5 0x55dd798d6f84 in nxt_conf_vldt_access_log unit/src/nxt_conf_validation.c:3426:11
	      nginx#6 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
	      nginx#7 0x55dd798d47bd in nxt_conf_validate unit/src/nxt_conf_validation.c:1421:11
	      nginx#8 0x55dd79871c82 in LLVMFuzzerTestOneInput unit/fuzzing/nxt_json_fuzz.c:67:5
	      nginx#9 0x55dd79770620 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
	      nginx#10 0x55dd7975adb4 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327:6
	      nginx#11 0x55dd7976084a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862:9
	      nginx#12 0x55dd7978cc42 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
	      nginx#13 0x7e8192213082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
	      nginx#14 0x55dd7975188d in _start

	    Uninitialized value was created by an allocation of 'error.i' in the stack frame
	      #0 0x55dd798dd42b in nxt_conf_vldt_var unit/src/nxt_conf_validation.c:1557:5
	      #1 0x55dd798dd42b in nxt_conf_vldt_if unit/src/nxt_conf_validation.c:1592:16

The issue was in nxt_tstr_test() where we create an error message with
nxt_sprintf(), where this error message is then later used with the
'%s' format specifier which expects a nul-terminated string, but by
default nxt_sprintf() doesn't nul-terminate, you must use the '%Z'
specifier to signify a '\0' at the end of the string.

Link: <https://github.com/google/oss-fuzz>
Link: <https://oss-fuzz.com/testcase-detail/5545917827055616>
Co-developed-by: Zhidao HONG <[email protected]>
Signed-off-by: Zhidao HONG <[email protected]>
Reviewed-by: Andrew Clayton <[email protected]>
Signed-off-by: Arjun <[email protected]>
[ Commit message/subject - Andrew ]
Signed-off-by: Andrew Clayton <[email protected]>
ac000 pushed a commit to ac000/unit that referenced this issue Jun 25, 2024
This issue was found with oss-fuzz.

  ==18420==WARNING: MemorySanitizer: use-of-uninitialized-value
	      #0 0x55dd798a5797 in nxt_vsprintf unit/src/nxt_sprintf.c:163:31
	      #1 0x55dd798d5bdb in nxt_conf_vldt_error unit/src/nxt_conf_validation.c:1525:11
	      #2 0x55dd798dd4cd in nxt_conf_vldt_var unit/src/nxt_conf_validation.c:1560:16
	      #3 0x55dd798dd4cd in nxt_conf_vldt_if unit/src/nxt_conf_validation.c:1592:16
	      nginx#4 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
	      nginx#5 0x55dd798d6f84 in nxt_conf_vldt_access_log unit/src/nxt_conf_validation.c:3426:11
	      nginx#6 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
	      nginx#7 0x55dd798d47bd in nxt_conf_validate unit/src/nxt_conf_validation.c:1421:11
	      nginx#8 0x55dd79871c82 in LLVMFuzzerTestOneInput unit/fuzzing/nxt_json_fuzz.c:67:5
	      nginx#9 0x55dd79770620 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
	      nginx#10 0x55dd7975adb4 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327:6
	      nginx#11 0x55dd7976084a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862:9
	      nginx#12 0x55dd7978cc42 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
	      nginx#13 0x7e8192213082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
	      nginx#14 0x55dd7975188d in _start

	    Uninitialized value was created by an allocation of 'error.i' in the stack frame
	      #0 0x55dd798dd42b in nxt_conf_vldt_var unit/src/nxt_conf_validation.c:1557:5
	      #1 0x55dd798dd42b in nxt_conf_vldt_if unit/src/nxt_conf_validation.c:1592:16

The issue was in nxt_tstr_test() where we create an error message with
nxt_sprintf(), where this error message is then later used with the
'%s' format specifier which expects a nul-terminated string, but by
default nxt_sprintf() doesn't nul-terminate, you must use the '%Z'
specifier to signify a '\0' at the end of the string.

Link: <https://github.com/google/oss-fuzz>
Link: <https://oss-fuzz.com/testcase-detail/5545917827055616>
Co-developed-by: Zhidao HONG <[email protected]>
Signed-off-by: Zhidao HONG <[email protected]>
Reviewed-by: Andrew Clayton <[email protected]>
Signed-off-by: Arjun <[email protected]>
[ Commit message/subject - Andrew ]
Signed-off-by: Andrew Clayton <[email protected]>
ac000 pushed a commit to ac000/unit that referenced this issue Jun 25, 2024
This issue was found with oss-fuzz.

  ==18420==WARNING: MemorySanitizer: use-of-uninitialized-value
	      #0 0x55dd798a5797 in nxt_vsprintf unit/src/nxt_sprintf.c:163:31
	      #1 0x55dd798d5bdb in nxt_conf_vldt_error unit/src/nxt_conf_validation.c:1525:11
	      #2 0x55dd798dd4cd in nxt_conf_vldt_var unit/src/nxt_conf_validation.c:1560:16
	      #3 0x55dd798dd4cd in nxt_conf_vldt_if unit/src/nxt_conf_validation.c:1592:16
	      nginx#4 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
	      nginx#5 0x55dd798d6f84 in nxt_conf_vldt_access_log unit/src/nxt_conf_validation.c:3426:11
	      nginx#6 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
	      nginx#7 0x55dd798d47bd in nxt_conf_validate unit/src/nxt_conf_validation.c:1421:11
	      nginx#8 0x55dd79871c82 in LLVMFuzzerTestOneInput unit/fuzzing/nxt_json_fuzz.c:67:5
	      nginx#9 0x55dd79770620 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
	      nginx#10 0x55dd7975adb4 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327:6
	      nginx#11 0x55dd7976084a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862:9
	      nginx#12 0x55dd7978cc42 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
	      nginx#13 0x7e8192213082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
	      nginx#14 0x55dd7975188d in _start

	    Uninitialized value was created by an allocation of 'error.i' in the stack frame
	      #0 0x55dd798dd42b in nxt_conf_vldt_var unit/src/nxt_conf_validation.c:1557:5
	      #1 0x55dd798dd42b in nxt_conf_vldt_if unit/src/nxt_conf_validation.c:1592:16

The issue was in nxt_tstr_test() where we create an error message with
nxt_sprintf(), where this error message is then later used with the
'%s' format specifier which expects a nul-terminated string, but by
default nxt_sprintf() doesn't nul-terminate, you must use the '%Z'
specifier to signify a '\0' at the end of the string.

Signed-off-by: Arjun <[email protected]>
Co-developed-by: Zhidao HONG <[email protected]>
Signed-off-by: Zhidao HONG <[email protected]>
Link: <https://github.com/google/oss-fuzz>
Link: <https://oss-fuzz.com/testcase-detail/5545917827055616>
Reviewed-by: Andrew Clayton <[email protected]>
[ Commit message/subject - Andrew ]
Signed-off-by: Andrew Clayton <[email protected]>
ac000 pushed a commit to ac000/unit that referenced this issue Jun 25, 2024
This issue was found with oss-fuzz.

  ==18420==WARNING: MemorySanitizer: use-of-uninitialized-value
	      #0 0x55dd798a5797 in nxt_vsprintf unit/src/nxt_sprintf.c:163:31
	      #1 0x55dd798d5bdb in nxt_conf_vldt_error unit/src/nxt_conf_validation.c:1525:11
	      #2 0x55dd798dd4cd in nxt_conf_vldt_var unit/src/nxt_conf_validation.c:1560:16
	      #3 0x55dd798dd4cd in nxt_conf_vldt_if unit/src/nxt_conf_validation.c:1592:16
	      nginx#4 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
	      nginx#5 0x55dd798d6f84 in nxt_conf_vldt_access_log unit/src/nxt_conf_validation.c:3426:11
	      nginx#6 0x55dd798d55f4 in nxt_conf_vldt_object unit/src/nxt_conf_validation.c:2815:23
	      nginx#7 0x55dd798d47bd in nxt_conf_validate unit/src/nxt_conf_validation.c:1421:11
	      nginx#8 0x55dd79871c82 in LLVMFuzzerTestOneInput unit/fuzzing/nxt_json_fuzz.c:67:5
	      nginx#9 0x55dd79770620 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerLoop.cpp:614:13
	      nginx#10 0x55dd7975adb4 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:327:6
	      nginx#11 0x55dd7976084a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:862:9
	      nginx#12 0x55dd7978cc42 in main /src/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
	      nginx#13 0x7e8192213082 in __libc_start_main /build/glibc-SzIz7B/glibc-2.31/csu/libc-start.c:308:16
	      nginx#14 0x55dd7975188d in _start

	    Uninitialized value was created by an allocation of 'error.i' in the stack frame
	      #0 0x55dd798dd42b in nxt_conf_vldt_var unit/src/nxt_conf_validation.c:1557:5
	      #1 0x55dd798dd42b in nxt_conf_vldt_if unit/src/nxt_conf_validation.c:1592:16

The issue was in nxt_tstr_test() where we create an error message with
nxt_sprintf(), where this error message is then later used with the
'%s' format specifier which expects a nul-terminated string, but by
default nxt_sprintf() doesn't nul-terminate, you must use the '%Z'
specifier to signify a '\0' at the end of the string.

Signed-off-by: Arjun <[email protected]>
Co-developed-by: Zhidao HONG <[email protected]>
Signed-off-by: Zhidao HONG <[email protected]>
Link: <https://github.com/google/oss-fuzz>
Reviewed-by: Andrew Clayton <[email protected]>
[ Commit message/subject - Andrew ]
Signed-off-by: Andrew Clayton <[email protected]>
ac000 added a commit to ac000/unit that referenced this issue Oct 16, 2024
Under Ubuntu 24.04 the pytest for
test/test_php_isolation.py::test_php_isolation_rootfs fails due to Unit
aborting (SIGABRT) in the PHP language module due to FORIFY_SOURCE
hardening detecting a buffer overflow

  2024/10/16 16:46:54 [info] 11661#11661 "phpinfo" application started
  *** buffer overflow detected ***: terminated
  2024/10/16 16:46:54 [alert] 11660#11660 app process 11661 exited on signal 6

After spending an extraordinary amount of time faffing around with
Ubuntu and pytests (they don't make for a pleasant combination) I was
able to reproduce it.

The crash was occurring here

  nginx#4  0x00007ebe818288ff in __GI_abort () at ./stdlib/abort.c:79
  nginx#5  0x00007ebe818297b6 in __libc_message_impl (
      fmt=fmt@entry=0x7ebe819ce765 "*** %s ***: terminated\n")
      at ../sysdeps/posix/libc_fatal.c:132
  nginx#6  0x00007ebe81936c19 in __GI___fortify_fail (
      msg=msg@entry=0x7ebe819ce74c "buffer overflow detected")
      at ./debug/fortify_fail.c:24
  nginx#7  0x00007ebe819365d4 in __GI___chk_fail () at ./debug/chk_fail.c:28
  nginx#8  0x00007ebe8134a055 in mempcpy (__len=10, __src=0x7ebe8160ade8,
      __dest=0x571ba9bd0930)
      at /usr/include/x86_64-linux-gnu/bits/string_fortified.h:45
  nginx#9  fake_data_segment (info=0x0, sysdb=0x571ba9bcf080)
      at /usr/src/php8.1-8.1.30-1+ubuntu24.04.1+deb.sury.org+1/ext/date/lib/parse_tz.c:921
  nginx#10 timelib_builtin_db ()
      at /usr/src/php8.1-8.1.30-1+ubuntu24.04.1+deb.sury.org+1/ext/date/lib/parse_tz.c:1084
  nginx#11 0x00007ebe812e0885 in zm_info_date (zend_module=0x571ba9a14420)

[Well as best as I can tell, as this is from the php 8.1 packages from
<https://github.com/oerdnj/deb.sury.org>, I don't know where the
packages (I'm assuming it's packages) shivammathur/setup-php@v2
installs come from.]

So we get killed in fake_data_segment(), the thing is, that function (as
well as timelib_builtin_db()) doesn't exist in upstream PHP.

It turns out these come from a patch that is applied by distributions to
make PHP use the system installed timezone database rather than the one
built into PHP.

I was unable to reproduce this with vanilla PHP 8.1.

It can be triggered on affected builds with the following config

  {
      "listeners": {
          "[::1]:8080": {
              "pass": "applications/php"
          }
      },

      "applications": {
          "php": {
              "type": "php",
              "root": "/app/php",

      	      "isolation": {
  	          "rootfs": "/tmp/unit-root",

                  "namespaces": {
  	              "mount": true,
  		      "credential": true,
  		      "pid": true
                  }
  	      }
          }
      }
  }

The crux of the issue seems to come down to in this case PHP can't open
the tz database as it's not contained in the new mount namespace.

  190437 openat(AT_FDCWD, "/usr/share/zoneinfo/", O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_DIRECTORY) = -1 ENOENT (No such file or directory)
  190437 openat(AT_FDCWD, "/usr/share/zoneinfo/zone.tab", O_RDONLY) = -1 ENOENT (No such file or directory)
  190437 writev(2, [{iov_base="*** ", iov_len=4}, {iov_base="buffer overflow detected", iov_len=24}, {iov_base=" ***: terminated\n", iov_len=17}], 3) = 45
  ...
  190437 --- SIGABRT {si_signo=SIGABRT, si_code=SI_TKILL, si_pid=2, si_uid=65534} ---
  190437 +++ killed by SIGABRT +++

Specifically the issue is with the following code in the patch
(certainly an earlier version of the patch, this is from a Debian patch
<https://sources.debian.org/src/php8.2/8.2.20-1~deb12u1/debian/patches/0007-Add-support-for-use-of-the-system-timezone-database.patch/>)

  +        data = malloc(3 * sysdb->index_size + 7);
  +
  +        p = mempcpy(data, FAKE_HEADER, sizeof(FAKE_HEADER) - 1);

If the zone file hasn't been found then sysdb->index_size is 0. So we
malloc(3) a total of 7 bytes.

However, sizeof(FAKE_HEADER) - 1 is 10. (Hence the __len=10 in the
mempcpy(3) in the above backtrace).

Of course 10 doesn't fit into 7 and the FORTIFY_SOURCE hardening kicks
in and SIGABRTs the process.

Now, it's worth noting that this issue doesn't occur with PHP 8.2 and
8.3.

As can been seen from the Fedora patch for this
<https://src.fedoraproject.org/rpms/php/blob/rawhide/f/php-8.4.0-systzdata-v24.patch>

They actually have a fix incorporated

  r23: fix possible buffer overflow

So the above patch now does

  +        data = malloc(3 * sysdb->index_size + sizeof(FAKE_HEADER) - 1);
  +
  +        p = mempcpy(data, FAKE_HEADER, sizeof(FAKE_HEADER) - 1);

So you will always get at least the required 10 bytes allocated.

I assume the PHP 8.2 & 8.3 packages either no longer use this patch or
have the fixed version. I don't know... I haven't found the sources...

Anyway the above was more about satisfying myself that the problem
wasn't with Unit.

PHP 8.1 is now in security maintenance mode and people are actively
encouraged to upgrade to 8.2/8.3

So lets just remove 8.1 from our testing...

[It's also worth noting that after all this, the ubuntu-latest runners
seemed to have switched back from 24.04 to 22.04. However lets stick
with this and the other ci fixes as who knows when it'll go back to
24.04 (or some other version) again...]

Link: <https://www.php.net/supported-versions.php>
Signed-off-by: Andrew Clayton <[email protected]>
avahahn pushed a commit to javorszky/unit that referenced this issue Oct 22, 2024
Under Ubuntu 24.04 the pytest for
test/test_php_isolation.py::test_php_isolation_rootfs fails due to Unit
aborting (SIGABRT) in the PHP language module due to FORIFY_SOURCE
hardening detecting a buffer overflow

  2024/10/16 16:46:54 [info] 11661#11661 "phpinfo" application started
  *** buffer overflow detected ***: terminated
  2024/10/16 16:46:54 [alert] 11660#11660 app process 11661 exited on signal 6

After spending an extraordinary amount of time faffing around with
Ubuntu and pytests (they don't make for a pleasant combination) I was
able to reproduce it.

The crash was occurring here

  nginx#4  0x00007ebe818288ff in __GI_abort () at ./stdlib/abort.c:79
  nginx#5  0x00007ebe818297b6 in __libc_message_impl (
      fmt=fmt@entry=0x7ebe819ce765 "*** %s ***: terminated\n")
      at ../sysdeps/posix/libc_fatal.c:132
  nginx#6  0x00007ebe81936c19 in __GI___fortify_fail (
      msg=msg@entry=0x7ebe819ce74c "buffer overflow detected")
      at ./debug/fortify_fail.c:24
  nginx#7  0x00007ebe819365d4 in __GI___chk_fail () at ./debug/chk_fail.c:28
  nginx#8  0x00007ebe8134a055 in mempcpy (__len=10, __src=0x7ebe8160ade8,
      __dest=0x571ba9bd0930)
      at /usr/include/x86_64-linux-gnu/bits/string_fortified.h:45
  nginx#9  fake_data_segment (info=0x0, sysdb=0x571ba9bcf080)
      at /usr/src/php8.1-8.1.30-1+ubuntu24.04.1+deb.sury.org+1/ext/date/lib/parse_tz.c:921
  nginx#10 timelib_builtin_db ()
      at /usr/src/php8.1-8.1.30-1+ubuntu24.04.1+deb.sury.org+1/ext/date/lib/parse_tz.c:1084
  nginx#11 0x00007ebe812e0885 in zm_info_date (zend_module=0x571ba9a14420)

[Well as best as I can tell, as this is from the php 8.1 packages from
<https://github.com/oerdnj/deb.sury.org>, I don't know where the
packages (I'm assuming it's packages) shivammathur/setup-php@v2
installs come from.]

So we get killed in fake_data_segment(), the thing is, that function (as
well as timelib_builtin_db()) doesn't exist in upstream PHP.

It turns out these come from a patch that is applied by distributions to
make PHP use the system installed timezone database rather than the one
built into PHP.

I was unable to reproduce this with vanilla PHP 8.1.

It can be triggered on affected builds with the following config

  {
      "listeners": {
          "[::1]:8080": {
              "pass": "applications/php"
          }
      },

      "applications": {
          "php": {
              "type": "php",
              "root": "/app/php",

      	      "isolation": {
  	          "rootfs": "/tmp/unit-root",

                  "namespaces": {
  	              "mount": true,
  		      "credential": true,
  		      "pid": true
                  }
  	      }
          }
      }
  }

The crux of the issue seems to come down to in this case PHP can't open
the tz database as it's not contained in the new mount namespace.

  190437 openat(AT_FDCWD, "/usr/share/zoneinfo/", O_RDONLY|O_NONBLOCK|O_CLOEXEC|O_DIRECTORY) = -1 ENOENT (No such file or directory)
  190437 openat(AT_FDCWD, "/usr/share/zoneinfo/zone.tab", O_RDONLY) = -1 ENOENT (No such file or directory)
  190437 writev(2, [{iov_base="*** ", iov_len=4}, {iov_base="buffer overflow detected", iov_len=24}, {iov_base=" ***: terminated\n", iov_len=17}], 3) = 45
  ...
  190437 --- SIGABRT {si_signo=SIGABRT, si_code=SI_TKILL, si_pid=2, si_uid=65534} ---
  190437 +++ killed by SIGABRT +++

Specifically the issue is with the following code in the patch
(certainly an earlier version of the patch, this is from a Debian patch
<https://sources.debian.org/src/php8.2/8.2.20-1~deb12u1/debian/patches/0007-Add-support-for-use-of-the-system-timezone-database.patch/>)

  +        data = malloc(3 * sysdb->index_size + 7);
  +
  +        p = mempcpy(data, FAKE_HEADER, sizeof(FAKE_HEADER) - 1);

If the zone file hasn't been found then sysdb->index_size is 0. So we
malloc(3) a total of 7 bytes.

However, sizeof(FAKE_HEADER) - 1 is 10. (Hence the __len=10 in the
mempcpy(3) in the above backtrace).

Of course 10 doesn't fit into 7 and the FORTIFY_SOURCE hardening kicks
in and SIGABRTs the process.

Now, it's worth noting that this issue doesn't occur with PHP 8.2 and
8.3.

As can been seen from the Fedora patch for this
<https://src.fedoraproject.org/rpms/php/blob/rawhide/f/php-8.4.0-systzdata-v24.patch>

They actually have a fix incorporated

  r23: fix possible buffer overflow

So the above patch now does

  +        data = malloc(3 * sysdb->index_size + sizeof(FAKE_HEADER) - 1);
  +
  +        p = mempcpy(data, FAKE_HEADER, sizeof(FAKE_HEADER) - 1);

So you will always get at least the required 10 bytes allocated.

I assume the PHP 8.2 & 8.3 packages either no longer use this patch or
have the fixed version. I don't know... I haven't found the sources...

Anyway the above was more about satisfying myself that the problem
wasn't with Unit.

PHP 8.1 is now in security maintenance mode and people are actively
encouraged to upgrade to 8.2/8.3

So lets just remove 8.1 from our testing...

[It's also worth noting that after all this, the ubuntu-latest runners
seemed to have switched back from 24.04 to 22.04. However lets stick
with this and the other ci fixes as who knows when it'll go back to
24.04 (or some other version) again...]

Link: <https://www.php.net/supported-versions.php>
Signed-off-by: Andrew Clayton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests