From 3f67c21ba8cf95b836f0f6dde9fd7ce635d5bf2b Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Tue, 19 Jan 2021 21:57:45 -0800 Subject: [PATCH] Fix the end of chunk parsing (#1403) While parsing a chunked response with the curl HTTP transport adapter, there was an issue for parsing the last chunk. As soon as the end of chunk data was found ("0") the adapter was returning and setting the session state as if the transfer was completed. However, the HTTP RFC for chunked data (https://tools.ietf.org/html/rfc7230#section-4.1) defines that there is a CRLF after the last chunk info. By not reading the last CRLF from the response, and if the connection was re-used right after reading the last chunk made the next request to get the `CRLF` as the first part for the response, making the parser crash. The fix in this PR makes sure that when the last chunk is found and parsed, the CRLF is also parsed from the response to make sure that the response data transfer has completed fixes: #1396 --- sdk/core/azure-core/CHANGELOG.md | 4 + sdk/core/azure-core/src/http/curl/curl.cpp | 50 ++++--- .../src/http/curl/curl_session_private.hpp | 13 ++ .../azure-core/test/ut/curl_session_test.cpp | 124 ++++++++++++++++++ 4 files changed, 175 insertions(+), 16 deletions(-) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index 074ee032e..281213249 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -6,6 +6,10 @@ - Make `ToLower` and `LocaleInvariantCaseInsensitiveEqual` internal by moving them from `Azure::Core::Strings` to `Azure::Core::Internal::Strings`. +### Bug Fixes + +- Fixed the parsing of the last chunk of a chunked response when using the curl transport adapter. + ## 1.0.0-beta.4 (2021-01-13) ### New Features diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index 89a188901..72057caf6 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -258,6 +258,7 @@ CURLcode CurlSession::Perform(Context const& context) if (this->m_lastStatusCode != HttpStatusCode::Continue) { LogThis("Server rejected the upload request"); + m_sessionState = SessionState::STREAMING; return result; // Won't upload. } @@ -267,6 +268,7 @@ CURLcode CurlSession::Perform(Context const& context) // If internal buffer has more data after the 100-continue means Server return an error. // We don't need to upload body, just parse the response from Server and return ReadStatusLineAndHeadersFromRawResponse(context, true); + m_sessionState = SessionState::STREAMING; return result; } @@ -274,6 +276,7 @@ CURLcode CurlSession::Perform(Context const& context) result = this->UploadBody(context); if (result != CURLE_OK) { + m_sessionState = SessionState::STREAMING; return result; // will throw transport exception before trying to read } @@ -467,6 +470,7 @@ void CurlSession::ParseChunkSize(Context const& context) if (this->m_chunkSize == 0) { // Response with no content. end of chunk keepPolling = false; + this->m_bodyStartInBuffer = index + 1; break; } @@ -603,6 +607,31 @@ void CurlSession::ReadStatusLineAndHeadersFromRawResponse( */ } +void CurlSession::ReadExpected(Context const& context, uint8_t expected) +{ + if (this->m_bodyStartInBuffer == -1 || this->m_bodyStartInBuffer == this->m_innerBufferSize) + { + // end of buffer, pull data from wire + this->m_innerBufferSize = m_connection->ReadFromSocket( + context, this->m_readBuffer, Details::c_DefaultLibcurlReaderSize); + this->m_bodyStartInBuffer = 0; + } + auto data = this->m_readBuffer[this->m_bodyStartInBuffer]; + if (data != expected) + { + throw TransportException( + "Unexpected format in HTTP response. Expecting: " + std::to_string(expected) + + ", but found: " + std::to_string(data) + "."); + } + this->m_bodyStartInBuffer += 1; +} + +void CurlSession::ReadCRLF(Context const& context) +{ + ReadExpected(context, '\r'); + ReadExpected(context, '\n'); +} + // Read from curl session int64_t CurlSession::OnRead(Context const& context, uint8_t* buffer, int64_t count) { @@ -614,27 +643,17 @@ int64_t CurlSession::OnRead(Context const& context, uint8_t* buffer, int64_t cou // check if all chunked is all read already if (this->m_isChunkedResponseType && this->m_chunkSize == this->m_sessionTotalRead) { - // Need to read CRLF after all chunk was read - for (int8_t i = 0; i < 2; i++) - { - if (this->m_bodyStartInBuffer > 0 && this->m_bodyStartInBuffer < this->m_innerBufferSize) - { - this->m_bodyStartInBuffer += 1; - } - else - { // end of buffer, pull data from wire - this->m_innerBufferSize = m_connection->ReadFromSocket( - context, this->m_readBuffer, Details::c_DefaultLibcurlReaderSize); - this->m_bodyStartInBuffer = 1; // jump first char (could be \r or \n) - } - } + ReadCRLF(context); // Reset session read counter for next chunk this->m_sessionTotalRead = 0; // get the size of next chunk ParseChunkSize(context); if (this->IsEOF()) - { // after parsing next chunk, check if it is zero + { + // Final CRLF after end of chunk + ReadCRLF(context); + // after parsing next chunk, check if it is zero return 0; } } @@ -1115,7 +1134,6 @@ std::unique_ptr CurlConnectionPool::GetCurlConnection( + std::string(curl_easy_strerror(result))); } - /******************** Curl handle options apply to all connections created * The keepAlive option is managed by the session directly. */ diff --git a/sdk/core/azure-core/src/http/curl/curl_session_private.hpp b/sdk/core/azure-core/src/http/curl/curl_session_private.hpp index d4462a634..3d40da60f 100644 --- a/sdk/core/azure-core/src/http/curl/curl_session_private.hpp +++ b/sdk/core/azure-core/src/http/curl/curl_session_private.hpp @@ -45,6 +45,19 @@ namespace Azure { namespace Core { namespace Http { friend class Azure::Core::Test::CurlConnectionPool_connectionPoolTest_Test; #endif private: + /** + * @brief Read one expected byte and throw if it is different than the \p expected + * + */ + void ReadExpected(Context const& context, uint8_t expected); + + /** + * @brief Read `\r\n` from internal buffer or from the wire. + * + * @remark throw if `\r\n` are not the next data read. + */ + void ReadCRLF(Context const& context); + /** * @brief This is used to set the current state of a session. * diff --git a/sdk/core/azure-core/test/ut/curl_session_test.cpp b/sdk/core/azure-core/test/ut/curl_session_test.cpp index 115795bbd..5b8138a17 100644 --- a/sdk/core/azure-core/test/ut/curl_session_test.cpp +++ b/sdk/core/azure-core/test/ut/curl_session_test.cpp @@ -82,6 +82,130 @@ namespace Azure { namespace Core { namespace Test { Azure::Core::Http::CurlConnectionPool::ConnectionPoolIndex.clear(); } + TEST_F(CurlSession, chunkBadFormatResponse) + { + // chunked response with unexpected char at the end + std::string response("HTTP/1.1 200 Ok\r\ntransfer-encoding: chunked\r\n\r\n9\r\n"); + std::string response2("123456789\r\n0\r\n\rx"); + std::string connectionKey("connection-key"); + + // Can't mock the curMock directly from a unique ptr, heap allocate it first and then make a + // unique ptr for it + MockCurlNetworkConnection* curlMock = new MockCurlNetworkConnection(); + EXPECT_CALL(*curlMock, SendBuffer(_, _, _)).WillOnce(Return(CURLE_OK)); + EXPECT_CALL(*curlMock, ReadFromSocket(_, _, _)) + .WillOnce(DoAll( + SetArrayArgument<1>(response.data(), response.data() + response.size()), + Return(response.size()))) + .WillOnce(DoAll( + SetArrayArgument<1>(response2.data(), response2.data() + response2.size()), + Return(response2.size()))); + EXPECT_CALL(*curlMock, GetConnectionKey()).WillRepeatedly(ReturnRef(connectionKey)); + EXPECT_CALL(*curlMock, updateLastUsageTime()); + EXPECT_CALL(*curlMock, DestructObj()); + + // Create the unique ptr to take care about memory free at the end + std::unique_ptr uniqueCurlMock(curlMock); + + // Simulate a request to be sent + Azure::Core::Http::Url url("http://microsoft.com"); + Azure::Core::Http::Request request(Azure::Core::Http::HttpMethod::Get, url); + + { + // Create the session inside scope so it is released and the connection is moved to the pool + auto session = std::make_unique( + request, std::move(uniqueCurlMock), true); + + EXPECT_NO_THROW(session->Perform(Azure::Core::GetApplicationContext())); + auto response = session->GetResponse(); + response->SetBodyStream(std::move(session)); + auto bodyS = response->GetBodyStream(); + + // Read the bodyStream to get all chunks + EXPECT_THROW( + Azure::Core::Http::BodyStream::ReadToEnd(Azure::Core::GetApplicationContext(), *bodyS), + Azure::Core::Http::TransportException); + } + // Clear the connections from the pool to invoke clean routine + Azure::Core::Http::CurlConnectionPool::ConnectionPoolIndex.clear(); + } + + TEST_F(CurlSession, chunkSegmentedResponse) + { + // chunked response - simulate the data that the wire will return on every read + std::string response0("HTTP/1.1 200 Ok\r"); + std::string response1("\ntransfer-encoding:"); + std::string response2(" chunke"); + std::string response3("d\r\n"); + std::string response4("\r"); + std::string response5("\n3\r\n"); + std::string response6("123"); + std::string response7("\r\n0\r\n"); + std::string response8("\r\n"); + + std::string connectionKey("connection-key"); + + // Can't mock the curMock directly from a unique ptr, heap allocate it first and then make a + // unique ptr for it + MockCurlNetworkConnection* curlMock = new MockCurlNetworkConnection(); + EXPECT_CALL(*curlMock, SendBuffer(_, _, _)).WillOnce(Return(CURLE_OK)); + EXPECT_CALL(*curlMock, ReadFromSocket(_, _, _)) + .WillOnce(DoAll( + SetArrayArgument<1>(response0.data(), response0.data() + response0.size()), + Return(response0.size()))) + .WillOnce(DoAll( + SetArrayArgument<1>(response1.data(), response1.data() + response1.size()), + Return(response1.size()))) + .WillOnce(DoAll( + SetArrayArgument<1>(response2.data(), response2.data() + response2.size()), + Return(response2.size()))) + .WillOnce(DoAll( + SetArrayArgument<1>(response3.data(), response3.data() + response3.size()), + Return(response3.size()))) + .WillOnce(DoAll( + SetArrayArgument<1>(response4.data(), response4.data() + response4.size()), + Return(response4.size()))) + .WillOnce(DoAll( + SetArrayArgument<1>(response5.data(), response5.data() + response5.size()), + Return(response5.size()))) + .WillOnce(DoAll( + SetArrayArgument<1>(response6.data(), response6.data() + response6.size()), + Return(response6.size()))) + .WillOnce(DoAll( + SetArrayArgument<1>(response7.data(), response7.data() + response7.size()), + Return(response7.size()))) + .WillOnce(DoAll( + SetArrayArgument<1>(response8.data(), response8.data() + response8.size()), + Return(response8.size()))); + EXPECT_CALL(*curlMock, GetConnectionKey()).WillRepeatedly(ReturnRef(connectionKey)); + EXPECT_CALL(*curlMock, updateLastUsageTime()); + EXPECT_CALL(*curlMock, DestructObj()); + + // Create the unique ptr to take care about memory free at the end + std::unique_ptr uniqueCurlMock(curlMock); + + // Simulate a request to be sent + Azure::Core::Http::Url url("http://microsoft.com"); + Azure::Core::Http::Request request(Azure::Core::Http::HttpMethod::Get, url); + + { + // Create the session inside scope so it is released and the connection is moved to the pool + auto session = std::make_unique( + request, std::move(uniqueCurlMock), true); + + EXPECT_NO_THROW(session->Perform(Azure::Core::GetApplicationContext())); + auto response = session->GetResponse(); + response->SetBodyStream(std::move(session)); + auto bodyS = response->GetBodyStream(); + + // Read the bodyStream to get all chunks + EXPECT_NO_THROW( + Azure::Core::Http::BodyStream::ReadToEnd(Azure::Core::GetApplicationContext(), *bodyS)); + } + // Clear the connections from the pool to invoke clean routine + Azure::Core::Http::CurlConnectionPool::ConnectionPoolIndex.clear(); + } + TEST_F(CurlSession, DoNotReuseConnectionIfDownloadFail) {