Skip to content
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

Table extension with HTML Renderer causes segfault #34

Closed
deep-spaced opened this issue Feb 2, 2017 · 8 comments
Closed

Table extension with HTML Renderer causes segfault #34

deep-spaced opened this issue Feb 2, 2017 · 8 comments

Comments

@deep-spaced
Copy link
Contributor

Greetings again!

@kivikakk I'm back with another table issue 😢 . Under Ruby 2.3.1, using an HTML Renderer with the table extension enabled at all causes a segfault. Here's a sample test that I set up that causes it every time: https://github.com/deep-spaced/commonmarker/commit/2d0e5770182ac1b80a52e7b420c20f2878bea1ef

To be specific, the latter test causes the segfault to occur: HtmlRenderer.new.render(doc)

Here's an example stacktrace: https://gist.github.com/deep-spaced/5e5ceebfed14aaae896bc5180ea89e60

Any thoughts on what might be going on here? Thank you for your time looking into this!

@kivikakk
Copy link
Collaborator

kivikakk commented Feb 6, 2017

Looking into this now!

@kivikakk
Copy link
Collaborator

kivikakk commented Feb 6, 2017

The table extension currently uses the user_data section of the node to store table-related information, which CommonMarker also uses in rb_node_to_value and related functions. I'll make these play a little nicer with each other!

@kivikakk
Copy link
Collaborator

kivikakk commented Feb 6, 2017

The "quickest" fix is to remove all the node caching in userdata, though it probably has some big memory/speed implications for using the node iterators on larger documents. @gjtorikian; do you have any thoughts on this?

Ideally we'd have separate per-node user-data addressable by a constant or something, but that's a big thing to want to add to cmark.

Edit: or perhaps we want a separate 'extension data' field on nodes. Hmmm.

@kivikakk
Copy link
Collaborator

kivikakk commented Feb 6, 2017

There's a look at what this would look like at https://github.com/gjtorikian/commonmarker/compare/table_render_crash (based on @deep-spaced's test branch).

@kivikakk
Copy link
Collaborator

kivikakk commented Feb 6, 2017

Dropping the cache has the result that GC ends up prematurely cleaning things up, so it's not good enough.

@kivikakk
Copy link
Collaborator

kivikakk commented Feb 6, 2017

The separate "extension data" field pretty much already exists, because the as union is unused for extension nodes. Easy solution. :)

@kivikakk
Copy link
Collaborator

kivikakk commented Feb 6, 2017

💎 0.14.3 is pushed, which fixes this issue! Thank you for reporting it, this has helped me clean up our extensions to cmark :)

@kivikakk kivikakk closed this as completed Feb 6, 2017
@deep-spaced
Copy link
Contributor Author

@kivikakk Thank you so much for jumping on this and fixing it!!! 👍 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants