After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 776932 - Singular matrices crash librsvg renderers (Rust panicked)
Singular matrices crash librsvg renderers (Rust panicked)
Status: RESOLVED FIXED
Product: librsvg
Classification: Core
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: Federico Mena Quintero
librsvg maintainers
Depends on:
Blocks:
 
 
Reported: 2017-01-06 07:24 UTC by Massimo
Modified: 2017-03-16 14:48 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Massimo 2017-01-06 07:24:13 UTC
Running for example

rsvg-convert -b white -w 480 -h 360 coords-trans-09-t.svg -o out.png

results in

thread '<unnamed>' panicked at 'Cairo error "invalid matrix (not invertible)"', /home/massimo/.cargo/git/checkouts/cairo-e6055a6d391c3fc9/30699593d8949a6306f29a7bf9614da1befad561/cairo-sys-rs/src/enums.rs:70


See https://www.w3.org/Graphics/SVG/Test/20110816/harness/htmlSVGWeb/coords-trans-09-t.html

it uses a <g transform="matrix(0 0 0 0 0 0)"> element
so I think it should be considered valid input.
Comment 1 Federico Mena Quintero 2017-03-03 15:48:36 UTC
I'm working on this.

My strategy is to bite the bullet and finally add error tracking to librsvg.

A Node will hold a "result" field, which is a Result<(), node::Error>.  In turn, the node::Error will be able to hold things like parse errors or invalid value errors for attributes.  These will be collected when nodes are being created, and when they set_atts() to parse their attributes.

At rendering time, we will ignore elements which hold an error result.  I am not decided if this should be done at the node level, or at the code which calls Node::draw() yet.

In this specific bug, the crash happens here:

  • #0 raise
  • #1 abort
  • #2 std::sys::imp::abort_internal
    at /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/sys/unix/mod.rs line 175
  • #3 std::sys_common::util::abort
    at /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/sys_common/util.rs line 43
  • #4 std::panicking::rust_panic
    at /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/panicking.rs line 586
  • #5 std::panicking::rust_panic_with_hook
    at /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/panicking.rs line 569
  • #6 std::panicking::begin_panic<collections::string::String>
    at /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/panicking.rs line 515
  • #7 std::panicking::begin_panic_fmt
    at /buildslave/rust-buildbot/slave/stable-dist-rustc-linux/build/src/libstd/panicking.rs line 499
  • #8 cairo_sys::enums::{{impl}}::ensure_valid
    at /home/federico/.cargo/git/checkouts/cairo-e6055a6d391c3fc9/411ffb9/cairo-sys-rs/src/enums.rs line 70
  • #9 cairo::matrices::{{impl}}::invert
    at /home/federico/.cargo/git/checkouts/cairo-e6055a6d391c3fc9/411ffb9/src/matrices.rs line 84
  • #10 rsvg_internals::bbox::rsvg_bbox_insert
    at /home/federico/src/librsvg-latest/rust/src/bbox.rs line 55
  • #11 rsvg_cairo_render_path_builder
    at rsvg-cairo-draw.c line 476
  • #12 rsvg_render_path_builder
    at rsvg-base.c line 2332
  • #13 rsvg_internals::drawing_ctx::render_path_builder
    at /home/federico/src/librsvg-latest/rust/src/drawing_ctx.rs line 144
  • #14 rsvg_internals::shapes::render_path_builder
    at /home/federico/src/librsvg-latest/rust/src/shapes.rs line 24
  • #15 rsvg_internals::shapes::{{impl}}::draw
    at /home/federico/src/librsvg-latest/rust/src/shapes.rs line 412
  • #16 rsvg_internals::node::{{impl}}::draw
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 134
  • #17 rsvg_internals::node::rsvg_node_draw
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 277
  • #18 rsvg_node_draw_from_stack
    at rsvg-structure.c line 76
  • #19 draw_child
    at rsvg-structure.c line 91
  • #20 rsvg_internals::node::rsvg_node_foreach_child
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 291
  • #21 _rsvg_node_draw_children
    at rsvg-structure.c line 106
  • #22 rsvg_group_draw
    at rsvg-structure.c line 121
  • #23 rsvg_internals::cnode::{{impl}}::draw
    at /home/federico/src/librsvg-latest/rust/src/cnode.rs line 27
  • #24 rsvg_internals::node::{{impl}}::draw
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 134
  • #25 rsvg_internals::node::rsvg_node_draw
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 277
  • #26 rsvg_node_draw_from_stack
    at rsvg-structure.c line 76
  • #27 draw_child
    at rsvg-structure.c line 91
  • #28 rsvg_internals::node::rsvg_node_foreach_child
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 291
  • #29 _rsvg_node_draw_children
    at rsvg-structure.c line 106
  • #30 rsvg_group_draw
    at rsvg-structure.c line 121
  • #31 rsvg_internals::cnode::{{impl}}::draw
    at /home/federico/src/librsvg-latest/rust/src/cnode.rs line 27
  • #32 rsvg_internals::node::{{impl}}::draw
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 134
  • #33 rsvg_internals::node::rsvg_node_draw
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 277
  • #34 rsvg_node_draw_from_stack
    at rsvg-structure.c line 76
  • #35 draw_child
    at rsvg-structure.c line 91
  • #36 rsvg_internals::node::rsvg_node_foreach_child
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 291
  • #37 _rsvg_node_draw_children
    at rsvg-structure.c line 106
  • #38 rsvg_group_draw
    at rsvg-structure.c line 121
  • #39 rsvg_internals::cnode::{{impl}}::draw
    at /home/federico/src/librsvg-latest/rust/src/cnode.rs line 27
  • #40 rsvg_internals::node::{{impl}}::draw
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 134
  • #41 rsvg_internals::node::rsvg_node_draw
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 277
  • #42 rsvg_node_draw_from_stack
    at rsvg-structure.c line 76
  • #43 draw_child
    at rsvg-structure.c line 91
  • #44 rsvg_internals::node::rsvg_node_foreach_child
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 291
  • #45 rsvg_node_svg_draw
    at rsvg-structure.c line 205
  • #46 rsvg_internals::cnode::{{impl}}::draw
    at /home/federico/src/librsvg-latest/rust/src/cnode.rs line 27
  • #47 rsvg_internals::node::{{impl}}::draw
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 134
  • #48 rsvg_internals::node::rsvg_node_draw
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 277
  • #49 rsvg_node_draw_from_stack
    at rsvg-structure.c line 76
  • #50 rsvg_handle_render_cairo_sub
    at rsvg-cairo-render.c line 244
  • #51 render_to_surface
    at rsvg-view.c line 200
  • #52 main
    at rsvg-view.c line 738
  • #0 rsvg_state_reinherit_top
    at rsvg-styles.c line 1738
  • #1 _rsvg_node_draw_children
    at rsvg-structure.c line 101
  • #2 rsvg_group_draw
    at rsvg-structure.c line 121
  • #3 rsvg_internals::cnode::{{impl}}::draw
    at /home/federico/src/librsvg-latest/rust/src/cnode.rs line 27
  • #4 rsvg_internals::node::{{impl}}::draw
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 134
  • #5 rsvg_internals::node::rsvg_node_draw
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 277
  • #6 rsvg_node_draw_from_stack
    at rsvg-structure.c line 76
  • #7 draw_child
    at rsvg-structure.c line 91
  • #8 rsvg_internals::node::rsvg_node_foreach_child
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 291
  • #9 _rsvg_node_draw_children
    at rsvg-structure.c line 106
  • #10 rsvg_group_draw
    at rsvg-structure.c line 121

The 0-transform from the group, which comes in the "state" argument to rsvg_state_reinherit_top()...

(gdb) p state->affine
$73 = {xx = 0, yx = 0, xy = 0, yy = 0, x0 = 0, y0 = 0}

The ctx->state has a valid, identity transform at the start of this function:

(gdb) p ctx->state->affine
$74 = {xx = 1, yx = 0, xy = 0, yy = 1, x0 = 0, y0 = 0}

But by the time rsvg_state_reinherit_top() finishes, the ctx->state has inherited the 0-transform:

(gdb) finish
Run till exit from #0  rsvg_state_reinherit_top (ctx=0x6ffa60, state=0x8d9d40, dominate=0) at rsvg-styles.c:1738
103             rsvg_push_discrete_layer (ctx);
(gdb) p ctx->state->affine
$75 = {xx = 0, yx = 0, xy = 0, yy = 0, x0 = 0, y0 = 0}

The surrounding code, for context:

(gdb) l
98      _rsvg_node_draw_children (RsvgNode *node, RsvgDrawingCtx *ctx, int dominate)
99      {
100         if (dominate != -1) {
101             rsvg_state_reinherit_top (ctx, rsvg_node_get_state (node), dominate);
102
103             rsvg_push_discrete_layer (ctx);
104         }
105
106         rsvg_node_foreach_child (node, draw_child, ctx);
107

_rsvg_node_draw_children() gets called for the group in question.  It calls rsvg_state_reinherit_top() with the group's state, and the drawing ctx.  At that point, ctx->state is valid.  After the call to rsvg_state_reinherit_top(), ctx->state has a 0-transform.

In the stack trace above, the faulty group node makes its first appearance in frame 7; that's a call to draw_child(node=0x6d4270, ...).  This is the call from when the group's parent is drawing its children, i.e. the group itself and its siblings.

a) The bug would not happen if Node::draw() would refuse to draw a node that is in error.

b) Or, up the call stack, the bug would also not happen if rsvg_node_draw_from_stack() in frame 6 would refuse to render a node if the node is in error.

If (a), then the node would not render, the bug would not happen, but the ancestor call to rsvg_node_draw_from_stack() would create an unnecessary inherited state in its stack.

If (b), we would catch the node that is in error earlier, and avoid the extra inherited state.  Maybe we should have both: (a) for general correctness, and (b) as an optimization?
Comment 2 Federico Mena Quintero 2017-03-03 15:51:23 UTC
Eh, Bugzilla ate my second stack trace and comments.  After the stack trace in the comment above, and before "The 0-transform from the group...", please mentally insert the following:

[talking about the first stack trace where the crash happens]
But that is a secondary effect from the all-zeros transform rippling down the code.

The transform gets created in RsvgNodeGroup's set_atts() method (the all-zeros transform comes in an SVG group).

Later, when the group is about to be drawn, its state gets reinherited into the ctx->state, and the ctx->state ends up with an all-zeros transform.

This is rsvg_state_reinherit_top() being called with the state from the group that has the 0-transform:

(gdb) where
  • #0 rsvg_state_reinherit_top
    at rsvg-styles.c line 1738
  • #1 _rsvg_node_draw_children
    at rsvg-structure.c line 101
  • #2 rsvg_group_draw
    at rsvg-structure.c line 121
  • #3 rsvg_internals::cnode::{{impl}}::draw
    at /home/federico/src/librsvg-latest/rust/src/cnode.rs line 27
  • #4 rsvg_internals::node::{{impl}}::draw
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 134
  • #5 rsvg_internals::node::rsvg_node_draw
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 277
  • #6 rsvg_node_draw_from_stack
    at rsvg-structure.c line 76
  • #7 draw_child
    at rsvg-structure.c line 91
  • #8 rsvg_internals::node::rsvg_node_foreach_child
    at /home/federico/src/librsvg-latest/rust/src/node.rs line 291
  • #9 _rsvg_node_draw_children
    at rsvg-structure.c line 106
  • #10 rsvg_group_draw
    at rsvg-structure.c line 121

Comment 3 Federico Mena Quintero 2017-03-16 14:48:00 UTC
Fixed with a slew of commits, starting with

  356cfdf8dddfd4054c4d8308410ba0c19f848f1a - node.rs: Start defining node::Error type

and ending with

  3b118396dded327e1b7bce43dfe37ed518a19e65 - bgo#776932: Don't render nodes whose "transform" attribute is invalid

This commit series adds infrastructure so that nodes will remember when they are "in error", per the SVG spec.  We will refuse to render nodes that are in error.

In this specific case, we parse a "transform" attribute and notice that it produced an invalid affine transformation matrix.  We remember this error, and at rendering time we don't render the problematic node.