Bug #15536
closedCrash on merging specific hashes using keyword splat
Description
Here's a snippet that leads to a crash on ruby 2.5.3. I tried to make it as small as possible.
1000.times do
{
**{
a1: nil,
a2: nil,
a3: nil,
a4: nil,
a5: nil,
a6: nil,
a7: nil,
a8: nil,
a9: nil
},
b1: nil,
b2: nil,
a4: nil,
**{ c1: nil, c2: nil },
a7: nil,
a8: nil,
a9: nil,
}
end
Results in *** Error in irb': malloc(): memory corruption: 0x000055ca6c832bd0 ***
(more detail in the attached file).
We came across this on ruby 2.5.3 and as far as I can tell it's no longer a problem on 2.6 but we yet to upgrade.
Files
Updated by mame (Yusuke Endoh) over 5 years ago
Good catch. The following code still crashes on trunk.
{
**{
a0: nil,
a1: nil,
a2: nil,
a3: nil,
a4: nil,
a5: nil,
a6: nil,
a7: nil,
a8: nil,
},
a0: nil,
a1: nil,
a2: nil,
a3: nil,
a4: nil,
a5: nil,
a6: nil,
a7: nil,
a8: nil,
**{
c: nil
},
b0: nil,
b1: nil,
b2: nil,
b3: nil,
b4: nil,
b5: nil,
b6: nil,
b7: nil,
b8: nil,
b9: nil,
b10: nil,
b11: nil,
b12: nil,
b13: nil,
b14: nil,
b15: nil,
b16: nil,
b17: nil,
b18: nil,
b19: nil,
b20: nil,
b21: nil,
}
Here is a patch. It might have an inefficient case, but I think it is easy to backport.
diff --git a/st.c b/st.c
index c6b3644e39..ed235c674e 100644
--- a/st.c
+++ b/st.c
@@ -2299,7 +2299,7 @@ rb_hash_bulk_insert_into_st_table(long argc, const VALUE *argv, VALUE hash)
st_table *tab = RHASH_ST_TABLE(hash);
tab = RHASH_TBL_RAW(hash);
- n = tab->num_entries + size;
+ n = tab->entries_bound + size;
st_expand_table(tab, n);
if (UNLIKELY(tab->num_entries))
st_insert_generic(tab, argc, argv, hash);
Updated by mame (Yusuke Endoh) over 5 years ago
- Status changed from Open to Closed
Applied in changeset trunk|r66832.
st.c (rb_hash_bulk_insert_into_st_table): avoid out-of-bounds write
"hash_bulk_insert" first expands the table, but the target size was
wrong: it was calculated by "num_entries + (size to buld insert)", but
it was wrong when "num_entries < entries_bound", i.e., it has a deleted
entry. "hash_bulk_insert" adds the given entries from entries_bound,
which led to out-of-bounds write access. [Bug #15536]
As a simple fix, this commit changes the calculation to "entries_bound +
size". I'm afraid if this might be inefficient, but I think it is safe
anyway.
Updated by mame (Yusuke Endoh) over 5 years ago
- Backport changed from 2.4: UNKNOWN, 2.5: UNKNOWN, 2.6: UNKNOWN to 2.4: UNKNOWN, 2.5: REQUIRED, 2.6: REQUIRED
Updated by decuplet (Nikita Shilnikov) over 5 years ago
That was fast, thank you.
Updated by naruse (Yui NARUSE) over 5 years ago
- Backport changed from 2.4: UNKNOWN, 2.5: REQUIRED, 2.6: REQUIRED to 2.4: UNKNOWN, 2.5: REQUIRED, 2.6: DONE
ruby_2_6 r66853 merged revision(s) 66832.
Updated by nagachika (Tomoyuki Chikanaga) about 5 years ago
- Backport changed from 2.4: UNKNOWN, 2.5: REQUIRED, 2.6: DONE to 2.4: UNKNOWN, 2.5: DONE, 2.6: DONE
ruby_2_5 r67236 merged revision(s) 66832.