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) {