Error 503 when using URI rewrite with query parameters

[rlandgrebe]

Environment

• Unit 1.31.0 via Docker image unit:php8.2
• Host system: Apple MacBook Pro M1 Pro (ARM64 architecture)

Steps to reproduce

(Note: I think that Docker is not needed to reproduce this error)

1. Create files

Dockerfile:

FROM unit:1.31.0-php8.2
COPY ./unit.json /docker-entrypoint.d/
RUN mkdir -p /var/www/html
COPY ./index.php /var/www/html

index.php:

<?php phpinfo();

unit.json:

{
    "listeners": {
        "*:80": {
            "pass": "routes"
        }
    },
    "routes": [
        {
            "action": {
                "rewrite": "$uri",
                "pass": "applications/test/index"
            }
        }
    ],
    "applications": {
        "test": {
            "type": "php",
            "targets": {
                "index": {
                    "root": "/var/www/html/",
                    "script": "index.php"
                }
            }
        }
    }
}

2. Build the image

docker build -t nginx_unit_test:latest .

3. Run the image

docker run -p 80:80 --rm nginx_unit_test:latest

4. Check that everything works without query parameters

Use the browser and check that the following addresses work correctly and show the PHP info page:

http://localhost
http://localhost/test

5. Use query parameters to trigger error behaviour

http://localhost/test?p=1

Expected behaviour

The PHP info page should be displayed:

Actual behaviour

I get an Error 503.

Additional information

Debug log: debug.txt

[lcrilly]

Thanks for the detailed report.

Using the same host system I can reproduce as instructed. I cannot reproduce with unit-php installed from Homebrew. I cannot reproduce with unit:python Docker image (although I don't have an equivalent phpinfo() to test with).

Requires deeper investigation.

[ac000]

The rewrite line in the config is causing a segfault in libphp-8.2.so likely because the rewrite rule is causing Unit to send some bogus data to PHP.

What is that line actually expected to do?

[rlandgrebe]

Thank you everyone for answering!

The rewrite line in the config is causing a segfault in libphp-8.2.so likely because the rewrite rule is causing Unit to send some bogus data to PHP.

What is that line actually expected to do?

Do you mean what the rewrite line is supposed to do? I actually wanted to rewrite path segments and used njs scripting to do this (calling replace() on the uri variable). This resulted in a 503 error when using query parameters in the URL. So I've tried to narrow down the error by removing any scripting and just using $uri in the rewrite rule. Unfortunately this gave the same error.

[ac000]

With a simple request like

$ curl http://localhost:8080/test?q=a

Without the rewrite line, the offset of where the query_string is in memory (the memory that contains a structure + data of the request) is 98, seems reasonable.

With the rewrite rule its coming out as some varying large number like 4294936094, this then causes a segfault in php when it tries to get the query string and it's now looking at memory outside its address space.

#0  0x00007f2abd494577 in php_default_treat_data (arg=1, str=0x0, 
    destArray=<optimized out>)
    at /usr/src/debug/php-8.2.10-1.fc38.x86_64/main/php_variables.c:488
488                     if (c_var && *c_var) {                                  
(gdb) list
483                     return;
484             }
485
486             if (arg == PARSE_GET) {         /* GET data */
487                     c_var = SG(request_info).query_string;
488                     if (c_var && *c_var) {
489                             res = (char *) estrdup(c_var);
490                             free_buffer = 1;
491                     } else {
492                             free_buffer = 0;
(gdb) p c_var
$1 = 0x7f2bb8880676 <error: Cannot access memory at address 0x7f2bb8880676>

Why the rewrite rule is doing this remains to be determined...

[ac000]

Next piece of the puzzle...

5506            nxt_unit_sptr_set(&req->query, query_pos);
(gdb) 

Thread 2 "unitd" hit Hardware watchpoint 2: req->query

Old value = {
  base = "",
  offset = 0
}
New value = {
  base = "\236",
  offset = 4294961822
}
nxt_unit_sptr_set (sptr=0x7fe6e46f6058, ptr=0x7fe6e46f4af6)
    at src/nxt_unit_sptr.h:28
28      }
(gdb) list
23
24      static inline void
25      nxt_unit_sptr_set(nxt_unit_sptr_t *sptr, void *ptr)
26      {
27          sptr->offset = (uint8_t *) ptr - sptr->base;
28      }

5506            nxt_unit_sptr_set(&req->query, query_pos);

is called from nxt_router_prepare_msg()

It ends up doing

0x7fe6e46f4af6 - 0x7fe6e46f6058 = -1562

which in decimal is -5474

Checking the type of ->offset

(gdb) ptype nxt_unit_sptr_t
type = union nxt_unit_sptr_u {
    uint8_t base[1];
    uint32_t offset;
}

so that means that value will count 5474 bytes back from UINT_MAX which gives us our 4294961822

Now to figure out why ptr is < sptr->base

[ac000]

Some more tracing in nxt_router_prepare_msg()

No rewrite

0x7fedb81b1090: "GET"
0x7fedb81b1094: "HTTP/1.1"
0x7fedb81b109d: "::1"
0x7fedb81b10a1: "::1"
0x7fedb81b10a5: "8080"
0x7fedb81b10aa: "localhost"
0x7fedb81b10b4: "/test?q=a"

(gdb) p target_pos
$4 = (void *) 0x7fedb81b10b4

(gdb) p query_pos
$7 = (void *) 0x7fedb81b10ba

(gdb) p r->args->start
$5 = (u_char *) 0x7fedb4002b02 "q=a HTTP/1.1\r\nHost: localhost:8080\r\nUser-Agent: curl/8.0.1\r\nAccept: */*\r\n\r\n"
(gdb) p r->target.start
$6 = (u_char *) 0x7fedb4002afc "/test?q=a HTTP/1.1\r\nHost: localhost:8080\r\nUser-Agent: curl/8.0.1\r\nAccept: */*\r\n\r\n"

With rewrite

0x7f7eaaaa8090: "GET"
0x7f7eaaaa8094: "HTTP/1.1"
0x7f7eaaaa809d: "::1"
0x7f7eaaaa80a1: "::1"
0x7f7eaaaa80a5: "8080"
0x7f7eaaaa80aa: "localhost"
0x7f7eaaaa80b4: "/test?q=a"
0x7f7eaaaa80be: "/test"

(gdb) p target_pos
$4 = (void *) 0x7f7eaaaa80b4

(gdb) p query_pos
$6 = (void *) 0x7f7eaaaa6af6

(gdb) p r->args->start
$8 = (u_char *) 0x7f7ea4002b02 "q=a HTTP/1.1\r\nHost: localhost:8080\r\nUser-Agent: curl/8.0.1\r\nAccept: */*\r\n\r\n"
(gdb) p r->target.start
$9 = (u_char *) 0x7f7ea40040c0 "/test?q=a"

Hmm, that last address 0x7f7ea40040c0 looks out of wack, it should be smaller than r->args->start.

It's coming from

 95         r->target = target;                                                 

in nxt_http_rewrite() where target is a newly allocated area of memory. This holds the new target (path) + query string.

We need to also update r->args->start so that it points to the start of the query string in this new memory.

[ac000]

@rlandgrebe

Fancy giving the following patch a spin?

diff --git a/src/nxt_http_rewrite.c b/src/nxt_http_rewrite.c
index ae5c865a..fb216eeb 100644
--- a/src/nxt_http_rewrite.c
+++ b/src/nxt_http_rewrite.c
@@ -93,6 +93,7 @@ nxt_http_rewrite(nxt_task_t *task, nxt_http_request_t *r)
         nxt_memcpy(p, r->args->start, r->args->length);

         r->target = target;
+        r->args->start = p;
     }

     r->path = nxt_mp_alloc(r->mem_pool, sizeof(nxt_str_t));

[rlandgrebe]

@rlandgrebe

Fancy giving the following patch a spin?

@ac000 I've just tested your patch on Ubuntu 23.04 (I've built Unit from source) and so far it looks good to me. Thank you very much!

Before patch: ❌ Error 503 After patch: ✅ Everything works fine

I also tested the patch with njs scripting to strip path prefixes from the uri string. This now also works as expected: I have used the following rule to strip /test1/ and /test2/from the beginning of the path:

"rewrite": "`${uri.replace(/\\/(test1|test2)\\//, '/')}`"

URI tested: http://192.168.240.130/test1/sub?q=a

Result:

• ✅ REQUEST_URI looks good
• ✅ QUERY_STRING looks good

[ac000]

@rlandgrebe

Great, thanks for testing! Hopefully we can get this fix into an upcoming release...

[lcrilly]

FWIW, I built a unit:php Docker image with the patch applied to this diff of the official Dockerfile:

diff -r 3a9046dca2a6 pkg/docker/Dockerfile.php8.2
--- a/pkg/docker/Dockerfile.php8.2      Wed Aug 23 11:29:07 2023 +0000
+++ b/pkg/docker/Dockerfile.php8.2      Wed Oct 04 20:44:52 2023 +0100
@@ -8,6 +8,8 @@
 LABEL org.opencontainers.image.vendor="NGINX Docker Maintainers <docker-maint@nginx.com>"
 LABEL org.opencontainers.image.version="1.31.0"

+COPY *.patch .
+
 RUN set -ex \
     && savedAptMark="$(apt-mark showmanual)" \
     && apt-get update \
@@ -17,6 +19,7 @@
     && cd /usr/src/unit \
     && hg clone -u 1.31.0-1 https://hg.nginx.org/unit \
     && cd unit \
+    && cat ../*.patch | patch -p1 \
     && NCPU="$(getconf _NPROCESSORS_ONLN)" \
     && DEB_HOST_MULTIARCH="$(dpkg-architecture -q DEB_HOST_MULTIARCH)" \
     && CC_OPT="$(DEB_BUILD_MAINT_OPTIONS="hardening=+all,-pie" DEB_CFLAGS_MAINT_APPEND="-Wp,-D_FORTIFY_SOURCE=2 -fPIC" dpkg-buildflags --get CFLAGS)" \

Cannot reproduce. So also looks good here.

[hongzhidao]

The patch looks good. args should be part of target in memory. The association between target and args changes in the same way in nxt_http_parse_complex_target().

And we need to add a test. @andrey-zelenkov, take a look at this plz.

{
    "listeners": {
        "*:8080": {
            "pass": "routes"
        }
    },
    "routes": [
        {
            "action": {
                "rewrite": "$uri",  /* 1. add rewrite option */
                "pass": "applications/app"
            }
        }
    ],
    "applications": {
        "app": {
            "type": "php",
            "root": "/tmp",
            "index": "index.php"
        }
    }
}
}

/* 2. add any query */
> curl http://127.1:8080/?p=1

[ac000]

Thanks @hongzhidao I'll take that as a review...

[ac000]

Fix merged as https://github.com/nginx/unit/commit/30142d2a3c862eef0a32805ee3907535ada32c71