Bug #12237: Coverage keeps tracking counts even after Coverage.result

• Status: Closed
• Assignee: mame (Yusuke Endoh)
• Target version: -
• ruby -v: ruby 2.4.0dev (2016-03-31 trunk 54454) [x86_64-linux]
• Backport: 2.1: REQUIRED, 2.2: DONE, 2.3: DONE

[Eregon (Benoit Daloze)]

While looking at #12220, I found a bug.
Attached is a patch that reveals it.

Running
$ make test-all TESTS="test/coverage -n /test_restarting_coverage/"
gives
[1/1] TestCoverage#test_restarting_coverage
Updating cov for /tmp/d20160331-27149-q8o6co/test.rb:1 : 1 size=3 # OK
Updating cov for /tmp/d20160331-27149-q8o6co/test.rb:2 : 1 size=0 # This did RARAAY_AREF(,2) on an array of size 0!
{"/tmp/d20160331-27149-q8o6co/test.rb"=>[]}
Updating cov for /tmp/d20160331-27149-q8o6co/test.rb:2 : 2 size=0 # This did RARAAY_AREF(,4) on an array of size 0!
Updating cov for /tmp/d20160331-27149-q8o6co/test2.rb:1 : 1 size=4 # OK
{"/tmp/d20160331-27149-q8o6co/test.rb"=>[], "/tmp/d20160331-27149-q8o6co/test2.rb"=>[1, nil, 0, nil]}

Coverage tracking of files after Coverage.result (= reset) should just disable itself.
Since the current strategy is to rb_ary_clear() these arrays referenced by the iseq, one should check for bounds instead, as in the commented code.

Ideally, it could even rewrite the trace coverage instruction to a no-op, but that's more involved and might have few real use-cases.
Note that these arrays also forever leak and it would seem rb_ary_clear() does not always free the storage but just set the size to 0.

Can I commit the fix in thread.c?
I think it should be back-ported as well as it could be accessing arrays out-of-bounds.

[Eregon (Benoit Daloze)]

• File 0001-thread.c-update_coverage-Do-not-track-coverage-in-lo.patch 0001-thread.c-update_coverage-Do-not-track-coverage-in-lo.patch added

Attached proposed fixing patch.

[mame (Yusuke Endoh)]

Benoit Daloze wrote:

Can I commit the fix in thread.c?

Sure, thank you very much!

--
Yusuke Endoh [e-mail]

[Eregon (Benoit Daloze)]

• Status changed from Open to Closed

Applied in changeset r54465.

---

• thread.c (update_coverage): Do not track coverage in loaded files
  after Coverage.result. Avoids out-of-bounds access. [Bug #12237]
• ext/coverage/coverage.c (coverage_clear_result_i): document.

[nagachika (Tomoyuki Chikanaga)]

• Backport changed from 2.1: UNKNOWN, 2.2: UNKNOWN, 2.3: UNKNOWN to 2.1: REQUIRED, 2.2: REQUIRED, 2.3: REQUIRED

[naruse (Yui NARUSE)]

• Backport changed from 2.1: REQUIRED, 2.2: REQUIRED, 2.3: REQUIRED to 2.1: REQUIRED, 2.2: REQUIRED, 2.3: DONE

ruby_2_3 r54632 merged revision(s) 54465.

[usa (Usaku NAKAMURA)]

• Backport changed from 2.1: REQUIRED, 2.2: REQUIRED, 2.3: DONE to 2.1: REQUIRED, 2.2: DONE, 2.3: DONE

ruby_2_2 r54680 merged revision(s) 54465.