Ruby: Handle response field arrays #998

ac000 · 2023-11-08T22:20:18Z

@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 · 2023-11-10T00:12:24Z

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 · 2023-11-13T22:49:49Z

Replying to the #974 (comment):

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 · 2023-11-13T23:22:35Z

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 · 2023-11-14T15:53:32Z

Drop the "Empty headers" patch
The main change in the remaining patch is we don't allow T_NIL in T_ARRAYs. The Rack spec has this to say

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 · 2023-11-14T17:58:46Z

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 · 2023-11-14T20:24:56Z

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 · 2023-12-05T22:06:13Z

Is anything still blocking this PR?

ac000 · 2023-12-06T00:17:00Z

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 · 2023-12-06T15:29:00Z

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 · 2023-12-07T11:22:04Z

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

@xeron

@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] nginx#8: Ruby: Wrong header entry 'value' from application 2023/10/22 20:57:28 [error] 56#56 [unit] nginx#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[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 [0]: <https://github.com/rack/rack/blob/main/UPGRADE-GUIDE.md?plain=1#L26> Reported-by: Ivan Larionov <xeron.oskom@gmail.com> Closes: <nginx#974> Link: <nginx#998> Tested-by: Timo Stark <t.stark@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

ac000 · 2023-12-08T13:51:15Z

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)

@xeron

@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] nginx#8: Ruby: Wrong header entry 'value' from application 2023/10/22 20:57:28 [error] 56#56 [unit] nginx#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[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 [0]: <https://github.com/rack/rack/blob/main/UPGRADE-GUIDE.md?plain=1#L26> Reported-by: Ivan Larionov <xeron.oskom@gmail.com> Closes: <nginx#974> Link: <nginx#998> Tested-by: Timo Stark <t.stark@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

hongzhidao · 2023-12-13T02:10:54Z

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 · 2023-12-13T03:31:29Z

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.

@xeron

@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[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 [0]: <https://github.com/rack/rack/blob/main/UPGRADE-GUIDE.md?plain=1#L26> Reported-by: Ivan Larionov <xeron.oskom@gmail.com> Closes: <#974> Link: <#998> Tested-by: Timo Stark <t.stark@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

@xeron

@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] nginx#8: Ruby: Wrong header entry 'value' from application 2023/10/22 20:57:28 [error] 56#56 [unit] nginx#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[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 [0]: <https://github.com/rack/rack/blob/main/UPGRADE-GUIDE.md?plain=1#L26> Reported-by: Ivan Larionov <xeron.oskom@gmail.com> Closes: <nginx#974> Link: <nginx#998> Tested-by: Timo Stark <t.stark@nginx.com> Signed-off-by: Andrew Clayton <a.clayton@nginx.com>

ac000 force-pushed the ruby branch 2 times, most recently from 22b204c to 5ff34ff Compare November 10, 2023 00:10

ac000 mentioned this pull request Nov 13, 2023

Doesn't work after rails 7.1 update #974

Closed

ac000 force-pushed the ruby branch from 5ff34ff to e1ae88a Compare November 14, 2023 15:41

ac000 changed the title ~~Ruby: Support empty response headers and arrays of header values~~ Ruby: Handle response field arrays Nov 14, 2023

ac000 added the z-ruby Ruby language module label Dec 1, 2023

ac000 force-pushed the ruby branch from e1ae88a to efe929d Compare December 6, 2023 15:26

ac000 force-pushed the ruby branch from efe929d to d9f5f1f Compare December 8, 2023 13:49

ac000 merged commit d9f5f1f into nginx:master Dec 8, 2023

ac000 deleted the ruby branch December 8, 2023 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ruby: Handle response field arrays #998

Ruby: Handle response field arrays #998

ac000 commented Nov 8, 2023 •

edited

ac000 commented Nov 10, 2023

andrey-zelenkov commented Nov 13, 2023

ac000 commented Nov 13, 2023

ac000 commented Nov 14, 2023

andrey-zelenkov commented Nov 14, 2023

ac000 commented Nov 14, 2023

xeron commented Dec 5, 2023

ac000 commented Dec 6, 2023

ac000 commented Dec 6, 2023

tippexs commented Dec 7, 2023

ac000 commented Dec 8, 2023

hongzhidao commented Dec 13, 2023

ac000 commented Dec 13, 2023

Ruby: Handle response field arrays #998

Ruby: Handle response field arrays #998

Conversation

ac000 commented Nov 8, 2023 • edited

ac000 commented Nov 10, 2023

andrey-zelenkov commented Nov 13, 2023

ac000 commented Nov 13, 2023

ac000 commented Nov 14, 2023

andrey-zelenkov commented Nov 14, 2023

ac000 commented Nov 14, 2023

xeron commented Dec 5, 2023

ac000 commented Dec 6, 2023

ac000 commented Dec 6, 2023

tippexs commented Dec 7, 2023

ac000 commented Dec 8, 2023

hongzhidao commented Dec 13, 2023

ac000 commented Dec 13, 2023

ac000 commented Nov 8, 2023 •

edited