-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
HeaderMap: C string to string_view conversion follow ups #6580
Comments
Once this is all done, we can reclaim the NUL byte: #6581 |
|
|
Looks like |
Description: `absl::SimpleAtoi` takes `absl::string_view` argument and can easily replace all of the calls to `StringUtil::atoull` that are using Base 10 conversion. This eliminates a significant number of places where a `std::string` needed to be constructed from `string_view` to obtain a C string for `StringUtil:atoull`. Risk Level: Low Testing: `bazel test //test/...` Docs Changes: N/A Release Notes: N/A Part of: Issue #6580 Signed-off-by: Dan Noé <dpn@google.com>
Description: absl::SimpleAtoi takes absl::string_view argument and can replace all of the calls to StringUtil::atoll that are using Base 10 conversion, which turns out to be all of them. This PR removes StringUtil::atoll entirely and migrates all callers to absl::SimpleAtoi. Risk Level: Low Testing: bazel test //test/... Docs Changes: N/A Release Notes: N/A Part of: Issue #6580 Signed-off-by: Dan Noé <dpn@google.com>
The remaining fruit left hanging:
|
Description: Convert `Span::setTag` and `Span::setOperation` to take `absl::string_view` arguments. This avoids unnecessary string construction when the value string originates from a header. Zipkin ultimately requires string construction in some cases, but we can directly convert to an Opentracing `string_view` flavor without any additional construction. Risk Level: Low Testing: `bazel test //test/...` Docs Changes: N/A Release Notes: N/A Part of: Issue #6580 Signed-off-by: Dan Noé <dpn@google.com>
@dnoe one quick thought here: Maybe rename |
I like this idea. It would enable us to write a custom hexToUint which only has to support hex, and then if any future other odd base use cases show up then the user can construct a string and use |
@mattklein123 @dnoe one thought I had was to switch everyone to safe variants of |
Yes agreed. |
Definitely. I'm just wondering if we can skip the generic I guess we might need to think about extensions we aren't aware of here - when we get to this it's probably worth polling the community in case someone is using octal or whatever. |
@dnoe I'm hoping we can also avoid direct reliance on |
This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions. |
Is there any work here remaining? Is this just down to |
I'm not sure of current status. This issue is pretty old so my preference would be to assign an owner and do a fresh pass if we are going to keep this open. |
This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions. |
Marking help wanted, I think we should at least cleanup the atoull. |
Description:
These are various places where we need to follow up to complete removal of C style strings in the header map.
HeaderString::find(const char* str)
can probably be eliminated entirely by converting call sites to usegetStringView().find()
.StringUtil::atoull
currently requires creation of some temporarystd::string
objects because it expects C strings, and also returns a C style string. Most call sites ignore the return value, so they can be converted to useabsl::SimpleAtoi
or a newStringUtil
function that takes and returns string views.UuidUtils::uuidModBy()
should be migrated to takeabsl::string_view
Span::setOperation()
should be migrated to takeabsl::string_view
Context:
#6494
#6564
Action item for CVE-2019-9900
The text was updated successfully, but these errors were encountered: