Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix code scanning alert #1530

Merged
merged 8 commits into from
Aug 3, 2022
Merged

Fix code scanning alert #1530

merged 8 commits into from
Aug 3, 2022

Conversation

esigo
Copy link
Member

@esigo esigo commented Jul 30, 2022

Fixes #1529 (issue)

Changes

uses HTTPS URLs

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Unit tests have been added
  • Changes in public API reviewed

@esigo esigo requested a review from a team as a code owner July 30, 2022 15:05
@codecov
Copy link

codecov bot commented Jul 30, 2022

Codecov Report

Merging #1530 (b2d6a35) into main (b4d8245) will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1530      +/-   ##
==========================================
+ Coverage   84.57%   84.63%   +0.07%     
==========================================
  Files         156      156              
  Lines        4795     4795              
==========================================
+ Hits         4055     4058       +3     
+ Misses        740      737       -3     
Impacted Files Coverage Δ
ext/src/http/client/curl/http_client_curl.cc 81.07% <0.00%> (+1.14%) ⬆️

@@ -329,7 +329,7 @@ TEST_F(BasicCurlHttpTests, SendGetRequestSyncTimeout)
curl::HttpClientSync http_client;

http_client::Headers m1 = {};
auto result = http_client.Get("http://222.222.222.200:19000/get/", m1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering why is the IP address changed? I think it is intended as some invalid/unreachable address. @lalitb

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this address is invalid either

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some local app could listen on https://127.0.0.1:19000?

Copy link
Member

@lalitb lalitb Jul 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also use the address from Documentation Address Block (https://datatracker.ietf.org/doc/html/rfc5737#section-4), as they are not allowed to be used on the public internet, and so would be always unreachable.

192.0.2.0 – 192.0.2.255
198.51.100.0 – 198.51.100.255
233.252.0.0 - 233.252.0.255

Copy link
Member

@owent owent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@esigo
Copy link
Member Author

esigo commented Aug 1, 2022

Hi @owent, do you have any idea why the asan test fails:

ext/test/http/curl_http_test.cc:429: Failure
Value of: sessions[i]->IsSessionActive()
  Actual: false
Expected: true

I can't reproduce it locally.
thanks

@owent
Copy link
Member

owent commented Aug 3, 2022

Hi @owent, do you have any idea why the asan test fails:

ext/test/http/curl_http_test.cc:429: Failure
Value of: sessions[i]->IsSessionActive()
  Actual: false
Expected: true

I can't reproduce it locally. thanks

Some implements of libcurl will connect failed immediately and call the callback in the IO thread if it try to connect a invalid address. And the callback will reset is_session_active_ to false.Maybe we can add GetEventHandler::OnEvent() to set is_called_ and use ASSERT_TRUE(sessions[i]->IsSessionActive() || handlers[i]->is_called_); here.

@esigo
Copy link
Member Author

esigo commented Aug 3, 2022

Hi @owent, do you have any idea why the asan test fails:

ext/test/http/curl_http_test.cc:429: Failure
Value of: sessions[i]->IsSessionActive()
  Actual: false
Expected: true

I can't reproduce it locally. thanks

Some implements of libcurl will connect failed immediately and call the callback in the IO thread if it try to connect a invalid address. And the callback will reset is_session_active_ to false.Maybe we can add GetEventHandler::OnEvent() to set is_called_ and use ASSERT_TRUE(sessions[i]->IsSessionActive() || handlers[i]->is_called_); here.

Thanks @owent, it fixed :)

@esigo esigo enabled auto-merge (squash) August 3, 2022 17:56
@esigo esigo merged commit d299348 into open-telemetry:main Aug 3, 2022
yxue pushed a commit to yxue/opentelemetry-cpp that referenced this pull request Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix code scanning alert - Failure to use HTTPS URLs
4 participants