-
Notifications
You must be signed in to change notification settings - Fork 61
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
[RKOTLIN-1089] Guard decoding issues when core logs invalid utf-8 messages #1792
Conversation
for (std::string::size_type i = 0; i < str.size(); ++i) | ||
ret << " 0x" << std::hex << std::setfill('0') << std::setw(2) << (int)s[i]; | ||
ret << "; "; | ||
ret << "in_begin = " << in_begin << "; "; | ||
ret << "in_end = " << in_end << "; "; | ||
ret << "in_begin = " << (void*) in_begin << "; "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why casting to opaque pointer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If treating in_begin
and in_end
as char *
they will be considered as 0-terminated strings and printed to the stream ... in which case we end up including the invalid UTF-8 bytes in the message - Just as for ret << "StringData.data = " << str << "; ";
. Want to avoid having invalid unicode in the exception message as spelled out in #1792 (comment). Just including the pointers to give some insights if we were to try to understand and reproduce an issue based on one of these exceptions.
@@ -82,13 +82,12 @@ static std::string string_to_hex(const std::string& message, realm::StringData& | |||
ret << "error_code = " << error_code << "; "; | |||
ret << "retcode = " << retcode << "; "; | |||
ret << "StringData.size = " << str.size() << "; "; | |||
ret << "StringData.data = " << str << "; "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're not printing data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string_to_hex
is only used to make a hex-dump of the data when we fail to decode it from UTF-8. So don't think it makes sense to include the non-decodable string itself. The result of this method is used as the message of an exception and we cannot pass that on to the JVM if the message itself is not valid unicode.
jstring j_message = NULL; | ||
try { | ||
j_message = to_jstring(jenv, message); | ||
} catch (RuntimeError exception) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you observe this on Android & JVM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is on both platforms
db2e957
to
7af0a91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should skip printing memory address of begin/end
if there's not plan how to use them
We can actually derive some context from it. The |
Closes #1760