Fix handling of 4-byte UTF8 values on Windows #5803

kneth · 2022-08-31T15:38:38Z

What, How & Why?

Fixes #5825

Some unused error handling is removed from ParentNode. Replaced by throwing.

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
C-API, if public C++ API changed.

jedelbo · 2022-09-06T11:57:19Z

@astigsen perhaps you can have a look at the changes in unicode.cpp.

ironage · 2022-09-06T16:17:53Z

src/realm/query_engine.hpp

@@ -1582,7 +1571,7 @@ class StringNode : public StringNodeBase {
        auto upper = case_map(v, true);
        auto lower = case_map(v, false);
        if (!upper || !lower) {
-            error_code = "Malformed UTF-8: " + std::string(v);
+            throw query_parser::InvalidQueryError(util::format("Malformed UTF-8: %1", v));


These are not "query parser" errors, they are general query errors. Can we use another name?

Ok - changed to std::runtime_error.

ironage · 2022-09-06T16:28:04Z

src/realm/unicode.cpp

    const char* begin = source.data();
    const char* end = begin + source.size();
    auto output = result.begin();
    while (begin != end) {
-        int n = static_cast<int>(sequence_length(*begin));
-        if (n == 0 || end - begin < n)


We still need something like this check don't we? What if there are random bytes at the end of the string which appear to be a multibyte character but are just garbage such that sequence_length() returns 4 but there is only one character left in the string?

Changed the logic a bit so that trailing garbage should be handled. I did not have a chance to test it on a Windows machine today. I will do that tomorrow.

Thanks, I'd be more convinced that this works if there was a test for trailing garbage as well 😄

Perhaps we should not specify how trailing garbage would be treated and fall back to "garbage in -> garbage out". This is anyway how the non-windows version works.

Sure, let's match the non-windows version. Can you add a test with trailing garbage to check the consistency though?

I don't think I can make a test that has the same outcome on both Windows and Linux. On Linux we will just copy over anything that is not an ASCII letter, but on Windows it will fail if there is something that cannot be translated. That was what I meant with my comment: We should not specify how it behaves as is behaves differently.

Ok, I will concede platform dependent behaviour here. However, I'd still request a (platform dependent) test of garbage characters to verify that we do not crash.

ironage · 2022-09-07T20:16:25Z

src/realm/exceptions.hpp

@@ -176,6 +176,25 @@ class NoSubscriptionForWrite : public std::runtime_error {
    NoSubscriptionForWrite(const std::string& msg);
 };

+namespace query_parser {


I should have put my previous comment on naming here. Since these exceptions are thrown from the query engine now and not necessarily from the query parser, can we change this namespace to something more general such as query?

Let's not change this in this PR.

Ok sure. In fact, I'd expect all these exceptions will changing soon with the upcoming error handling work.

ironage

LGTM, after addressing clang-format 👍

…nification * origin/master: Stop forcing enums to be 64 bits unnecessarily clean up documentation of internal fields in config structs SyncConfig should be default constructible Traversal functions use IteratorControl values rather than true/false which is more expressive (#5857) Fix handling of 4-byte UTF8 values on Windows (#5803) Encode links in a way the server can understand (#5835) expose `Group::remove_table` in the C API (#5860) Disable auto refresh for old realm during migration (#5856) Expose `list_find` in the c api (#5848) Do not allow asymmetric tables in pbs (#5853) Refactor link tracing code (#5796) Expose `Obj::get_parent_object` in the C API (#5851) Update app.hpp (#5854) Fix appending to list ignores existing query (#5850)

cla-bot bot added the cla: yes label Aug 31, 2022

kneth self-assigned this Aug 31, 2022

Try to reproduce https://github.com/realm/realm-js/issues/4844

c47cbdf

kneth force-pushed the kneth/bug/contains-with-emoiji branch from c871631 to c47cbdf Compare August 31, 2022 15:50

kneth mentioned this pull request Aug 31, 2022

Realm query using CONTAINS with [c] case insensitive crashes Realm when an emoji is in search string #5825

Closed

jedelbo added 2 commits September 6, 2022 11:30

Improve handling of UTF8 error on Windows

284bdcd

Update test

479b026

jedelbo force-pushed the kneth/bug/contains-with-emoiji branch from 38dabaa to 479b026 Compare September 6, 2022 10:00

jedelbo added 3 commits September 6, 2022 13:00

Fix Windows unicode conversion

5f9f51c

Merge branch 'master' into kneth/bug/contains-with-emoiji

cee09ea

Update release notes

0058012

sync-by-unito bot mentioned this pull request Sep 6, 2022

Realm query using CONTAINS with [c] case insensitive crashes Realm when an emoji is in search string #5826

Closed

jedelbo changed the title ~~Try to reproduce https://github.com/realm/realm-js/issues/4844~~ Fix handling of 4-byte UTF8 values on Windows Sep 6, 2022

jedelbo requested review from ironage and astigsen September 6, 2022 11:59

Optimized case_map on Windows

9f380e5

ironage reviewed Sep 6, 2022

View reviewed changes

Update after review

a7697c5

ironage reviewed Sep 7, 2022

View reviewed changes

jedelbo force-pushed the kneth/bug/contains-with-emoiji branch from 7f3d7c3 to 91f94e9 Compare September 14, 2022 09:15

ironage approved these changes Sep 14, 2022

View reviewed changes

Improve case_map function on non-Windows platforms

22c91fa

jedelbo force-pushed the kneth/bug/contains-with-emoiji branch from 91f94e9 to 22c91fa Compare September 15, 2022 09:53

Merge branch 'master' into kneth/bug/contains-with-emoiji

950ee90

jedelbo assigned jedelbo and unassigned kneth Sep 15, 2022

jedelbo added 2 commits September 15, 2022 16:16

Update Package.swift

e1195ba

Fix warning

fc85d52

jedelbo merged commit 6b04398 into master Sep 15, 2022

jedelbo deleted the kneth/bug/contains-with-emoiji branch September 15, 2022 15:14

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of 4-byte UTF8 values on Windows #5803

Fix handling of 4-byte UTF8 values on Windows #5803

kneth commented Aug 31, 2022 •

edited by jedelbo

Loading

jedelbo commented Sep 6, 2022

ironage Sep 6, 2022

jedelbo Sep 7, 2022

ironage Sep 6, 2022

jedelbo Sep 7, 2022

ironage Sep 7, 2022

jedelbo Sep 8, 2022

ironage Sep 8, 2022

jedelbo Sep 9, 2022

ironage Sep 9, 2022

ironage Sep 7, 2022

jedelbo Sep 8, 2022

ironage Sep 8, 2022

ironage left a comment

Fix handling of 4-byte UTF8 values on Windows #5803

Fix handling of 4-byte UTF8 values on Windows #5803

Conversation

kneth commented Aug 31, 2022 • edited by jedelbo Loading

What, How & Why?

☑️ ToDos

jedelbo commented Sep 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironage left a comment

Choose a reason for hiding this comment

kneth commented Aug 31, 2022 •

edited by jedelbo

Loading