Bug #12670: Segmentation fault on `Magick::Image#get_pixels` on ruby since ruby-2.2

• Status: Closed
• Assignee: ko1 (Koichi Sasada)
• Target version: -
• ruby -v: 2.4
• Backport: 2.1: UNKNOWN, 2.2: UNKNOWN, 2.3: DONE, 2.4: DONE

[ShockwaveNN (Pavel Lobashov)]

1. Create test.rb this code

require 'rmagick'
include Magick

(0..10).each do |_|
  ImageList.new('1.bmp').get_pixels(0, 0, 1600, 800).each_slice(1600).to_a
end

2. Create any 1.bmp file with dimension at least 1600*800 (Include zip file with it, extract it in directory with test.tb)
3. Install ruby 2.1.9 (via RVM for example) and run

  ruby -v # ruby 2.1.9p490 (2016-03-30 revision 54437) [x86_64-linux]
  ruby test.rb # Nothing output, all good

4. Install ruby 2.3.1 (via RVM for example) and run

  ruby 2.3.1p112 (2016-04-26 revision 54768) [x86_64-linux]
  ruby test.rb # You get a big log of Segmentation failed

I issued an problem on rmagick issue tracker (https://github.com/rmagick/rmagick/issues/212) but there is no progress at all
So I thought since both rubies use same version of rmagick I think problem may be in ruby itself.

[wanabe (_ wanabe)]

It may be related to #10440 r48239, as git bisect shown.

[wanabe (_ wanabe)]

Line 5426 of gc.c at v2_3_1 https://github.com/ruby/ruby/blob/v2_3_1/gc.c#L5426 that is pointed by out.log, is about GC but r48239 is not include a change of GC, as I see.
r48239 may not be related...

[wanabe (_ wanabe)]

I got it.
SEGV is raised from gc_marks_continue(), line 5426: slots = heap->free_pages->free_slots,
as pointed by out.log. https://github.com/ruby/ruby/blob/v2_3_1/gc.c#L5426
My gdb shows the value of heap->free_pages is NULL when SEGV.

heap_page_resurrect() can return page even if page->freelist is NULL. It is intended - see r43461.
In the case, heap_add_freepage() does not set heap->free_pages and still keeps it NULL.

[nagachika (Tomoyuki Chikanaga)]

• Status changed from Open to Assigned
• Assignee set to ko1 (Koichi Sasada)

[nagachika (Tomoyuki Chikanaga)]

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

wanabe san, thank you for your great investigation!

Sasada san, could you take a look at the lines wanabe san pointed out?

[ko1 (Koichi Sasada)]

• Status changed from Assigned to Closed

Applied in changeset r56558.

---

• gc.c (heap_page_resurrect): do not return tomb_pages when
  page->freelist == NULL.
  [Bug #12670]

[ko1 (Koichi Sasada)]

• Status changed from Closed to Feedback

I commit a fix on trunk. Could you check it?

[ShockwaveNN (Pavel Lobashov)]

• File core_dump.log core_dump.log added
• ruby -v changed from 2.31 to 2.4

Koichi Sasada wrote:

I commit a fix on trunk. Could you check it?

Seems nothing changed. Errors looks like same

ruby -v
ruby 2.4.0dev (2016-11-04 trunk 56558) [x86_64-linux]
Attached a terminal output file.

[nagachika (Tomoyuki Chikanaga)]

• Status changed from Feedback to Assigned

[wanabe (_ wanabe)]

I think the status is changed, but there is another issue.
We can see a assertion fail when I set RGENGC_CHECK_MODE as 1 in gc.c.

$ bundle exec ruby test.rb
ruby: ../gc.c:1532: heap_page_allocate: Assertion `heap_allocated_pages <= heap_pages_sorted_length' failed.
Aborted (core dumped)

This is in heap_page_allocate().
heap_pages_sorted_length, length of heap_pages_sorted[], must be greater than heap_allocated_pages when ruby call the function.
Otherwise, it causes invalid MEMMOVE() or assignment (heap_pages_sorted[hi] = page;).

[ko1 (Koichi Sasada)]

On my linux VM, I can't reproduce with and without previous patch.

• ruby 2.4.0dev (2016-11-05 trunk 56574) [x86_64-linux]
• ruby 2.4.0dev (2016-11-01 trunk 56534) [x86_64-linux]

Sorry...

[ShockwaveNN (Pavel Lobashov)]

Koichi Sasada wrote:

On my linux VM, I can't reproduce with and without previous patch.

• ruby 2.4.0dev (2016-11-05 trunk 56574) [x86_64-linux]
• ruby 2.4.0dev (2016-11-01 trunk 56534) [x86_64-linux]

Sorry...

I have several PC and this problem reproduces even in TravisCi. But all my PC's and travis images are Ubuntu-based. It may be important. Whis linux VM are you using?

[ko1 (Koichi Sasada)]

I'm also using "Ubuntu 14.04.5 LTS" with gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.3).

[wanabe (_ wanabe)]

• File Dockerfile Dockerfile added

The attached Dockerfile may help to reproduce.

[ko1 (Koichi Sasada)]

Sorry for late response.

Unfortunately I have no enough disk to run docker. Try later when I get new machine.

[wanabe (_ wanabe)]

• File segv.c segv.c added
• File segv.rb segv.rb added

I reduced repro codes, segv.c and segv.rb.
They cause assertion failure with RGENGC_CHECK_MODE=1.

ruby 2.5.0dev (2017-05-21 trunk 58832) [x86_64-linux]
creating Makefile
linking shared-object segv.so
0
1
2
3
4
5
6
7
8
9
10
11
12
13
14
ruby: ../../gc.c:1533: heap_page_allocate: Assertion `heap_allocated_pages <= heap_pages_sorted_length' failed.
Aborted (core dumped)

[nobu (Nobuyoshi Nakada)]

Data_Wrap_Struct(rb_cData, NULL, dummy_free, &i);

Wrapping the pointer to a local variable?
Obviously it is a bug.

If it is the thing what happens in RMagick, it's a bug in object lifetime management of RMagick.

[wanabe (_ wanabe)]

• File segv.c segv.c added

nobu (Nobuyoshi Nakada) wrote:

Data_Wrap_Struct(rb_cData, NULL, dummy_free, &i);

Wrapping the pointer to a local variable?
Obviously it is a bug.

I'm sorry that it is not RMagick but MY bug.
Please let me try it again.

[nobu (Nobuyoshi Nakada)]

It reproduced "incorrect checksum for freed object", with a fix.

-    Data_Wrap_Struct(rb_cData, NULL, dummy_free, destroy_pixel);
+    Data_Wrap_Struct(rb_cData, NULL, destroy_pixel, pixel);

[nobu (Nobuyoshi Nakada)]

• Status changed from Assigned to Closed

Applied in changeset trunk|r59116.

---

test for [Bug #12670]

heap corruption by deferred free.

[nobu (Nobuyoshi Nakada)]

• Status changed from Closed to Assigned

[nobu (Nobuyoshi Nakada)]

• Status changed from Assigned to Closed

Applied in changeset trunk|r59136.

---

gc.c: expand sorted pages

• gc.c (heap_page_allocate): expand sorted pages before inserting
  allocated new page. [Bug #12670]

[usa (Usaku NAKAMURA)]

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

[usa (Usaku NAKAMURA)]

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

ruby_2_3 r59234 merged revision(s) 56558,59116,59136.

[nagachika (Tomoyuki Chikanaga)]

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

ruby_2_4 r59302 merged revision(s) 56558,59116,59136.

[ShockwaveNN (Pavel Lobashov)]

nagachika (Tomoyuki Chikanaga) wrote:

ruby_2_4 r59302 merged revision(s) 56558,59116,59136.

Can confirm bug is fixed.
Checked again with attached Dockerfile
Bug reproduced on:
v2_4_1
v2_3_4
v2_2_8

Bug fixed on:
v2_4_2
v2_3_5

[nobu (Nobuyoshi Nakada)]

• Has duplicate Bug #13900: Segmentation fault - 2 different machines added