ROX-26025: Sanitize UTF-8 strings in process signals #1857
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This method replaces every invalid UTF-8 character with a '?'
Molter73
reviewed
Sep 23, 2024
collector/lib/Utility.h
Outdated
| @@ -108,6 +108,36 @@ ScopedLock<Mutex> Lock(Mutex& mutex) { | |||
| #define ssizeof(x) static_cast<ssize_t>(sizeof(x)) | |||
|
|
|||
| std::optional<std::string_view> ExtractContainerIDFromCgroup(std::string_view cgroup); | |||
|
|
|||
| // A Holder refers to an object that it can own, or not. | |||
There was a problem hiding this comment.
This sounds like a lifetime nightmare. Can't we do something similar to what protobuf does and simply use pointers? Or maybe make SanitizeUTF8 return a std::optional<std::string>? That way if the return value is empty it would mean the original string is valid and, if it is not empty, it will hold the sanitized string.
There was a problem hiding this comment.
I see your point, I wasn't convinced myself.
The problem is with the immutability of std::string, which makes you copy and copy again. I will figure out a more maintainable way.
ovalenti
force-pushed
the
ovalenti/coerse_utf8_in_process_signals
branch
2 times, most recently
from
4cb0181 to
558f363
Compare
ovalenti
force-pushed
the
ovalenti/coerse_utf8_in_process_signals
branch
from
558f363 to
6226563
Compare
Molter73
reviewed
Sep 23, 2024
Co-authored-by: Mauro Ezequiel Moltrasio <[email protected]>
Co-authored-by: Mauro Ezequiel Moltrasio <[email protected]>
ovalenti
added a commit
that referenced
this pull request
Sep 25, 2024
Replace every byte of an invalid UTF-8 sequence with '?' Co-authored-by: Mauro Ezequiel Moltrasio <[email protected]>
ovalenti
added a commit
that referenced
this pull request
Sep 26, 2024
* ROX-26025: Sanitize UTF-8 strings in process signals (#1857) Replace every byte of an invalid UTF-8 sequence with '?' * Pin ansible version to <2.17 to avoid loop bug (#1837) --------- Co-authored-by: Mauro Ezequiel Moltrasio <[email protected]> Co-authored-by: Giles Hutton <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Containers can produce non-UTF8 process names or paths. We want to prevent this from causing issues in protobuf, which doesn't like it at all.
As a quick workaround, we reuse
google::protobuf::internal::UTF8CoerceToStructurallyValidto replace any invalid sequence with '?'.In order to minimize string copies in the "no modifications required" case, we introduce a Holder<> class to either point to the original unmodified string, or to an altered copy of it.
Checklist