Bug #13595: rb_alloc_tmp_buffer2 broken when: elsize % sizeof(VALUE) == 0

• Status: Closed
• Assignee: naruse (Yui NARUSE)
• Target version: -
• Backport: 2.2: DONTNEED, 2.3: DONTNEED, 2.4: DONE

[normalperson (Eric Wong)]

Here is the function in full as of current trunk (r58863):

static inline void *
rb_alloc_tmp_buffer2(volatile VALUE *store, long count, size_t elsize)
{
    size_t cnt = (size_t)count;
    if (elsize % sizeof(VALUE) == 0) {
	if (RB_UNLIKELY(cnt > LONG_MAX / sizeof(VALUE))) {
	    ruby_malloc_size_overflow(cnt, elsize);
	}
    }
    else {
	size_t size, max = LONG_MAX - sizeof(VALUE) + 1;
	if (RB_UNLIKELY(rb_mul_size_overflow(cnt, elsize, max, &size))) {
	    ruby_malloc_size_overflow(cnt, elsize);
	}
	cnt = (size + sizeof(VALUE) - 1) / sizeof(VALUE);
    }
    return rb_alloc_tmp_buffer_with_count(store, cnt * sizeof(VALUE), cnt);
}

Notice that elsize is completely ignored in the top branch when
"(elsize % sizeof(VALUE) == 0)" is true; this gives me problems
when attempting to use ALLOCV_N.

I am terrible at arithmetic and this function is too complicated for me,
so I'll let naruse or someone else fix this. But please do. Thanks

[normalperson (Eric Wong)]

[e-mail] wrote:

Bug #13595: rb_alloc_tmp_buffer2 broken when: elsize % sizeof(VALUE) == 0
https://bugs.ruby-lang.org/issues/13595

Perhaps the following patch is what is needed:

--- a/include/ruby/ruby.h
+++ b/include/ruby/ruby.h
@@ -1611,7 +1611,7 @@ static inline void *
 rb_alloc_tmp_buffer2(volatile VALUE *store, long count, size_t elsize)
 {
 size_t cnt = (size_t)count;
-    if (elsize % sizeof(VALUE) == 0) {
+    if (elsize == sizeof(VALUE)) {
 	if (RB_UNLIKELY(cnt > LONG_MAX / sizeof(VALUE))) {
 	    ruby_malloc_size_overflow(cnt, elsize);
 	}
 
 ...
 } /* else {...} */
 return rb_alloc_tmp_buffer_with_count(store, cnt * sizeof(VALUE), cnt);

Sorry; I am horrible at arithmetic. Honestly I have panic
attacks whenever I try to do it. Any help checking this
function would be greatly appreciated. Thank you.

[nobu (Nobuyoshi Nakada)]

• Description updated (diff)

Maybe like this?

diff --git a/include/ruby/ruby.h b/include/ruby/ruby.h
index 0b277dce19..95dab1bf3f 100644
--- a/include/ruby/ruby.h
+++ b/include/ruby/ruby.h
@@ -1612,9 +1612,10 @@ rb_alloc_tmp_buffer2(volatile VALUE *store, long count, size_t elsize)
 {
     size_t cnt = (size_t)count;
     if (elsize % sizeof(VALUE) == 0) {
-	if (RB_UNLIKELY(cnt > LONG_MAX / sizeof(VALUE))) {
+	if (RB_UNLIKELY(cnt > LONG_MAX / elsize)) {
 	    ruby_malloc_size_overflow(cnt, elsize);
 	}
+	cnt *= elsize / sizeof(VALUE);
     }
     else {
 	size_t size, max = LONG_MAX - sizeof(VALUE) + 1;

[nobu (Nobuyoshi Nakada)]

Reconsidered and yours seems the intended.

• Status changed from Open to Closed

Applied in changeset trunk|r58902.

---

attempt to fix rb_alloc_tmp_buffer2 for ALLOCV_N

This is a confusing function to my arithmetic-challenged mind,
but nobu seems alright with this. Anyways this lets me use
large values of elsize without segfaulting, and "make exam"
passes.

• include/ruby/ruby.h (rb_alloc_tmp_buffer2): attempt to fix
  [ruby-core:81388] [ruby-core:81391] [Bug #13595]

[usa (Usaku NAKAMURA)]

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

[nagachika (Tomoyuki Chikanaga)]

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

ruby_2_4 r59408 merged revision(s) 58902.