GlyphStore out-of-bounds access with multiple combining characters

[kmcallister]

Sourced from http://en.wikipedia.org/wiki/Wikipedia. The text is অসমীয়া ; base64'd below.

Steps to reproduce:

RUST_LOG='gfx::text::glyph=4,gfx::font=2' \
./servo 'data:text/html;base64,PGh0bWw+PGhlYWQ+PG1ldGEgY2hhcnNldD0iVVRGLTgiPjwvaGVhZD48Ym9keT7gpoXgprjgpq7gp4Dgpq/gprzgpr48L2JvZHk+PC9odG1sPgo='

Output (with some of my own debug statements included):

rust: ~"making GlyphStore for ~\"\\u0985\\u09b8\\u09ae\\u09c0\\u09af\\u09bc\\u09be\" = 7 characters"
rust: ~"add_glyph_for_char_index: index 0, my entries are ~[{value: 0}, {value: 0}, {value: 0}, {value: 0}, {value: 0}, {value: 0}, {value: 0}]"
rust: ~"add_glyph_for_char_index: index 1, my entries are ~[{value: 2194669568}, {value: 0}, {value: 0}, {value: 0}, {value: 0}, {value: 0}, {value: 0}]"
rust: ~"add_glyph_for_char_index: index 2, my entries are ~[{value: 2194669568}, {value: 2194669568}, {value: 0}, {value: 0}, {value: 0}, {value: 0}, {value: 0}]"
rust: ~"add_glyph_for_char_index: index 3, my entries are ~[{value: 2194669568}, {value: 2194669568}, {value: 2194669568}, {value: 0}, {value: 0}, {value: 0}, {value: 0}]"
rust: ~"Adding entry[off=4] for detailed glyphs: &[{index: 0, advance: {__field__: 720}, offset: {x: {__field__: 0}, y: {__field__: 0}}}, {index: 0, advance: {__field__: 0}, offset: {x: {__field__: 0}, y: {__field__: 0}}}]"
rust: ~"creating complex glyph entry: starts_cluster=true, starts_ligature=false, glyph_count=2"
rust: ~"Adding multiple glyphs[idx=4, count=2]: {value: 517}"
rust: ~"creating complex glyph entry: starts_cluster=false, starts_ligature=false, glyph_count=0"
rust: ~"adding spacer for chracter without associated glyph[idx=5]"
rust: ~"creating complex glyph entry: starts_cluster=false, starts_ligature=false, glyph_count=0"
rust: ~"adding spacer for chracter without associated glyph[idx=6]"
rust: ~"add_glyph_for_char_index: index 7, my entries are ~[{value: 2194669568}, {value: 2194669568}, {value: 2194669568}, {value: 2194669568}, {value: 517}, {value: 7}, {value: 7}]"
rust: task failed at 'assertion failed: i < self.entry_buffer.len()', /home/keegan/proj/servo/servo/src/components/gfx/text/glyph.rs:549
rust: domain main @0x7f3e780103e0 root task failed
rust: task failed at 'killed', /home/keegan/proj/servo/servo/src/compiler/rust/src/libstd/pipes.rs:282
rust: task failed at 'killed', /home/keegan/proj/servo/servo/src/compiler/rust/src/libstd/pipes.rs:282
rust: task failed at 'killed', /home/keegan/proj/servo/servo/src/compiler/rust/src/libstd/pipes.rs:282
rust: task failed at 'killed', /home/keegan/proj/servo/servo/src/compiler/rust/src/libstd/pipes.rs:282
rust: task failed at 'killed', /home/keegan/proj/servo/servo/src/compiler/rust/src/libstd/pipes.rs:282
rust: task failed at 'killed', /home/keegan/proj/servo/servo/src/compiler/rust/src/libstd/pipes.rs:282
rust: task failed at 'killed', /home/keegan/proj/servo/servo/src/compiler/rust/src/libstd/pipes.rs:282
rust: task failed at 'killed', /home/keegan/proj/servo/servo/src/compiler/rust/src/libstd/pipes.rs:282
rust: task failed at 'killed', /home/keegan/proj/servo/servo/src/compiler/rust/src/libstd/pipes.rs:282
rust: task failed at 'killed', /home/keegan/proj/servo/servo/src/compiler/rust/src/libstd/pipes.rs:282
rust: task failed at 'killed', /home/keegan/proj/servo/servo/src/compiler/rust/src/libstd/pipes.rs:282
Segmentation fault

[kmcallister]

Okay, the crash happens whenever we try to put multiple combining characters on the same base character. For example, two umlauts on an n:

./servo 'data:;base64,'$(printf '<p>n\xcc\x88\xcc\x88</p>' | base64)

[burg]

I'm not surprised at all that this doesn't work yet :-) The storage strategy for glyphs is pretty much the same as Gecko/thebes, so it shouldn't be too hard to resume work. I think I was focusing more on multi-byte characters (Japanese) than combining chars, since I know Japanese and not complex script languages.

[burg]

It would be nice to separately test the case where \xcc is used to glue arbitrary glyphs together, versus a combining character such as combining-acute (and ostensibly the Devanagari-looking original test case)

[jdm]

Well, the crash is gone. Should this be rescoped to making combining characters show up?

[burg]

Yes. (Good luck.)

On Apr 18, 2014, at 7:40 AM, Josh Matthews notifications@github.com wrote:

Well, the crash is gone. Should this be rescoped to making combining characters show up?

—
Reply to this email directly or view it on GitHub.

[shinglyu]

Is this related? I got a crash like this:

% ./mach run tests/html/combining-character-sequences.html                                                                                                                1 ↵ ✹ ✭

thread 'LayoutWorker worker 1/3' panicked at 'assertion failed: glyph_count <= char_max', /home/shinglyu/workspace/servo/components/gfx/text/shaping/harfbuzz.rs:283
stack backtrace:
   1:     0x7f3ef6a2aab0 - sys::backtrace::write::ha4a9415e174db438Rsy
   2:     0x7f3ef6a3fa90 - failure::on_fail::hac5db0a0a6a39fd6LWF
   3:     0x7f3ef6a0eea0 - rt::unwind::begin_unwind_inner::h6e98ac6f3dbe1e23iBF
   4:     0x7f3ef6127330 - rt::unwind::begin_unwind::h7047728535879307745
   5:     0x7f3ef62371c0 - text::shaping::harfbuzz::Shaper::save_glyph_results::h90e1d038fcfc8896iLh
   6:     0x7f3ef61b6190 - text::shaping::harfbuzz::Shaper.ShaperMethods::shape_text::hc46115752dbb2f0fjJh
   7:     0x7f3ef61b2de0 - font::Font::shape_text::h5a317f38c035569eV3d
   8:     0x7f3ef623e800 - text::text_run::TextRun::break_and_shape::he2cb9af27637d8dfTAi
   9:     0x7f3ef61bf2b0 - text::text_run::TextRun::new::h64c3a9d871480f426zi
  10:     0x7f3ef578e400 - text::TextRunScanner::flush_clump_to_list::h414c602f66e75378nTq
  11:     0x7f3ef55eb0f0 - text::TextRunScanner::scan_for_runs::h1ac8f66bc116e49aIPq
  12:     0x7f3ef55f4930 - construct::FlowConstructor<'a>::build_flow_for_block_starting_with_fragment::hae2689a2a567a63eYkd
  13:     0x7f3ef55fa4f0 - construct::FlowConstructor<'a>::build_flow_for_block::ha61653748008651eOnd
  14:     0x7f3ef55fcf60 - construct::FlowConstructor<'a>::build_flow_for_nonfloated_block::h28f2a816cca9c6c7rpd
  15:     0x7f3ef55f61c0 - construct::FlowConstructor<'a>.PostorderNodeMutTraversal::process::hc9340c4d103ffd819Rd
  16:     0x7f3ef577cfa0 - traversal::ConstructFlows<'a>.PostorderDomTraversal::process::h25b465971a41e78b6qr
  17:     0x7f3ef577cdb0 - parallel::ParallelPostorderDomTraversal::run_parallel::h469996496867798057
  18:     0x7f3ef577ccb0 - parallel::construct_flows::h935c54896b33b34aEgo
  19:     0x7f3ef577bd00 - parallel::RecalcStyleForNode<'a>.ParallelPreorderDomTraversal::run_parallel::h47a961611ec0efb7sfo
  20:     0x7f3ef577cc00 - parallel::recalc_style::h9d91ca55163bc0a6Xfo
  21:     0x7f3ef5726dc0 - workqueue::WorkerThread<QueueData, WorkData>::start::h10097103087649136029
  22:     0x7f3ef5726c30 - workqueue::WorkQueue<QueueData, WorkData>::new::closure.31552
  23:     0x7f3ef5726950 - task::spawn_named::closure.31545
  24:     0x7f3ef5726850 - thunk::Thunk<(), R>::new::closure.31541
  25:     0x7f3ef57266e0 - thunk::F.Invoke<A, R>::invoke::h14488316712187114163
  26:     0x7f3ef5663620 - thunk::Thunk<A, R>::invoke::h2461764668060504916
  27:     0x7f3ef5664320 - thread::Builder::spawn_inner::closure.28627
  28:     0x7f3ef56642b0 - rt::unwind::try::try_fn::__rust_abi::h1151427703532831683
  29:     0x7f3ef5664270 - rt::unwind::try::try_fn::h1151427703532831683
  30:     0x7f3ef6a49e50 - rust_try_inner
  31:     0x7f3ef6a49e40 - rust_try
  32:     0x7f3ef56636b0 - rt::unwind::try::h11773544114778286838
  33:     0x7f3ef5662400 - thread::Builder::spawn_inner::closure.28526
  34:     0x7f3ef56652b0 - thunk::Thunk<(), R>::new::closure.28651
  35:     0x7f3ef5665190 - thunk::F.Invoke<A, R>::invoke::h4396906089258962130
  36:     0x7f3ef6a2e360 - sys::thread::thread_start::hc5bbfed3723b5236ICB
  37:     0x7f3ef41650c0 - start_thread
  38:     0x7f3ef366cfd9 - __clone
  39:                0x0 - <unknown>

[jdm]

Sounds like a different failure than the original one reported, but should definitely be fixed!

[shinglyu]

Let me create a new issue for that later today

On Wed, 11 Mar 2015 at 22:48 Josh Matthews notifications@github.com wrote:

Sounds like a different failure than the original one reported, but should
definitely be fixed!

—
Reply to this email directly or view it on GitHub
#582 (comment).

[mbrubeck]

These are both fixed, possibly by #10895.