[http server]Receiving http pipeline while asynchronously writing resp, there is a potential crash risk

[cxyxiaoli]

Hello, libhv is a very good open source library, recently I am learning the source code of libhv. While reading the http server code, I found a thread safety issue that may cause coredump, so I want to confirm it with you. The problem is described as follows:
(1) Pre-scenario: libhv's http server supports asynchronous response, that is, its own thread in the upper layer to use HttpResponseWriterPtr to write data to HttpResponsePtr.
At the same time, libhv support keep-alive, namely in the connection I received a new request, will call an HttpHandler: : FeedRecvData function, then the request to reset the old data.
(2) Reproduction method: When the upper-layer asynchronously calls HttpResponseWriterPtr to write the response data, if the customer sends the data again (can be http-pipeline or attack), the state of HttpHandler is not equal to WANT_RECV, Then call HttpHandler::Reset, and then call HttpMessage::Reset(), which will empty some object data.These data is not done thread-safe, so in the upper layer with HttpResponseWriterPtr modification, may cause segment errors.
Hope to get your reply:
(1) Whether the scene of this problem exists.
(2) If so, can it be optimized?

很抱歉我的英文表达能力不是很好，以下是issue的中文描述：
您好，libhv是一个非常优秀的开源库，最近我在学习libhv的源码。在阅读http服务器代码的时候，发现了一个可能会引发coredump的线程安全问题，因此想跟您确认一下。问题描述如下：
（1）前置场景：libhv的http server是支持异步响应的，也就是在上层自己的线程去使用HttpResponseWriterPtr写入数据到HttpResponsePtr。
同时，libhv也支持keep-alive，也就是在连接收到新的请求时，会进入HttpHandler::FeedRecvData函数，然后去reset旧的请求的数据。
（2）复现方式：在上层异步调用HttpResponseWriterPtr写入响应数据时，如果这时候客户再次发送数据过来（可以是http-pipeline，也可以是攻击），这时候会HttpHandler的state不等于WANT_RECV，然后会调用HttpHandler::Reset，进而调用HttpMessage::Reset()，这里会清空一些对象的数据，因为这些数据是没有做线程安全的，因此在上层用HttpResponseWriterPtr修改的时候，可能会导致段错误。

希望能得到您的答复：
（1）这个问题的场景是否存在。
（2）如果存在，能否优化呢？

[ithewei]

我也直接使用中文回复了，首先表扬你看源码看的很仔细，思考的很深入。
（1）这里确实没有考虑http pipeline场景或者恶意攻击的情况，假设的是常规的一应一答的请求-响应场景，在你描述的场景里使用http异步回调确实有线程安全问题，可能导致coredump。
（2）上面问题主要是因为一个HTTP连接里只使用了一个HttpRequest、HttpResponse请求上下文导致的，如果要优化的话，就得每接收一个新的请求就是New一个HttpRequest、HttpResponse请求上下文，不要去复用之前的来避免竞争，缺点就是多个请求上下文的生命周期管理有点麻烦，连接断开后，才能释放掉这条连接上所有的请求上下文。

如果不考虑支持http pipeline场景，作为服务端确实也应该考虑这种情况可能导致的coredump，后续我想下怎么规避下，加锁可能会影响效率。

[cxyxiaoli]

非常感谢作者的回复。
我这边有两个方案，不知道合不合理：
方案一：如果是临时规避的话，可以先在HttpHandler::FeedRecvData加个判断：如果state == HANDLE_CONTINUE && writer->end != HttpResponseWriter::SEND_END，则直接return 0，那么httpServer.cpp:on_recv会视为异常，然后关闭连接。这样可以先规避被攻击的场景。
方案二：后续如果要支持http-pipeline，那么也是在HttpHandler::FeedRecvData加个判断：如果state != WANT_RECV，那么先将data存入HttpHandler的一个缓冲buf变量，然后在send完上一次响应的时候，去检查这个缓冲buf是否有数据，有的话则调用HttpHandler::Reset，然后将buf送给HttpParser::FeedRecvData。
不过方案二也有点复杂，涉及到：
（1）buf空间的限制；
（2）上层writer结束后，如何读取buf。以及buf在写入时的线程互斥问题。