aws-cpp-sdk-core/source/http/curl/CurlHttpClient.cpp: Bad call to cURL
RuurdBeerstra opened this issue · 6 comments
Describe the bug
During an an update of cURL code a type-error crept in near line 890. The new CURLINFO_APPCONNECT_TIME_T cURL api returns a curl_off_t, the code still uses a double. This causes a nasty floating point exception.
Expected Behavior
no crash.
Current Behavior
Crash.
Reproduction Steps
Build AWS core on Linux, run code that establishes an HTTP(s) session. Stack trace of an example crash (signal SIGFPE, floating point exception):
0x00007ffff6289764 in Aws::Http::CurlHttpClient::MakeRequest (this=0x1cbbf00, request=std::shared_ptr (count 2, weak 0) 0x1cbd230, readLimiter=0x0, writeLimiter=0x0)
at /vobs/obj.dbg.RHAT7/thirdparty/AWS/64bit/build/aws-sdk-cpp/src/aws-cpp-sdk-core/source/http/curl/CurlHttpClient.cpp:896
0x00007ffff6316041 in Aws::Client::AWSClient::<lambda()>::operator()(void) const (__closure=0x7fffffff58d0)
at /vobs/obj.dbg.RHAT7/thirdparty/AWS/64bit/build/aws-sdk-cpp/src/aws-cpp-sdk-core/source/client/AWSClient.cpp:555
0x00007ffff6322d54 in std::_Function_handler<std::shared_ptr<Aws::Http::HttpResponse>(), Aws::Client::AWSClient::AttemptOneRequest(const std::shared_ptr<Aws::Http::HttpRequest>&, const Aws::AmazonWebServiceReq
r const*, char const*, char const*) const::<lambda()> >::_M_invoke(const std::_Any_data &) (__functor=...) at /opt/rh/devtoolset-7/root/usr/include/c++/7/bits/std_function.h:302
0x00007ffff634df2f in std::function<std::shared_ptr<Aws::Http::HttpResponse> ()>::operator()() const (this=0x7fffffff58d0) at /opt/rh/devtoolset-7/root/usr/include/c++/7/bits/std_function.h:706
0x00007ffff633f58d in smithy::components::tracing::TracingUtils::MakeCallWithTiming<std::shared_ptr<Aws::Http::HttpResponse> >(std::function<std::shared_ptr<Aws::Http::HttpResponse> ()>, std::string const&, sm
onents::tracing::Meter const&, std::map<std::string, std::string, std::less<std::string>, std::allocator<std::pair<std::string const, std::string> > >&&, std::string const&) (func=...,
metricName="smithy.client.service_call_duration", meter=..., attributes=..., description="") at /vobs/obj.dbg.RHAT7/thirdparty/AWS/64bit/build/aws-sdk-cpp/src/aws-cpp-sdk-core/include/smithy/tracing/TracingUti
0x00007ffff6316ca3 in Aws::Client::AWSClient::AttemptOneRequest (this=0x1d759c0, httpRequest=std::shared_ptr (count 2, weak 0) 0x1cbd230, request=..., signerName=0x1dc2028 "SignatureV4",
signerRegionOverride=0x1dc1848 "us-east-1", signerServiceNameOverride=0x1dc17b8 "s3") at /vobs/obj.dbg.RHAT7/thirdparty/AWS/64bit/build/aws-sdk-cpp/src/aws-cpp-sdk-core/source/client/AWSClient.cpp:559
0x00007ffff63128f1 in Aws::Client::AWSClient::AttemptExhaustively (this=0x1d759c0, uri=..., request=..., method=Aws::Http::HttpMethod::HTTP_HEAD, signerName=0x1dc2028 "SignatureV4",
signerRegionOverride=0x1dc1848 "us-east-1", signerServiceNameOverride=0x1dc17b8 "s3") at /vobs/obj.dbg.RHAT7/thirdparty/AWS/64bit/build/aws-sdk-cpp/src/aws-cpp-sdk-core/source/client/AWSClient.cpp:281
0x00007ffff631eff9 in Aws::Client::AWSXMLClient::MakeRequest (this=0x1d759c0, uri=..., request=..., method=Aws::Http::HttpMethod::HTTP_HEAD, signerName=0x1dc2028 "SignatureV4",
signerRegionOverride=0x1dc1848 "us-east-1", signerServiceNameOverride=0x1dc17b8 "s3") at /vobs/obj.dbg.RHAT7/thirdparty/AWS/64bit/build/aws-sdk-cpp/src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp:101
0x00007ffff631ea91 in Aws::Client::AWSXMLClient::MakeRequest (this=0x1d759c0, request=..., endpoint=..., method=Aws::Http::HttpMethod::HTTP_HEAD, signerName=0x1dc2028 "SignatureV4",
signerRegionOverride=0x1dc1848 "us-east-1", signerServiceNameOverride=0x1dc17b8 "s3") at /vobs/obj.dbg.RHAT7/thirdparty/AWS/64bit/build/aws-sdk-cpp/src/aws-cpp-sdk-core/source/client/AWSXmlClient.cpp:68
0x00007ffff7562158 in Aws::S3::S3Client::<lambda()>::operator()(void) const (__closure=0x1dc0a10) at /vobs/obj.dbg.RHAT7/thirdparty/AWS/64bit/build/aws-sdk-cpp/generated/src/aws-cpp-sdk-s3/source/S3Client.cpp:
0x00007ffff75b0aab in std::_Function_handler<Aws::Utils::Outcome<Aws::S3::Model::HeadObjectResult, Aws::S3::S3Error>(), Aws::S3::S3Client::HeadObject(const Aws::S3::Model::HeadObjectRequest&) const::<lambda()>
oke(const std::_Any_data &) (__functor=...) at /opt/rh/devtoolset-7/root/usr/include/c++/7/bits/std_function.h:302
0x00007ffff7609b91 in std::function<Aws::Utils::Outcome<Aws::S3::Model::HeadObjectResult, Aws::S3::S3Error> ()>::operator()() const (this=0x7fffffff7a10)
at /opt/rh/devtoolset-7/root/usr/include/c++/7/bits/std_function.h:706
0x00007ffff75f261e in smithy::components::tracing::TracingUtils::MakeCallWithTiming<Aws::Utils::Outcome<Aws::S3::Model::HeadObjectResult, Aws::S3::S3Error> >(std::function<Aws::Utils::Outcome<Aws::S3::Model::H
esult, Aws::S3::S3Error> ()>, std::string const&, smithy::components::tracing::Meter const&, std::map<std::string, std::string, std::less<std::string>, std::allocator<std::pair<std::string const, std::string> > >&
ring const&) (func=..., metricName="smithy.client.duration", meter=..., attributes=..., description="")
at /vobs/obj.dbg.RHAT7/thirdparty/AWS/64bit/build/aws-sdk-cpp/src/aws-cpp-sdk-core/include/smithy/tracing/TracingUtils.h:66
0x00007ffff7563214 in Aws::S3::S3Client::HeadObject (this=0x1d759c0, request=...) at /vobs/obj.dbg.RHAT7/thirdparty/AWS/64bit/build/aws-sdk-cpp/generated/src/aws-cpp-sdk-s3/source/S3Client.cpp:2721
Possible Solution
We've patched the code as follows:
#if LIBCURL_VERSION_NUM >= 0x073700 // 7.55.0
curl_off_t AppConnectT;
ret = curl_easy_getinfo(connectionHandle, CURLINFO_APPCONNECT_TIME_T, &AppConnectT); // Ssl Latency
if (ret == CURLE_OK)
{
request->AddRequestMetric(GetHttpClientMetricNameByType(HttpClientMetricsType::SslLatency), static_cast<int64_t>(AppConnectT * 1000));
}
#else
ret = curl_easy_getinfo(connectionHandle, CURLINFO_APPCONNECT_TIME, &timep); // Ssl Latency
if (ret == CURLE_OK)
{
request->AddRequestMetric(GetHttpClientMetricNameByType(HttpClientMetricsType::SslLatency), static_cast<int64_t>(timep * 1000));
}
#endifAdditional Information/Context
The fix works.
AWS CPP SDK version used
1.11.277
Compiler and Version used
g++ (GCC) 7.3.1 20180303 (Red Hat 7.3.1-5)
Operating System and version
Linux i-0ea8deb92c374.stable.infordev.local 5.10.209-198.858.amzn2.x86_64 #1
Thanks for pointing this out, we're testing your fix
Hi, thank you for the fix. However, on the very next line there is a similar issue, but there the logic is reversed:
904 curl_off_t speed;
905 #if LIBCURL_VERSION_NUM >= 0x073700 // 7.55.0
906 ret = curl_easy_getinfo(connectionHandle, CURLINFO_SPEED_DOWNLOAD_T, &speed); // throughput
907 #else
908 ret = curl_easy_getinfo(connectionHandle, CURLINFO_SPEED_DOWNLOAD, &speed); // throughput
909 #endif
The CURLINFO_SPEED_DOWNLOAD (old) takes a pointer-to-double, the CURLINFO_SPEED_DOWNLOAD_T (new) takes a pointer to curl_off_t.
That will not cause a floating point exception, but is is incorrect code. Version 7.55.0 is from August 2017, so it is unlikely that this will cause issues anywhere (and it works for us), but I had hoped this would have been spotted by a review on the fix...
And a co-worker just pointed out that the "curl_off_t speed" is also mis-used for CURLINFO_SPEED_UPLOAD_T vs. CURLINFO_SPEED_UPLOAD a few lines further down.
created another PR, take a look and approve if everything looks good to you
That seems like a proper solution. I think in this case a comment might be warranted to explain the slightly tricky definition of either a double or curl_off_t of 'metric', but I'll leave that up to you.
Thank you for the quick support!
This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.