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

Encode the file path in url authority so we don't crash on whitespaces #499

Merged
merged 3 commits into from
Jul 3, 2024

Conversation

paullegranddc
Copy link
Contributor

What does this PR do?

Encode the file path in url authority so we don't crash on whitespaces

Motivation

The telemetry lib would fail because of a panic on URI violation, because we were trying to encode the file path in the url path, which doesn't accept some characters.

This PR encodes the path as hex, like we do for unix socket and windows pipe.

Additional Notes

Anything else we should know when reviewing?

How to test the change?

Describe here in detail how the change can be validated.

@codecov-commenter
Copy link

codecov-commenter commented Jun 21, 2024

Codecov Report

Attention: Patch coverage is 71.73913% with 13 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@88cb232). Learn more about missing BASE report.
Report is 14 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #499   +/-   ##
=======================================
  Coverage        ?   70.39%           
=======================================
  Files           ?      204           
  Lines           ?    27843           
  Branches        ?        0           
=======================================
  Hits            ?    19600           
  Misses          ?     8243           
  Partials        ?        0           
Components Coverage Δ
crashtracker 16.72% <0.00%> (?)
datadog-alloc 98.76% <ø> (?)
data-pipeline 51.15% <ø> (?)
data-pipeline-ffi 0.00% <ø> (?)
ddcommon 86.22% <95.65%> (?)
ddcommon-ffi 75.31% <100.00%> (?)
ddtelemetry 59.33% <84.61%> (?)
ipc 84.66% <ø> (?)
profiling 78.63% <ø> (?)
profiling-ffi 58.19% <ø> (?)
serverless 0.00% <ø> (?)
sidecar 36.19% <ø> (?)
sidecar-ffi 0.00% <ø> (?)
spawn-worker 54.98% <ø> (?)
trace-mini-agent 69.70% <ø> (?)
trace-normalization 97.79% <ø> (?)
trace-obfuscation 95.75% <ø> (?)
trace-protobuf 77.16% <ø> (?)
trace-utils 91.21% <ø> (?)

Copy link
Contributor

@bantonsson bantonsson left a comment

Choose a reason for hiding this comment

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

So why don't we URL encode the path with %20 for space etc?

@chrisnas
Copy link

chrisnas commented Jun 24, 2024

So why don't we URL encode the path with %20 for space etc?

Would %20 work for a file path on Windows?

Since I found this issue by calling ddog_endpoint_from_url(), would it be possible to add a test to validate that it does not return a null pointer?

@paullegranddc
Copy link
Contributor Author

So why don't we URL encode the path with %20 for space etc?

I reused the existing method to encode paths in urls. In the end it doesn't really matter because it's just an internal representation.
As to why we use the authority instead of the path of the uri to smuggle the path for unix sockets and windows, it was because the authority of hyper::Uri needs to be non empty anyway, and I was afraid the authority was used in hyper client to implement connection reuse, so a connection to a socket would be reused to connect to another path.

Copy link
Contributor

@bantonsson bantonsson left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation.

@paullegranddc paullegranddc force-pushed the paullgdc/encode_file_uri_to_support_spaces branch from 115b5b2 to ddcb56d Compare July 1, 2024 13:44
@paullegranddc paullegranddc merged commit ce2afd9 into main Jul 3, 2024
30 of 31 checks passed
@paullegranddc paullegranddc deleted the paullgdc/encode_file_uri_to_support_spaces branch July 3, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants