ua_buffer_reader should be released in deallocate_redirect_postdata

[scw00]

ua_buffer_reader should be released in deallocate_redirect_postdata_buffers

Otherwise, we will never append block in mbuf. The max_read_avail (in read_from net) will check every readers in this mbuf and find out this reader. But we've not consumed data in this reader.

So if the buffer is full, the request will be blocked until timeout.

[mingzym]

[approve ci]

[zwoop]

[approve ci]

[zwoop]

Interesting. This seems pretty serious, is this a 7.1.0 candidate? @oknet ?

[scw00]

Considering this problem
1 The client send several post bytes to ats. We alloc the post buf and clone reader from tunnel's read_buffer, then tunnel and post buf consume data and return to eventsytem.

2 The client send left data and fill buffer, but tunnel dealloc post buf without consuming if the length of data is larger than HttpConfig::m_master.post_copy_size, then setup next read.

3 The read check the data block was full and still have data which not conuming(see write_avail in read_from_net), it will disable read and return directly. Then the vc will be lost until timeout.

[zwoop]

@oknet What's the decision / thought on 7.1.0 or not? We need it ?

[zwoop]

I've put this on the docs.trafficserver / ci.trafficserver machines, for testing.

[oknet]

You should check all the calls to deallocate_redirect_postdata_buffers(); and always call the deallocate_buffers(); after it.

HttpSM::tunnel_handler_cache_fill, HttpSM::tunnel_handler_100_continue, HttpSM::tunnel_handler_push, etc ...

[bryancall]

@oknet If we should always call deallocate_redirect_postdata_buffers() before deallocate_buffers() then should the HttpTunnel class handle this itself?

[oknet]

Yes, @scw00 has done it in the last commit. The comment is only for the first commit.

[scw00]

Hmm, I think we should assign ua_reader_buffer to nullptr when we free the ua_reader_buffer's mbuf.

[oknet]

[approve ci]

[zwoop]

@oknet @scw00 what's the word on this PR? 7.1.0 ?

[scw00]

@zwoop This is a 7.1.0 candidate.

[zwoop]

Ready to land this ?

[bryancall]

This seems like a very error prone interface to clean up the IOBufferReader

[scw00]

Hmm, The reader will be cleaned if mbuf freed in deallocate_buffer

[scw00]

So, we don't need to consider the order of deallocate_buffer.

[scw00]

Attach a golang test script

package main

import(
  "net"
  "log"
  "time"
  "fmt"
)

func main() {
  conn, err := net.Dial("tcp", "127.0.0.1:8080")
  if err != nil {
    log.Fatal(err)
  }

  data := "POST HTTP://www.scw.com/ HTTP/1.1\r\n" +
    "Content-Length: 400000\r\n\r\n"
  conn.Write([]byte(data))
  data1 := make([]byte, 153)
  time.Sleep(1 * time.Second)
  conn.Write(data1)
  data2 := make([]byte, 400000 - 153)
  conn.Write(data2)

  data3 := make([]byte, 40000)
  conn.Read(data3)
  fmt.Println(string(data3))
  time.Sleep(3 * time.Second)
}

[oknet]

Set the postbuf->ua_buffer_reader to nullptr If the ATS free the producer.read_buffer.
To block any access to the postbuf->ua_buffer_reader in deallocate_redirect_postdata_buffers .

[scw00]

Config

CONFIG proxy.config.http.number_of_redirections INT 3

[scw00]

Could I merge this pr ?

[zwoop]

@bryancall Ping? Should we merge this, and cherry-pick to 7.1.0? you had some concerns.

[scw00]

Hi @bryancall , any idea for this PR ? We can use the golang script for a test. ATS will hang up the vc when enable post redirection.

[zwoop]

Ping @bryancall @scw00 and @oknet on this. What's the word? This might be too late now for 7.0.0, but do we want it for 7.1.1 ?

[zwoop]

Cherry-picked to 7.1.x

[ranxuxin001]

I've read lots of pr about post data redirection request. Can I think that GET request redirection is not affected, please?

[oknet]

@ranxuxin yes, the buffer is used to replay the post payload while the request is redirected to the new server.