github/azure-sdk-for-cpp
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Fix the end of chunk parsing (#1403)

Browse Source 
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
This commit is contained in:
Victor Vazquez 2021-01-19 21:57:45 -08:00 committed by GitHub
parent 7d04092708
commit 3f67c21ba8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 175 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
sdk/core/azure-core/CHANGELOG.md
View File

@ -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

50
sdk/core/azure-core/src/http/curl/curl.cpp
View File

@ -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<CurlNetworkConnection> 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.
*/

13
sdk/core/azure-core/src/http/curl/curl_session_private.hpp
View File

@ -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.
*

124
sdk/core/azure-core/test/ut/curl_session_test.cpp
View File

@ -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<MockCurlNetworkConnection> 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<Azure::Core::Http::CurlSession>(
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<MockCurlNetworkConnection> 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<Azure::Core::Http::CurlSession>(
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)
{