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 324 forks source link

Ruby: Handle response field arrays #998

Closed ac000 closed 7 months ago

ac000 commented 8 months ago

@xeron on GitHub reported an issue whereby with a Rails 7.1 application they were getting the following error

2023/10/22 20:57:28 [error] 56#56 [unit] #8: Ruby: Wrong header entry 'value' from application
2023/10/22 20:57:28 [error] 56#56 [unit] #8: Ruby: Failed to run ruby script

After some back and forth debugging it turns out rack was trying to send back a header comprised of an array of values. E.g

app = Proc.new do |env|
    ["200", {
        "Content-Type" => "text/plain",
        "X-Array-Header" => ["Item-1", "Item-2"],
    }, ["Hello World\n"]]
end

run app

It seems this became a possibility in rack v3.0

So along with a header value type of T_STRING we need to also allow T_ARRAY.

If we get a T_ARRAY we need to build up the header field using the given values.

E.g

"X-Array-Header" => ["Item-1", "", "Item-3", "Item-4"],

becomes

X-Array-Header: Item-1; ; Item-3; Item-4
ac000 commented 8 months ago

Use the nxt_unit_malloc()/nxt_unit_free() wrappers.

Range diff

1:  22b204cc ! 1:  5ff34ff2 Ruby: Handle response field arrays.
    @@ src/ruby/nxt_ruby.c: nxt_ruby_hash_add(VALUE r_key, VALUE r_value, VALUE arg)
     +            len += 2;    /* +2 for '; ' */
     +        }
     +
    -+        field = nxt_malloc(len);
    ++        field = nxt_unit_malloc(NULL, len);
     +        if (field == NULL) {
     +            goto fail;
     +        }
    @@ src/ruby/nxt_ruby.c: nxt_ruby_hash_add(VALUE r_key, VALUE r_value, VALUE arg)
     +        *rc = nxt_unit_response_add_field(headers_info->req,
     +                                          RSTRING_PTR(r_key), key_len,
     +                                          field, len);
    -+        nxt_free(field);
    ++        nxt_unit_free(NULL, field);
     +
     +        if (nxt_slow_path(*rc != NXT_UNIT_OK)) {
     +            goto fail;
andrey-zelenkov commented 7 months ago

Replying to the https://github.com/nginx/unit/issues/974#issuecomment-1809116333:

Which 'spec' are you reading?

https://github.com/rack/rack/blob/main/SPEC.rdoc

Also in the past I have been required to send empty headers to the UK's Make Tax Digital service.

It is still possible to send empty headers by passing empty string as a value:

'x-empty' => '',

Seems overly restrictive...

Yeah, I don't insist on it. Just noticed that this part is not requered by spec and Rack's own tool report it as error. So I just suggested to simplify our code and avoid possible reworks here in the future.

ac000 commented 7 months ago

Hmm, confirming that

"X-Empty-Header" => nil

and

"X-Empty-Header" => ""

do in fact send the same thing...

recvfrom(5, "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nX-Empty-Header: \r\nServer: Unit/1.32.0\r\nDate: Mon, 13 Nov 2023 23:15:24 GMT\r\nTransfer-Encoding: chunked\r\n\r\nc\r\nHello World\n\r\n0\r\n\r\n", 102400, 0, NULL, NULL) = 171

vs

recvfrom(5, "HTTP/1.1 200 OK\r\nContent-Type: text/plain\r\nX-Empty-Header: \r\nServer: Unit/1.32.0\r\nDate: Mon, 13 Nov 2023 23:16:16 GMT\r\nTransfer-Encoding: chunked\r\n\r\nc\r\nHello World\n\r\n0\r\n\r\n", 102400, 0, NULL, NULL) = 171

I'd propose to drop the first patch...

ac000 commented 7 months ago

Header values must be either a String instance, or an Array of String instances, such that each String instance must not contain characters below 037.

Range diff

1:  5a82c9b1 < -:  -------- Ruby: Allow empty response headers.
2:  5ff34ff2 ! 1:  e1ae88ad Ruby: Handle response field arrays.
    @@ Commit message

         It seems this became a possibility in rack v3.0[0]

    -    So along with header value types of T_STRING & T_NIL we need to also
    -    allow T_ARRAY.
    +    So along with a header value type of T_STRING we need to also allow
    +    T_ARRAY.

         If we get a T_ARRAY we need to build up the header field using the given
         values.

         E.g

    -      "X-Array-Header" => ["Item-1", nil, "Item-3", "Item-4"],
    +      "X-Array-Header" => ["Item-1", "", "Item-3", "Item-4"],

         becomes

    @@ src/ruby/nxt_ruby.c: nxt_ruby_hash_info(VALUE r_key, VALUE r_value, VALUE arg)
              goto fail;
          }

    --    if (nxt_slow_path(TYPE(r_value) != T_STRING && TYPE(r_value) != T_NIL)) {
    -+    if (nxt_slow_path(TYPE(r_value) != T_STRING
    -+                      && TYPE(r_value) != T_ARRAY
    -+                      && TYPE(r_value) != T_NIL))
    -+    {
    +-    if (nxt_slow_path(TYPE(r_value) != T_STRING)) {
    ++    if (nxt_slow_path(TYPE(r_value) != T_STRING && TYPE(r_value) != T_ARRAY)) {
              nxt_unit_req_error(headers_info->req,
                                 "Ruby: Wrong header entry 'value' from application");

    @@ src/ruby/nxt_ruby.c: nxt_ruby_hash_info(VALUE r_key, VALUE r_value, VALUE arg)
     +
     +        for (i = 0; i < arr_len; i++) {
     +            item = rb_ary_entry(r_value, i);
    -+            if (TYPE(item) != T_STRING && TYPE(item) != T_NIL) {
    ++            if (TYPE(item) != T_STRING) {
     +                nxt_unit_req_error(headers_info->req,
     +                                   "Ruby: Wrong header entry in 'value' array "
     +                                   "from application");
     +                goto fail;
     +            }
     +
    -+            if (TYPE(item) == T_STRING) {
    -+                len += RSTRING_LEN(item);
    -+            }
    -+
    -+            len += 2;    /* +2 for '; ' */
    ++            len += RSTRING_LEN(item) + 2;   /* +2 for '; ' */
     +        }
     +
     +        headers_info->fields++;
    @@ src/ruby/nxt_ruby.c: nxt_ruby_hash_info(VALUE r_key, VALUE r_value, VALUE arg)
     +        return ST_CONTINUE;
     +    }
     +
    -     NXT_RUBY_SET_HDR_VALUE(r_value, value, value_end);
    -     pos = value;
    +     value = RSTRING_PTR(r_value);
    +     value_end = value + RSTRING_LEN(r_value);

     @@ src/ruby/nxt_ruby.c: nxt_ruby_hash_add(VALUE r_key, VALUE r_value, VALUE arg)
    +     headers_info = (void *) (uintptr_t) arg;
    +     rc = &headers_info->rc;

    -     key_len = RSTRING_LEN(r_key);
    - 
    ++    key_len = RSTRING_LEN(r_key);
    ++
     +    if (TYPE(r_value) == T_ARRAY) {
     +        int     i;
     +        int     arr_len = RARRAY_LEN(r_value);
@@ src/ruby/nxt_ruby.c: nxt_ruby_hash_add(VALUE r_key, VALUE r_value, VALUE arg)
     +        for (i = 0; i < arr_len; i++) {
     +            item = rb_ary_entry(r_value, i);
     +
    -+            if (TYPE(item) == T_STRING) {
    -+                len += RSTRING_LEN(item);
    -+            }
    -+
    -+            len += 2;    /* +2 for '; ' */
    ++            len += RSTRING_LEN(item) + 2;   /* +2 for '; ' */
     +        }
     +
     +        field = nxt_unit_malloc(NULL, len);
    @@ src/ruby/nxt_ruby.c: nxt_ruby_hash_add(VALUE r_key, VALUE r_value, VALUE arg)
     +
     +        for (i = 0; i < arr_len; i++) {
     +            item = rb_ary_entry(r_value, i);
    -+            if (TYPE(item) == T_STRING) {
    -+                p = nxt_cpymem(p, RSTRING_PTR(item), RSTRING_LEN(item));
    -+            }
     +
    ++            p = nxt_cpymem(p, RSTRING_PTR(item), RSTRING_LEN(item));
     +            p = nxt_cpymem(p, "; ", 2);
     +        }
     +
    @@ src/ruby/nxt_ruby.c: nxt_ruby_hash_add(VALUE r_key, VALUE r_value, VALUE arg)
     +        return ST_CONTINUE;
     +    }
     +
    -     NXT_RUBY_SET_HDR_VALUE(r_value, value, value_end);
    +     value = RSTRING_PTR(r_value);
    +     value_end = value + RSTRING_LEN(r_value);
    + 
    +-    key_len = RSTRING_LEN(r_key);
    +-
          pos = value;

    +     for ( ;; ) {
andrey-zelenkov commented 7 months ago

nil value in array of headers can be handled (and interpreted as empty string) by rackup for some reason. So, we can revert this change if you feel comfortable with this one.

I mean:

"X-Array-Header" => ["Item-1", nil, "Item-3", "Item-4"],
ac000 commented 7 months ago

Seeing as the rack linter doesn't like T_NIL by itself as a header value I'm inclined to interpret

... or an Array of String instances ...

as meaning an array of T_STRINGs.

xeron commented 7 months ago

Is anything still blocking this PR?

ac000 commented 7 months ago

Hi @xeron

We've been doing some repository reorganisations, now that's done, from my point of view I'm ready to merge this, so unless anyone speaks up otherwise, I will do so in the coming days.

ac000 commented 7 months ago

Remove extraneous '.' from summary line

$ git range-diff e1ae88ad...efe929de
1:  e1ae88ad ! 1:  efe929de Ruby: Handle response field arrays.
    @@ Metadata
     Author: Andrew Clayton <a.clayton@nginx.com>

      ## Commit message ##
    -    Ruby: Handle response field arrays.
    +    Ruby: Handle response field arrays

         @xeron on GitHub reported an issue whereby with a Rails 7.1 application
         they were getting the following error
tippexs commented 7 months ago

Hi @xeron

We've been doing some repository reorganisations, now that's done, from my point of view I'm ready to merge this, so unless anyone speaks up otherwise, I will do so in the coming days.

LGTM

ac000 commented 7 months ago

Rebased with master

 -:  -------- >  1:  dd0c53a7 Python: Do nxt_unit_sptr_get() earlier in nxt_python_field_value().
 -:  -------- >  2:  5cfad9cc Python: Fix header field values character encoding.
 -:  -------- >  3:  dfdf948f Define nxt_cpu_pause for ARM64.
 -:  -------- >  4:  27c787f4 Fix comments for src/nxt_unit.h.
 -:  -------- >  5:  919cae7f PHP: Fix a possible file-pointer leak.
 -:  -------- >  6:  1443d623 Node.js: ServerResponse.flushHeaders() implemented.
 -:  -------- >  7:  8fbe437c Tests: Ruby input.rewind is no longer required.
 -:  -------- >  8:  0fc52321 Tests: added more expected Ruby features.
 -:  -------- >  9:  6b6e3bd8 Fixed the MD5Encoder deprecation warning.
 -:  -------- > 10:  73d723e5 Red Hat should always be spelled as two words.
 -:  -------- > 11:  3fdf8c63 Fix port number in listener object for php hello world app.
 -:  -------- > 12:  a922f9a6 Update third-party components for the Java module.
 -:  -------- > 13:  9a36de84 Go: Use Homebrew include paths
 -:  -------- > 14:  846a7f48 .mailmap: Set correct address for Danielle
 1:  efe929de ! 15:  d9f5f1fb Ruby: Handle response field arrays
    @@ Commit message

         [0]: <https://github.com/rack/rack/blob/main/UPGRADE-GUIDE.md?plain=1#L26>

         Reported-by: Ivan Larionov <xeron.oskom@gmail.com>
         Closes: <https://github.com/nginx/unit/issues/974>
    +    Link: <https://github.com/nginx/unit/pull/998>
         Tested-by: Timo Stark <t.stark@nginx.com>
         Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

      ## src/ruby/nxt_ruby.c ##
     @@ src/ruby/nxt_ruby.c: nxt_ruby_hash_info(VALUE r_key, VALUE r_value, VALUE arg)
hongzhidao commented 7 months ago

Hi @ac000 @andrey-zelenkov

Take a look at the report.

New defect(s) Reported-by: Coverity Scan
Showing 1 of 1 defect(s)

** CID 411012:  Memory - corruptions  (OVERRUN)

________________________________________________________________________________________________________
*** CID 411012:  Memory - corruptions  (OVERRUN)
/src/ruby/nxt_ruby.c: 999 in nxt_ruby_hash_add()
993                 p = nxt_cpymem(p, RSTRING_PTR(item), RSTRING_LEN(item));
994                 p = nxt_cpymem(p, "; ", 2);
995             }
996     
997             len -= 2;
998     
>>>     CID 411012:  Memory - corruptions  (OVERRUN)
>>>     Calling "nxt_unit_response_add_field" with "field" and "len" is suspicious because of the very large index, 4294967294. The index may be due to a negative parameter being interpreted as unsigned.
999             *rc = nxt_unit_response_add_field(headers_info->req,
1000                                               RSTRING_PTR(r_key), key_len,
1001                                               field, len);
1002             nxt_unit_free(NULL, field);
1003     
1004             if (nxt_slow_path(*rc != NXT_UNIT_OK)) {

Is it reproduced with headers like this?

"X-Array-Header" => []

Btw, it looks like it's helpful to add tests.

ac000 commented 7 months ago

Is it reproduced with headers like this?

"X-Array-Header" => []

Right... Fix merged.

Btw, it looks like it's helpful to add tests.

@andrey-zelenkov

We should add a test for empty arrays.