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.

Sign up for GitHub

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

Jump to bottom

[bug] valgrind errors spotted in gumbo parser #2276

Closed
flavorjones opened this issue Jun 19, 2021 · 11 comments
Closed

[bug] valgrind errors spotted in gumbo parser #2276

flavorjones opened this issue Jun 19, 2021 · 11 comments
Labels
topic/gumbo Gumbo HTML5 parser topic/memory Segfaults, memory leaks, valgrind testing, etc.
Milestone
v1.12.0

Comments

@flavorjones
Copy link
Member

Please describe the bug

A CI test run caught some memory errors in gumbo.

The build log is at https://github.com/sparklemotion/nokogiri/pull/2272/checks?check_run_id=2862967128

Here's the relevant snippet:

# Running tests with run options --seed 25442:

==489== Invalid read of size 8
==489==    at 0x8A170F4: parse_args_mark (gumbo.c:307)
==489==    by 0x490A58B: gc_mark_stacked_objects (gc.c:4737)
==489==    by 0x490A58B: gc_mark_stacked_objects_all (gc.c:4777)
==489==    by 0x490A58B: gc_marks_rest (gc.c:5653)
==489==    by 0x490AE9F: gc_marks (gc.c:5713)
==489==    by 0x490AE9F: gc_start (gc.c:6493)
==489==    by 0x490AE9F: gc_start (gc.c:6417)
==489==    by 0x490B5E2: garbage_collect (gc.c:6413)
==489==    by 0x490B5E2: gc_start_internal (gc.c:6722)
==489==    by 0x4A42198: vm_call_cfunc_with_frame (vm_insnhelper.c:1918)
==489==    by 0x4A42198: vm_call_cfunc (vm_insnhelper.c:1934)
==489==    by 0x4A4CEED: vm_exec_core (insns.def:915)
==489==    by 0x4A51FDC: vm_exec (vm.c:1778)
==489==    by 0x4A58F70: invoke_block (vm.c:979)
==489==    by 0x4A58F70: invoke_iseq_block_from_c (vm.c:1031)
==489==    by 0x4A58F70: invoke_block_from_c_bh (vm.c:1049)
==489==    by 0x4A58F70: vm_yield (vm.c:1094)
==489==    by 0x4A58F70: rb_yield_0 (vm_eval.c:970)
==489==    by 0x4A58F70: rb_yield_1 (vm_eval.c:976)
==489==    by 0x4A58F70: rb_yield (vm_eval.c:986)
==489==    by 0x487477B: rb_ary_each (array.c:1837)
==489==    by 0x4A42198: vm_call_cfunc_with_frame (vm_insnhelper.c:1918)
==489==    by 0x4A42198: vm_call_cfunc (vm_insnhelper.c:1934)
==489==    by 0x4A4D962: vm_exec_core (insns.def:850)
==489==    by 0x4A51FDC: vm_exec (vm.c:1778)
==489==    by 0x4A58F70: invoke_block (vm.c:979)
==489==    by 0x4A58F70: invoke_iseq_block_from_c (vm.c:1031)
==489==    by 0x4A58F70: invoke_block_from_c_bh (vm.c:1049)
==489==    by 0x4A58F70: vm_yield (vm.c:1094)
==489==    by 0x4A58F70: rb_yield_0 (vm_eval.c:970)
==489==    by 0x4A58F70: rb_yield_1 (vm_eval.c:976)
==489==    by 0x4A58F70: rb_yield (vm_eval.c:986)
==489==    by 0x487477B: rb_ary_each (array.c:1837)
==489==    by 0x4A42198: vm_call_cfunc_with_frame (vm_insnhelper.c:1918)
==489==    by 0x4A42198: vm_call_cfunc (vm_insnhelper.c:1934)
==489==    by 0x4A4D962: vm_exec_core (insns.def:850)
==489==    by 0x4A51FDC: vm_exec (vm.c:1778)
==489==    by 0x4A584C4: invoke_block (vm.c:979)
==489==    by 0x4A584C4: invoke_iseq_block_from_c (vm.c:1031)
==489==    by 0x4A584C4: invoke_block_from_c_bh (vm.c:1049)
==489==    by 0x4A584C4: vm_yield_force_blockarg (vm.c:1110)
==489==    by 0x4A584C4: rb_yield_force_blockarg (vm_eval.c:1035)
==489==    by 0x4878FAB: rb_ary_collect (array.c:2758)
==489==    by 0x4A42198: vm_call_cfunc_with_frame (vm_insnhelper.c:1918)
==489==    by 0x4A42198: vm_call_cfunc (vm_insnhelper.c:1934)
==489==    by 0x4A53E8B: vm_call_method_each_type.part.135 (vm_insnhelper.c:2232)
==489==    by 0x4A5450A: vm_call_method_each_type (vm_insnhelper.c:2380)
==489==    by 0x4A5450A: vm_call_method (vm_insnhelper.c:2384)
==489==    by 0x4A5450A: vm_call_method (vm_insnhelper.c:2351)
==489==    by 0x4A4D962: vm_exec_core (insns.def:850)
==489==    by 0x4A51FDC: vm_exec (vm.c:1778)
==489==    by 0x4A52BAA: invoke_block (vm.c:979)
==489==    by 0x4A52BAA: invoke_iseq_block_from_c (vm.c:1031)
==489==    by 0x4A53889: invoke_block_from_c_proc (vm.c:1124)
==489==    by 0x4A53889: vm_invoke_proc (vm.c:1149)
==489==    by 0x4984B18: rb_proc_call (proc.c:887)
==489==    by 0x48F401C: exec_end_procs_chain (eval_jump.c:108)
==489==    by 0x48F401C: rb_exec_end_proc (eval_jump.c:125)
==489==    by 0x48F4171: ruby_finalize_0 (eval.c:124)
==489==    by 0x48F4497: ruby_cleanup (eval.c:182)
==489==    by 0x48F47C4: ruby_run_node (eval.c:303)
==489==    by 0x1090EA: main (main.c:42)
==489==  Address 0x1ffeffcce8 is on thread 1's stack
==489==  312 bytes below stack pointer
==489== 
{
   <insert_a_suppression_name_here>
   Memcheck:Addr8
   fun:parse_args_mark
   fun:gc_mark_stacked_objects
   fun:gc_mark_stacked_objects_all
   fun:gc_marks_rest
   fun:gc_marks
   fun:gc_start
   fun:gc_start
   fun:garbage_collect
   fun:gc_start_internal
   fun:vm_call_cfunc_with_frame
   fun:vm_call_cfunc
   fun:vm_exec_core
   fun:vm_exec
   fun:invoke_block
   fun:invoke_iseq_block_from_c
   fun:invoke_block_from_c_bh
   fun:vm_yield
   fun:rb_yield_0
   fun:rb_yield_1
   fun:rb_yield
   fun:rb_ary_each
   fun:vm_call_cfunc_with_frame
   fun:vm_call_cfunc
   fun:vm_exec_core
   fun:vm_exec
   fun:invoke_block
   fun:invoke_iseq_block_from_c
   fun:invoke_block_from_c_bh
   fun:vm_yield
   fun:rb_yield_0
   fun:rb_yield_1
   fun:rb_yield
   fun:rb_ary_each
   fun:vm_call_cfunc_with_frame
   fun:vm_call_cfunc
   fun:vm_exec_core
   fun:vm_exec
   fun:invoke_block
   fun:invoke_iseq_block_from_c
   fun:invoke_block_from_c_bh
   fun:vm_yield_force_blockarg
   fun:rb_yield_force_blockarg
   fun:rb_ary_collect
   fun:vm_call_cfunc_with_frame
   fun:vm_call_cfunc
   fun:vm_call_method_each_type.part.135
   fun:vm_call_method_each_type
   fun:vm_call_method
   fun:vm_call_method
   fun:vm_exec_core
   fun:vm_exec
   fun:invoke_block
   fun:invoke_iseq_block_from_c
   fun:invoke_block_from_c_proc
   fun:vm_invoke_proc
   fun:rb_proc_call
   fun:exec_end_procs_chain
   fun:rb_exec_end_proc
   fun:ruby_finalize_0
   fun:ruby_cleanup
   fun:ruby_run_node
   fun:main
}
==489== Invalid read of size 8
==489==    at 0x8A170FD: parse_args_mark (gumbo.c:308)
==489==    by 0x490A58B: gc_mark_stacked_objects (gc.c:4737)
==489==    by 0x490A58B: gc_mark_stacked_objects_all (gc.c:4777)
==489==    by 0x490A58B: gc_marks_rest (gc.c:5653)
==489==    by 0x490AE9F: gc_marks (gc.c:5713)
==489==    by 0x490AE9F: gc_start (gc.c:6493)
==489==    by 0x490AE9F: gc_start (gc.c:6417)
==489==    by 0x490B5E2: garbage_collect (gc.c:6413)
==489==    by 0x490B5E2: gc_start_internal (gc.c:6722)
==489==    by 0x4A42198: vm_call_cfunc_with_frame (vm_insnhelper.c:1918)
==489==    by 0x4A42198: vm_call_cfunc (vm_insnhelper.c:1934)
==489==    by 0x4A4CEED: vm_exec_core (insns.def:915)
==489==    by 0x4A51FDC: vm_exec (vm.c:1778)
==489==    by 0x4A58F70: invoke_block (vm.c:979)
==489==    by 0x4A58F70: invoke_iseq_block_from_c (vm.c:1031)
==489==    by 0x4A58F70: invoke_block_from_c_bh (vm.c:1049)
==489==    by 0x4A58F70: vm_yield (vm.c:1094)
==489==    by 0x4A58F70: rb_yield_0 (vm_eval.c:970)
==489==    by 0x4A58F70: rb_yield_1 (vm_eval.c:976)
==489==    by 0x4A58F70: rb_yield (vm_eval.c:986)
==489==    by 0x487477B: rb_ary_each (array.c:1837)
==489==    by 0x4A42198: vm_call_cfunc_with_frame (vm_insnhelper.c:1918)
==489==    by 0x4A42198: vm_call_cfunc (vm_insnhelper.c:1934)
==489==    by 0x4A4D962: vm_exec_core (insns.def:850)
==489==    by 0x4A51FDC: vm_exec (vm.c:1778)
==489==    by 0x4A58F70: invoke_block (vm.c:979)
==489==    by 0x4A58F70: invoke_iseq_block_from_c (vm.c:1031)
==489==    by 0x4A58F70: invoke_block_from_c_bh (vm.c:1049)
==489==    by 0x4A58F70: vm_yield (vm.c:1094)
==489==    by 0x4A58F70: rb_yield_0 (vm_eval.c:970)
==489==    by 0x4A58F70: rb_yield_1 (vm_eval.c:976)
==489==    by 0x4A58F70: rb_yield (vm_eval.c:986)
==489==    by 0x487477B: rb_ary_each (array.c:1837)
==489==    by 0x4A42198: vm_call_cfunc_with_frame (vm_insnhelper.c:1918)
==489==    by 0x4A42198: vm_call_cfunc (vm_insnhelper.c:1934)
==489==    by 0x4A4D962: vm_exec_core (insns.def:850)
==489==    by 0x4A51FDC: vm_exec (vm.c:1778)
==489==    by 0x4A584C4: invoke_block (vm.c:979)
==489==    by 0x4A584C4: invoke_iseq_block_from_c (vm.c:1031)
==489==    by 0x4A584C4: invoke_block_from_c_bh (vm.c:1049)
==489==    by 0x4A584C4: vm_yield_force_blockarg (vm.c:1110)
==489==    by 0x4A584C4: rb_yield_force_blockarg (vm_eval.c:1035)
==489==    by 0x4878FAB: rb_ary_collect (array.c:2758)
==489==    by 0x4A42198: vm_call_cfunc_with_frame (vm_insnhelper.c:1918)
==489==    by 0x4A42198: vm_call_cfunc (vm_insnhelper.c:1934)
==489==    by 0x4A53E8B: vm_call_method_each_type.part.135 (vm_insnhelper.c:2232)
==489==    by 0x4A5450A: vm_call_method_each_type (vm_insnhelper.c:2380)
==489==    by 0x4A5450A: vm_call_method (vm_insnhelper.c:2384)
==489==    by 0x4A5450A: vm_call_method (vm_insnhelper.c:2351)
==489==    by 0x4A4D962: vm_exec_core (insns.def:850)
==489==    by 0x4A51FDC: vm_exec (vm.c:1778)
==489==    by 0x4A52BAA: invoke_block (vm.c:979)
==489==    by 0x4A52BAA: invoke_iseq_block_from_c (vm.c:1031)
==489==    by 0x4A53889: invoke_block_from_c_proc (vm.c:1124)
==489==    by 0x4A53889: vm_invoke_proc (vm.c:1149)
==489==    by 0x4984B18: rb_proc_call (proc.c:887)
==489==    by 0x48F401C: exec_end_procs_chain (eval_jump.c:108)
==489==    by 0x48F401C: rb_exec_end_proc (eval_jump.c:125)
==489==    by 0x48F4171: ruby_finalize_0 (eval.c:124)
==489==    by 0x48F4497: ruby_cleanup (eval.c:182)
==489==    by 0x48F47C4: ruby_run_node (eval.c:303)
==489==    by 0x1090EA: main (main.c:42)
==489==  Address 0x1ffeffccf0 is on thread 1's stack
==489==  304 bytes below stack pointer
==489== 
{
   <insert_a_suppression_name_here>
   Memcheck:Addr8
   fun:parse_args_mark
   fun:gc_mark_stacked_objects
   fun:gc_mark_stacked_objects_all
   fun:gc_marks_rest
   fun:gc_marks
   fun:gc_start
   fun:gc_start
   fun:garbage_collect
   fun:gc_start_internal
   fun:vm_call_cfunc_with_frame
   fun:vm_call_cfunc
   fun:vm_exec_core
   fun:vm_exec
   fun:invoke_block
   fun:invoke_iseq_block_from_c
   fun:invoke_block_from_c_bh
   fun:vm_yield
   fun:rb_yield_0
   fun:rb_yield_1
   fun:rb_yield
   fun:rb_ary_each
   fun:vm_call_cfunc_with_frame
   fun:vm_call_cfunc
   fun:vm_exec_core
   fun:vm_exec
   fun:invoke_block
   fun:invoke_iseq_block_from_c
   fun:invoke_block_from_c_bh
   fun:vm_yield
   fun:rb_yield_0
   fun:rb_yield_1
   fun:rb_yield
   fun:rb_ary_each
   fun:vm_call_cfunc_with_frame
   fun:vm_call_cfunc
   fun:vm_exec_core
   fun:vm_exec
   fun:invoke_block
   fun:invoke_iseq_block_from_c
   fun:invoke_block_from_c_bh
   fun:vm_yield_force_blockarg
   fun:rb_yield_force_blockarg
   fun:rb_ary_collect
   fun:vm_call_cfunc_with_frame
   fun:vm_call_cfunc
   fun:vm_call_method_each_type.part.135
   fun:vm_call_method_each_type
   fun:vm_call_method
   fun:vm_call_method
   fun:vm_exec_core
   fun:vm_exec
   fun:invoke_block
   fun:invoke_iseq_block_from_c
   fun:invoke_block_from_c_proc
   fun:vm_invoke_proc
   fun:rb_proc_call
   fun:exec_end_procs_chain
   fun:rb_exec_end_proc
   fun:ruby_finalize_0
   fun:ruby_cleanup
   fun:ruby_run_node
   fun:main
}
@flavorjones flavorjones added state/needs-triage Inbox for non-installation-related bug reports or help requests topic/memory Segfaults, memory leaks, valgrind testing, etc. topic/gumbo Gumbo HTML5 parser and removed state/needs-triage Inbox for non-installation-related bug reports or help requests topic/memory Segfaults, memory leaks, valgrind testing, etc. labels Jun 19, 2021
@flavorjones flavorjones added this to the v1.12.0 milestone Jun 19, 2021
@flavorjones
Copy link
Member Author

One more instance: https://github.com/sparklemotion/nokogiri/pull/2273/checks?check_run_id=2863381608#step:7:40

@flavorjones
Copy link
Member Author

I spent some time looking at this today. I think I understand the warning, but want to verify with someone who understands GC better than me, maybe @tenderlove or @rubys.

Let's focus on this section of gumbo.c's function parse:

nokogiri/ext/nokogiri/gumbo.c

Lines 357 to 365 in 2bb0084

ParseArgs args = {
.output = output,
.input = input,
.url_or_frag = url,
.doc = NULL,
};
VALUE parse_args = wrap_parse_args(&args);
return rb_ensure(parse_continue, parse_args, parse_cleanup, parse_args);

This code creates a Ruby object that wraps a struct located on the stack, which is used to pass arguments into parse_continue. The valgrind error comes from this object getting marked after rb_ensure and the C function parse have both returned (so the address is "304 bytes below stack pointer").

I'm not sure I understand why mark is getting called on the object after the function has returned. Maybe someone can explain how this works?

If that's what's happening, wrapping a stack-located struct seems like an unsafe idiom and we should convert to using a struct that's allocated on the heap. Is there a preferable idiom that we could use to avoid an additional memory allocation?

Side note: this is extremely hard to reproduce.

@flavorjones
Copy link
Member Author

🤔 Can we just pass a pointer to this struct (cast as VALUE) and not have to wrap a Ruby object around it, since we don't care about the object lifecycle in this case (and input and url are both objects that don't need to be marked since they're pass in as arguments)?

flavorjones added a commit that referenced this issue Jul 11, 2021
@flavorjones
fix(gumbo): use C struct pointer directly instead of wrapping
b8f4abd 
This should avoid the stack-address valgrind warning from #2276
@flavorjones flavorjones mentioned this issue Jul 11, 2021
Merged
@flavorjones
Copy link
Member Author

I submitted a PR at #2290 to use the struct pointer directly.

@stevecheckoway
Copy link
Contributor

stevecheckoway commented Jul 12, 2021 •
edited

I wrote a response to this but GitHub appears to have lost it.

I don't think that the approach in #2290 works. If the Ruby runtime is calling parse_args_mark, then it is definitely treating the VALUE argument as an actual VALUE. Thus passing something that isn't a VALUE is likely to cause different problems.

I can't think of a reason why the runtime would be holding on to it after it has returned from rb_ensure().

Maybe my code is misusing RUBY_NEVER_FREE. My intent was to prevent Ruby from trying to free the underlying data, but maybe it causes Ruby to hold on to the wrapper forever?

@stevecheckoway
Copy link
Contributor

I suspect the best thing to do is to use TypedData_Make_Struct. It sounds like the Data_xxx macros are deprecated and TypedData_xxx is the way to go. https://docs.ruby-lang.org/en/2.7.0/doc/extension_rdoc.html#label-C+struct+to+Ruby+object suggests that the object should derive from rb_cData. I thought I tried that and it didn't work, but I don't recall the details.

@stevecheckoway stevecheckoway mentioned this issue Jul 12, 2021
Closed
@stevecheckoway
Copy link
Contributor

With any luck, #2291 will solve the issue.

@larskanis
Copy link
Member

larskanis commented Jul 12, 2021 •
edited

I don't think that the approach in #2290 works. If the Ruby runtime is calling parse_args_mark, then it is definitely treating the VALUE argument as an actual VALUE. Thus passing something that isn't a VALUE is likely to cause different problems.

The second and fourth argument to rb_ensure are just opaque pointers, with no usage as a real VALUE. There are several such cases in the Ruby C-API (rb_thread_call_with_gvl etc.) where the meaning is void* but for some convenience, it is of type VALUE. The Ruby runtime calls parse_args_mark only because the VALUE was registered by Data_Wrap_Struct.

So I think #2290 is correct. The only issue is about input and url_or_frag members not being registered as in use by some mark function. However this marking is not necessary, since both VALUEs are already part of the arguments of parse() and fragment(), so they are protected for being GC'ed.

Also #2291 is correct, but it is overkill IMHO. There's no need to use heap memory in this case.

If that's what's happening, wrapping a stack-located struct seems like an unsafe idiom and we should convert to using a struct that's allocated on the heap. Is there a preferable idiom that we could use to avoid an additional memory allocation?

It's still an option to use a registered VALUE object wrapping a pointer on the stack, as it's done now. Unfortunately there's one issue with the current implementation: the wrapped pointer must be decoupled from the ruby object at the end of parse() and fragment() functions, since it's on the stack and the stack becomes invalid. This can be done by

    DATA_PTR(parse_args) = NULL;

and checking for NULL of parse_args in parse_args_mark().

I suspect the best thing to do is to use TypedData_Make_Struct. It sounds like the Data_xxx macros are deprecated and TypedData_xxx is the way to go.

Yes they are deprecated, but there's nothing in the new API that helps for this particular issue. Use of the new API is an open issue: #2107 . With the new API recoupling can be done per:

    RTYPEDDATA_DATA(parse_args) = NULL;

@stevecheckoway
Copy link
Contributor

I don't think that the approach in #2290 works. If the Ruby runtime is calling parse_args_mark, then it is definitely treating the VALUE argument as an actual VALUE. Thus passing something that isn't a VALUE is likely to cause different problems.

The second and fourth argument to rb_ensure are just opaque pointers, with no usage as a real VALUE. There are several such cases in the Ruby C-API (rb_thread_call_with_gvl etc.) where the meaning is void* but for some convenience, it is of type VALUE. The Ruby runtime calls parse_args_mark only because the VALUE was registered by Data_Wrap_Struct.

Makes sense.

So I think #2290 is correct. The only issue is about input and url_or_frag members not being registered as in use by some mark function. However this marking is not necessary, since both VALUEs are already part of the arguments of parse() and fragment(), so they are protected for being GC'ed.

Great. That's much simpler since it avoids having to create a new Ruby object (the heap allocation isn't a big deal either way given how many other heap allocations are going to be happening here).

@flavorjones
Copy link
Member Author

Thanks @larskanis and @stevecheckoway for the great conversation here. I'm glad we were able to discuss asynchronously, because I now understand this a bit more deeply (particularly Lars's explanation). Will merge #2290.

@flavorjones
Copy link
Member Author

Closed by #2290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic/gumbo Gumbo HTML5 parser topic/memory Segfaults, memory leaks, valgrind testing, etc.
Projects
None yet
Milestone
v1.12.0
Development

Successfully merging a pull request may close this issue.

Hopefully fix Ruby's GC marking stack objects after return
3 participants
@flavorjones @larskanis @stevecheckoway