Delete set on compound emojis can lead to block with size 0, leading to crash due to underflow

[xyzhangfred]

Hi, we encountered a crash while using the C FFI of yrs. It was due to a block having the size of 0, (not sure how exactly), which leads to overflow when subtracted by 1 (at block_store.rs:452:22). We were able to 100% reproduce it with the same data, but unfortunately we cannot share the date here since it contains sensitive info.
Here is the stack trace:
thread '<unnamed>' panicked at 'attempt to subtract with overflow', yrs/src/block_store.rs:452:22 stack backtrace: 0: rust_begin_unwind at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/panicking.rs:593:5 1: core::panicking::panic_fmt at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/core/src/panicking.rs:67:14 2: core::panicking::panic at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/core/src/panicking.rs:117:5 3: yrs::block_store::BlockStore::get_item_clean_end at /Users/fred_zoom/SourceCode/y-crdt/yrs/src/block_store.rs:452:22 4: yrs::block::Item::repair at /Users/fred_zoom/SourceCode/y-crdt/yrs/src/block.rs:1448:25 5: yrs::update::Update::integrate at /Users/fred_zoom/SourceCode/y-crdt/yrs/src/update.rs:220:33 6: yrs::transaction::TransactionMut::apply_update at /Users/fred_zoom/SourceCode/y-crdt/yrs/src/transaction.rs:559:41 7: ytransaction_apply at /Users/fred_zoom/SourceCode/y-crdt/yffi/src/lib.rs:1091:13 8: main at ./main_debug_crash.cpp:162:18 9: <unknown> note: Some details are omitted, run withRUST_BACKTRACE=full for a verbose backtrace. fatal runtime error: failed to initiate panic, error 5 [1] 36799 abort RUST_BACKTRACE=1 ./yrs-ffi-tests

[xyzhangfred]

Here is the code I am referring to (line 452)

[Horusiath]

Block with 0 length is invalid to begin with. If it's a part of the payload, then the actual issue started before, in code that produced it in the first place.

[xyzhangfred]

I managed to recreate the bug with minimal text, it seems to have something to do with compound emoji (🇿🇩🇿🇩) and deleting.
the following code will lead to a crash

    YDoc* doc = ydoc_new();
    Branch* txt = ytext(doc, "quill");
    YTransaction* txn = ydoc_write_transaction(doc, 0, NULL);
    ytext_insert(txt, txn, 0, "🇿🇿🇿🇿🇩🇩🇩🇿🇩🇩🇩🇩🇩🇿🇩🇩🇿🇩🇿🇩", NULL);
    ytext_remove_range(txt, txn, 0, 5); 

here are the backtrace
[100%] Built target yrs-ffi-tests

thread '' panicked at 'attempt to subtract with overflow', yrs/src/block.rs:1580:21
stack backtrace:

0: rust_begin_unwind
          at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/std/src/panicking.rs:593:5
1: core::panicking::panic_fmt
          at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/core/src/panicking.rs:67:14
2: core::panicking::panic
          at /rustc/5680fa18feaa87f3ff04063800aec256c3d4b4be/library/core/src/panicking.rs:117:5
3: yrs::block::SplittableString::block_offset
          at /Users/fred_zoom/SourceCode/y-crdt/yrs/src/block.rs:1580:21
4: yrs::types::text::remove
          at /Users/fred_zoom/SourceCode/y-crdt/yrs/src/types/text.rs:656:29
5: yrs::types::text::Text::remove_range
          at /Users/fred_zoom/SourceCode/y-crdt/yrs/src/types/text.rs:340:13
6: ytext_remove_range
          at /Users/fred_zoom/SourceCode/y-crdt/yffi/src/lib.rs:1325:5
7: main
          at ./main_debug_crash.cpp:145:5
8: <unknown>
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
fatal runtime error: failed to initiate panic, error 5
[1]    61437 abort      RUST_BACKTRACE=1 ./yrs-ffi-tests

[xyzhangfred]

I edited the title to better describe the issue.

[Horusiath]

Thanks @xyzhangfred . I'll work on it.

[xyzhangfred]

BTW, the initial data that caused the error was produced by our FE which was using Y.js, so might worth checking if this is a issue for Y.js as well

[Horusiath]

@xyzhangfred just for the info: yjs uses UTF-16 encoding (default for JavaScript), while yrs by default uses UTF-8 (default for Rust). While this shouldn't cause problems when applying updates between yjs/yrs, if we're talking about user API, you cannot just copy the same arguments when working with multi-byte characters:

// yjs
const doc = new Y.Doc()
const text = doc.getText('text')
text.insert(0, "🇿🇿🇿🇿🇩🇩🇩🇿🇩🇩🇩🇩🇩🇿🇩🇩🇿🇩🇿🇩")
text.delete(0, 5) // <-- these indexes are calculated in UTF-16 code points

vs.

// yrs / yffi
YDoc* doc = ydoc_new();
Branch* txt = ytext(doc, "quill");
YTransaction* txn = ydoc_write_transaction(doc, 0, NULL);
ytext_insert(txt, txn, 0, "🇿🇿🇿🇿🇩🇩🇩🇿🇩🇩🇩🇩🇩🇿🇩🇩🇿🇩🇿🇩", NULL);
ytext_remove_range(txt, txn, 0, 5);  // these are calculated using UTF-8 byte-lengths

If you want to let yrs recalculate offsets in specific encoding you need to configure it via yrs::Options.offset_kind. For C bindings: yoptions.encoding = Y_OFFSET_UTF16

[xyzhangfred]

@Horusiath yes, what I meant in "the initial data was produced by yjs" is that yjs sends the update to yrs and yrs applies the update (which already contains some malformed data, like blocks with 0 size), which leads to the underflow. In our regular workflow, Yrs never edited the text explicitly (no "ytext_remove_range" or "ytext_insert" or anything alike), only applies updates from yjs.

The code script I pasted was just what I used to try to reproduce the bug. It seems yrs try to determine what the size of the current "char" is, and in case of this emoji it determines it to be 4, which would somehow lead to a crash when the last argument of "ytext_remove_range()" is not a multiple of 4.
ytext_insert(txt, txn, 0, "🇿🇿🇿🇿🇩🇩🇩🇿🇩🇩🇩🇩🇩🇿🇩🇩🇿🇩🇿🇩", NULL); ytext_remove_range(txt, txn, 0, 5); // these are calculated using UTF-8 byte-lengths

[xyzhangfred]

I did some further research and proposed a temporary fix to this issue: pr link.