https://bugs.ruby-lang.org/https://bugs.ruby-lang.org/favicon.ico?17113305112017-04-15T02:10:51ZRuby Issue Tracking SystemRuby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=642362017-04-15T02:10:51Zko1 (Koichi Sasada)
<ul><li><strong>Assignee</strong> set to <i>ko1 (Koichi Sasada)</i></li></ul><p>Thank you for your survey. Great help.</p>
<p>Just for confirmation.<br>
Your patch doesn't solve memory problem, but for configuration, right?</p>
<p>Thanks,<br>
Koichi</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=642372017-04-15T05:41:13Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>ko1 (Koichi Sasada) wrote:</p>
<blockquote>
<p>Just for confirmation.<br>
Your patch doesn't solve memory problem, but for configuration, right?</p>
</blockquote>
<p>This is not just a configuration issue, this patch actually solves the heap overflow issue by making sure the correct <code>#define</code> is in <code>config.h</code>. Without this patch, on OpenBSD (and maybe other operating systems) HEAP_PAGE_ALIGN_LOG is set to 14 instead of 12, making HEAP_PAGE_SIZE and HEAP_PAGE_BITMAP_SIZE larger. I'm not actually sure where the overflow occurs, and I think the overflow may only be a single byte (<code>0x3fd8@0x3fd8</code> I believe means that malloc allocated 16344 bytes and there was a write at offset 16344).</p>
<p>Thanks,<br>
Jeremy</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=642512017-04-16T03:43:24Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>jeremyevans0 (Jeremy Evans) wrote:</p>
<blockquote>
<p>ko1 (Koichi Sasada) wrote:</p>
<blockquote>
<p>Just for confirmation.<br>
Your patch doesn't solve memory problem, but for configuration, right?</p>
</blockquote>
<p>This is not just a configuration issue, this patch actually solves the heap overflow issue by making sure the correct <code>#define</code> is in <code>config.h</code>. Without this patch, on OpenBSD (and maybe other operating systems) HEAP_PAGE_ALIGN_LOG is set to 14 instead of 12, making HEAP_PAGE_SIZE and HEAP_PAGE_BITMAP_SIZE larger. I'm not actually sure where the overflow occurs, and I think the overflow may only be a single byte (<code>0x3fd8@0x3fd8</code> I believe means that malloc allocated 16344 bytes and there was a write at offset 16344).</p>
</blockquote>
<p>Koichi,</p>
<p>After giving this some more thought, it's possible this doesn't fix the underlying memory issue. There are two possibilities:</p>
<ol>
<li>
<p>The heap overflow only happens when the operating system uses <16kb pages and ruby is set to use 16k heap pages.</p>
</li>
<li>
<p>The heap overflow always happens when ruby uses 16kb heap pages.</p>
</li>
</ol>
<p>If 1) is true, then this should be the only fix necessary. If 2) is true, then there is a separate memory issue that should be fixed. I suspect the problem is more likely 2), but this is outside my area of expertise.</p>
<p>In either case, I think this patch should be applied.</p>
<p>Thanks,<br>
Jeremy</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=642972017-04-17T20:36:00Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul><li><strong>File</strong> <a href="/attachments/6494">0001-Fix-heap-overflow-by-allocating-more-memory-per-heap.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/6494/0001-Fix-heap-overflow-by-allocating-more-memory-per-heap.patch">0001-Fix-heap-overflow-by-allocating-more-memory-per-heap.patch</a> added</li></ul><p>jeremyevans0 (Jeremy Evans) wrote:</p>
<blockquote>
<ol>
<li>
<p>The heap overflow only happens when the operating system uses <16kb pages and ruby is set to use 16k heap pages.</p>
</li>
<li>
<p>The heap overflow always happens when ruby uses 16kb heap pages.</p>
</li>
</ol>
<p>If 1) is true, then this should be the only fix necessary. If 2) is true, then there is a separate memory issue that should be fixed. I suspect the problem is more likely 2), but this is outside my area of expertise.</p>
</blockquote>
<p>I did some testing with different versions of HEAP_PAGE_ALIGN_LOG. Here's the results of my testing, with the first entry being HEAP_PAGE_ALIGN_LOG, the second being the page size, and the third being the result:</p>
<pre><code>6 64B Couldn't build: SIGFPE
7 128B No error
8 256B No error
9 512B No error
10 1KB No error
11 2KB No error
12 4KB No error
13 8KB chunk canary corrupted 0x1fd8@0x1fd8
14 16KB chunk canary corrupted 0x3fd8@0x3fd8
15 32KB chunk canary corrupted 0x7fd8@0x7fd8
16 64KB chunk canary corrupted 0xffd8@0xffd8
17 128KB chunk canary corrupted 0x1ffd8@0x1ffd8
18 256KB chunk canary corrupted 0x3ffd8@0x3ffd8
19 512KB Couldn't build: Failed to allocate memory
</code></pre>
<p>I first thought that when using >4KB pages, there is a heap overflow, but the heap overflow doesn't happen when using <=4KB pages. However, I think there may always be a heap overflow, even when using <=4KB pages. It turns out the OpenBSD malloc canary support is only turned on when allocating >=4KB. This leads me to believe the issue is that there is always a heap overflow, no matter the HEAP_PAGE_ALIGN_LOG value.</p>
<p>I tried increasing the size passed to <code>aligned_malloc</code> to see if I could determine the size of the overflow. It turns out that it overflows not by a single byte, but by 40 bytes. Coincidentally, that is also the value of REQUIRED_SIZE_BY_MALLOC. Maybe REQUIRED_SIZE_BY_MALLOC just needs to be added when calling <code>aligned_malloc</code>? I tried that and it appears to fix things.</p>
<p>The attached patch should fix the heap overflow for all page sizes that compile (tested on HEAP_PAGE_ALIGN 7..18).</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=643002017-04-18T00:33:53Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>jeremyevans0 (Jeremy Evans) wrote:</p>
<blockquote>
<p>I tried increasing the size passed to <code>aligned_malloc</code> to see if I could determine the size of the overflow. It turns out that it overflows not by a single byte, but by 40 bytes. Coincidentally, that is also the value of REQUIRED_SIZE_BY_MALLOC. Maybe REQUIRED_SIZE_BY_MALLOC just needs to be added when calling <code>aligned_malloc</code>? I tried that and it appears to fix things.</p>
<p>The attached patch should fix the heap overflow for all page sizes that compile (tested on HEAP_PAGE_ALIGN 7..18).</p>
</blockquote>
<p>After some more analysis and research, I don't think this patch to gc.c is necessary. I think this is a problem on OpenBSD when calling posix_memalign with allocations over 4KB that are slightly less than the aligned size when using malloc canaries, and that there isn't actually a heap overflow. So the patch to gc.c can be ignored, but the patch to configure.in should still be applied so that HEAP_PAGE_ALIGN_LOG is set correctly.</p>
<p>Thanks,<br>
Jeremy</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=643022017-04-18T00:42:28Zko1 (Koichi Sasada)
<ul></ul><blockquote>
<p>I think this is a problem on OpenBSD when calling posix_memalign with allocations over 4KB that are slightly less than the aligned size when using malloc canaries</p>
</blockquote>
<p>OMG. Thank you for your analysis.<br>
So you mean we should reduce <code>HEAP_PAGE_ALIGN_LOG</code> value on OpenBSD?</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=643042017-04-18T02:09:09Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>ko1 (Koichi Sasada) wrote:</p>
<blockquote>
<blockquote>
<p>I think this is a problem on OpenBSD when calling posix_memalign with allocations over 4KB that are slightly less than the aligned size when using malloc canaries</p>
</blockquote>
<p>OMG. Thank you for your analysis.<br>
So you mean we should reduce <code>HEAP_PAGE_ALIGN_LOG</code> value on OpenBSD?</p>
</blockquote>
<p>I don't think we need to make any changes to gc.c if configure.in is fixed so that HEAP_PAGE_ALIGN_LOG is set correctly.</p>
<p>Is there a reason that heap page allocation sizes are reduced by REQUIRED_SIZE_BY_MALLOC (40 bytes)? It seems like it would be better if HEAP_PAGE_ALIGN and HEAP_PAGE_SIZE were the same. The only reason I can think of is to work around a performance issue in a malloc implementation that stores metadata in the 40 bytes after the allocation. On OpenBSD, having them the same size can be better for security, because if you enable guard pages any overflow for heap pages would result in a segmentation fault.</p>
<p>Thanks,<br>
Jeremy</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=643612017-04-18T16:23:38Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>jeremyevans0 (Jeremy Evans) wrote:</p>
<blockquote>
<p>After some more analysis and research, I don't think this patch to gc.c is necessary. I think this is a problem on OpenBSD when calling posix_memalign with allocations over 4KB that are slightly less than the aligned size when using malloc canaries, and that there isn't actually a heap overflow. So the patch to gc.c can be ignored, but the patch to configure.in should still be applied so that HEAP_PAGE_ALIGN_LOG is set correctly.</p>
</blockquote>
<p>I've confirmed this was a problem with OpenBSD's posix_memalign when using malloc canaries. It has been fixed in OpenBSD-current. So this can be closed once the configure.in patch is committed. I think it should be backported to 2.4, to ensure that ruby GC respects the operating system page size (as it did in ruby 2.3 and below).</p>
<p>Thanks,<br>
Jeremy</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=643722017-04-18T23:21:50Zko1 (Koichi Sasada)
<ul></ul><p>On 2017/04/19 1:23, <a href="mailto:merch-redmine@jeremyevans.net" class="email">merch-redmine@jeremyevans.net</a> wrote:</p>
<blockquote>
<p>I think it should be backported to 2.4, to ensure that ruby GC respects the operating system page size (as it did in ruby 2.3 and below).</p>
</blockquote>
<p>Note that gc's PAGE is not relative to OS/machie page.</p>
<p>--<br>
// SASADA Koichi at atdot dot net</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=643732017-04-18T23:50:55Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul><li><strong>File</strong> <a href="/attachments/6496">0001-Remove-overriding-of-HEAP_PAGE_ALIGN_LOG.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/6496/0001-Remove-overriding-of-HEAP_PAGE_ALIGN_LOG.patch">0001-Remove-overriding-of-HEAP_PAGE_ALIGN_LOG.patch</a> added</li><li><strong>Backport</strong> changed from <i>2.2: UNKNOWN, 2.3: UNKNOWN, 2.4: REQUIRED</i> to <i>2.2: DONTNEED, 2.3: DONTNEED, 2.4: DONTNEED</i></li></ul><p>ko1 (Koichi Sasada) wrote:</p>
<blockquote>
<p>On 2017/04/19 1:23, <a href="mailto:merch-redmine@jeremyevans.net" class="email">merch-redmine@jeremyevans.net</a> wrote:</p>
<blockquote>
<p>I think it should be backported to 2.4, to ensure that ruby GC respects the operating system page size (as it did in ruby 2.3 and below).</p>
</blockquote>
<p>Note that gc's PAGE is not relative to OS/machie page.</p>
</blockquote>
<p>Ah. I see. Reading more of configure.in, HEAP_ALIGN_LOG is only set to 12 or 13 support OpenBSD <5.2 and MirOS. At this point, we may want to drop OpenBSD <5.2 support, or maybe the entire branch unless there are MirOS #10semel users. The MirOS #10semel release was over 9 years ago, and the recommended development snapshots work with the standard 16kb pages according to the comment in configure.in. Attached is a patch that remove the HEAP_ALIGN_LOG setting in configure.in and HEAP_PAGE_ALIGN_LOG overriding in gc.c.</p>
<p>Thanks,<br>
Jeremy</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=645572017-04-28T13:45:42Zshyouhei (Shyouhei Urabe)shyouhei@ruby-lang.org
<ul><li><strong>Status</strong> changed from <i>Open</i> to <i>Assigned</i></li></ul> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=648942017-05-19T02:42:37Zko1 (Koichi Sasada)
<ul><li><strong>Assignee</strong> changed from <i>ko1 (Koichi Sasada)</i> to <i>jeremyevans0 (Jeremy Evans)</i></li></ul><p>Sorry for late response.</p>
<blockquote>
<p>we may want to drop OpenBSD <5.2 support</p>
</blockquote>
<p>On <a href="https://bugs.ruby-lang.org/projects/ruby-trunk/wiki/SupportedPlatforms" class="external">https://bugs.ruby-lang.org/projects/ruby-trunk/wiki/SupportedPlatforms</a></p>
<p>You can decide it. I don't have any idea we should or we shouldn't. Could you decide and commit it?</p>
<p>Thanks,<br>
Koichi</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=650522017-05-23T16:57:57Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>ko1 (Koichi Sasada) wrote:</p>
<blockquote>
<blockquote>
<p>we may want to drop OpenBSD <5.2 support</p>
</blockquote>
<p>On <a href="https://bugs.ruby-lang.org/projects/ruby-trunk/wiki/SupportedPlatforms" class="external">https://bugs.ruby-lang.org/projects/ruby-trunk/wiki/SupportedPlatforms</a></p>
<p>You can decide it. I don't have any idea we should or we shouldn't. Could you decide and commit it?</p>
</blockquote>
<p>I think we should drop OpenBSD <5.2 support. I doubt anyone running such an old version of OpenBSD would want to run ruby 2.5+. The OpenBSD project only supports OpenBSD 6.1 and 6.0 currently.</p>
<p>I don't currently have commit rights, so I can't currently commit the patch myself. Hopefully another developer can commit it.</p>
<p>Thanks,<br>
Jeremy</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=668942017-09-25T12:11:20Zshyouhei (Shyouhei Urabe)shyouhei@ruby-lang.org
<ul></ul><p>We looked at this issue several times in recent developer meeting and it seems okay to merge.</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=724262018-06-06T19:31:15Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul><li><strong>File</strong> <a href="/attachments/7191">0001-Remove-HEAP_ALIGN_LOG-setting-in-configure.ac-for-Op.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/7191/0001-Remove-HEAP_ALIGN_LOG-setting-in-configure.ac-for-Op.patch">0001-Remove-HEAP_ALIGN_LOG-setting-in-configure.ac-for-Op.patch</a> added</li></ul><p>shyouhei (Shyouhei Urabe) wrote:</p>
<blockquote>
<p>We looked at this issue several times in recent developer meeting and it seems okay to merge.</p>
</blockquote>
<p>This hasn't been merged yet, and the most recent patch was before the <code>configure.in</code> to <code>configure.ac</code> renaming. Attached is an updated patch that should apply to current trunk.</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=752662018-11-29T04:40:48Zjeremyevans0 (Jeremy Evans)merch-redmine@jeremyevans.net
<ul></ul><p>jeremyevans0 (Jeremy Evans) wrote:</p>
<blockquote>
<p>shyouhei (Shyouhei Urabe) wrote:</p>
<blockquote>
<p>We looked at this issue several times in recent developer meeting and it seems okay to merge.</p>
</blockquote>
<p>This hasn't been merged yet, and the most recent patch was before the <code>configure.in</code> to <code>configure.ac</code> renaming. Attached is an updated patch that should apply to current trunk.</p>
</blockquote>
<p>This was approved back in September 2017, but it didn't make the 2.5 release. I would like it to make the 2.6 release. The latest patch still applies (with offset). Could a committer please merge it?</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=752692018-11-29T06:16:45Zshyouhei (Shyouhei Urabe)shyouhei@ruby-lang.org
<ul><li><strong>Status</strong> changed from <i>Assigned</i> to <i>Closed</i></li></ul><p>Applied in changeset trunk|r66086.</p>
<hr>
<p>Remove HEAP_ALIGN_LOG setting in configure.ac for OpenBSD/MirOS</p>
<p>The ruby setting was renamed to HEAP_PAGE_ALIGN_LOG, but the<br>
configure.in (now configure.ac) file was not updated, so the<br>
setting had no effect. The configure setting is unnecessary<br>
after OpenBSD 5.2 and MirOS has been discontinued (with the last<br>
release being over 10 years ago), so it is better to just remove<br>
the related configure setting.</p>
<p>Fix [Bug <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Bug: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaming (Closed)" href="https://bugs.ruby-lang.org/issues/13438">#13438</a>]<br>
From: Jeremy Evans <a href="mailto:code@jeremyevans.net" class="email">code@jeremyevans.net</a></p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=752702018-11-29T06:19:44Zshyouhei (Shyouhei Urabe)shyouhei@ruby-lang.org
<ul></ul><p>Oops, stunned that nobody noticed this issue for over a yer.<br>
Had just pushed the patch. Sorry for the delay!</p> Ruby master - Bug #13438: Fix heap overflow due to configure.in not being updated for HEAP_* -> HEAP_PAGE_* variable renaminghttps://bugs.ruby-lang.org/issues/13438?journal_id=752742018-11-29T06:56:02Zmame (Yusuke Endoh)mame@ruby-lang.org
<ul><li><strong>Backport</strong> changed from <i>2.2: DONTNEED, 2.3: DONTNEED, 2.4: DONTNEED</i> to <i>2.2: DONTNEED, 2.3: DONTNEED, 2.4: REQUIRED, 2.5: REQUIRED</i></li></ul>