Replace tombstone when converting AR to ST hash

[Bug #21170]

st_table reserves -1 as a special hash value to indicate that an entry
has been deleted. So that that's a valid value to be returned from the
hash function, do_hash replaces -1 with 0 so that it is not mistaken for
the sentinel.

Previously, when upgrading an AR table to an ST table,
rb_st_add_direct_with_hash was used which did not perform the same
conversion, this could lead to a hash in a broken state where one if its
entries which was supposed to exist being marked as a tombstone.

The hash could then become further corrupted when the ST table required
resizing as the falsely tombstoned entry would be skipped but it would
be counted in num entries, leading to an uninitialized entry at index
15.

In most cases this will be really rare, unless using a very poorly
implemented custom hash function.

This also adds two debug assertions, one that st_add_direct_with_hash
does not receive the reserved hash value, and a second in
rebuild_table_with, which ensures that after we rebuild/compact a table
it contains the expected number of elements.

Co-authored-by: Alan Wu <alanwu@ruby-lang.org>

Author: jhawthorn

common.mk

@@ -17680,6 +17680,7 @@ st.$(OBJEXT): {$(VPATH)}internal/variable.h
 st.$(OBJEXT): {$(VPATH)}internal/warning_push.h
 st.$(OBJEXT): {$(VPATH)}internal/xmalloc.h
 st.$(OBJEXT): {$(VPATH)}missing.h
+st.$(OBJEXT): {$(VPATH)}ruby_assert.h
 st.$(OBJEXT): {$(VPATH)}st.c
 st.$(OBJEXT): {$(VPATH)}st.h
 st.$(OBJEXT): {$(VPATH)}subst.h

st.c

@@ -103,20 +103,21 @@
 #ifdef NOT_RUBY
 #include "regint.h"
 #include "st.h"
+#include <assert.h>
 #elif defined RUBY_EXPORT
 #include "internal.h"
 #include "internal/bits.h"
 #include "internal/hash.h"
 #include "internal/sanitizers.h"
 #include "internal/st.h"
+#include "ruby_assert.h"
 #endif
 
 #include <stdio.h>
 #ifdef HAVE_STDLIB_H
 #include <stdlib.h>
 #endif
 #include <string.h>
-#include <assert.h>
 
 #ifdef __GNUC__
 #define PREFETCH(addr, write_p) __builtin_prefetch(addr, write_p)
@@ -314,17 +315,22 @@ static const struct st_features features[] = {
 #define RESERVED_HASH_VAL (~(st_hash_t) 0)
 #define RESERVED_HASH_SUBSTITUTION_VAL ((st_hash_t) 0)
 
-/* Return hash value of KEY for table TAB.  */
 static inline st_hash_t
-do_hash(st_data_t key, st_table *tab)
+normalize_hash_value(st_hash_t hash)
 {
-    st_hash_t hash = (st_hash_t)(tab->type->hash)(key);
-
     /* RESERVED_HASH_VAL is used for a deleted entry.  Map it into
        another value.  Such mapping should be extremely rare.  */
     return hash == RESERVED_HASH_VAL ? RESERVED_HASH_SUBSTITUTION_VAL : hash;
 }
 
+/* Return hash value of KEY for table TAB.  */
+static inline st_hash_t
+do_hash(st_data_t key, st_table *tab)
+{
+    st_hash_t hash = (st_hash_t)(tab->type->hash)(key);
+    return normalize_hash_value(hash);
+}
+
 /* Power of 2 defining the minimal number of allocated entries.  */
 #define MINIMAL_POWER2 2
 
@@ -784,6 +790,8 @@ rebuild_table_with(st_table *const new_tab, st_table *const tab)
         new_tab->num_entries++;
         ni++;
     }
+
+    assert(new_tab->num_entries == tab->num_entries);
 }
 
 static void
@@ -1173,6 +1181,8 @@ st_add_direct_with_hash(st_table *tab,
     st_index_t ind;
     st_index_t bin_ind;
 
+    assert(hash != RESERVED_HASH_VAL);
+
     rebuild_table_if_necessary(tab);
     ind = tab->entries_bound++;
     entry = &tab->entries[ind];
@@ -1190,7 +1200,7 @@ void
 rb_st_add_direct_with_hash(st_table *tab,
                            st_data_t key, st_data_t value, st_hash_t hash)
 {
-    st_add_direct_with_hash(tab, key, value, hash);
+    st_add_direct_with_hash(tab, key, value, normalize_hash_value(hash));
 }
 
 /* Insert (KEY, VALUE) into table TAB.  The table should not have

test/ruby/test_hash.rb

@@ -2389,4 +2389,18 @@ def hash
       end
     end;
   end
+
+  def test_ar_to_st_reserved_value
+    klass = Class.new do
+      attr_reader :hash
+      def initialize(val) = @hash = val
+    end
+
+    values = 0.downto(-16).to_a
+    hash = {}
+    values.each do |val|
+      hash[klass.new(val)] = val
+    end
+    assert_equal values, hash.values, "[ruby-core:121239] [Bug #21170]"
+  end
 end