api7 / ngx_multi_upstream_module

ngx_multi_upstream_module for nginx
BSD 2-Clause "Simplified" License
9 stars 5 forks source link

avoid add field not at the end of `ngx_connection_s` #10

Closed liudongmiao closed 2 years ago

liudongmiao commented 2 years ago

It would prevent any nginx dynamic module to use ngx_connection_s, aka r->connection.

spacewander commented 2 years ago

Could you give an example?

liudongmiao commented 2 years ago

modsecurity-nginx, a waf engine: https://github.com/SpiderLabs/ModSecurity-nginx/blob/master/src/ngx_http_modsecurity_rewrite.c#L55-L82

I use apisix with docker, use gdb to find the issue.

As you see, as there are a new void *multi_c, cannot get the right memory offset.

{data = 0x70e200, read = 0x0, write = 0x6cd1b0, fd = 0x6e51c0, recv = 0xc, send = 0x45e6c0 <ngx_unix_recv>, recv_chain = 0x45eb50 <ngx_unix_send>, send_chain = 0x45e850 <ngx_readv_chain>, 
  listening = 0x463bb0 <ngx_linux_sendfile_chain>, sent = 0x65d270, log = 0x0, pool = 0x835080, type = 8605728, sockaddr = 0x1, socklen = 8605808, addr_text = {len = 16, data = 0x9 <error: Cannot access memory at address 0x9>}, 
  proxy_protocol = 0x8350d0, ssl = 0x0, udp = 0x0, local_sockaddr = 0x0, local_socklen = 7098784, buffer = 0x10, queue = {prev = 0x835138, next = 0x65caf0}, number = 6671088, start_time = 1, requests = 9393270922, buffered = 1, 
  log_error = 0, timedout = 0, error = 0, destroyed = 0, idle = 0, reusable = 0, close = 0, shared = 0, sendfile = 0, sndlowat = 0, tcp_nodelay = 0, tcp_nopush = 0, need_last_buf = 0, busy_count = 0, sendfile_task = 0x840200}

I have two workaround:

    // apisix-base / ngx_multi_upstream_module add `void *multi_c` before `read`.
    if (connection->read == NULL || (connection->sent != 0 && connection->log == NULL)) {
        connection = (ngx_connection_t *) (((char *) (r)->connection) + sizeof(void *));
    }

or, don't access r->connection at all. However, I don't think modsecurity would merge the codes.

liudongmiao commented 2 years ago

I have checked the patch, only ngx_connection_s is added not at the end...

https://github.com/api7/ngx_multi_upstream_module/blob/master/nginx-1.21.4.patch

spacewander commented 2 years ago

If you can submit a PR, I am willing to accept it.