From a2c76998853ccde5f312aed70e416b734c4a11da Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 15 Aug 2023 12:42:52 -0700 Subject: [PATCH] ruby: Fix object cache lookups on 32-bit platforms (#13494) https://github.com/protocolbuffers/protobuf/pull/13204 refactored the Ruby object cache to use a key of `LL2NUM(key_val)` instead of `LL2NUM(key_val >> 2)`. On 32-bit systems, `LL2NUM(key_val)` returns inconsistent results because a large value has to be stored as a Bignum on the heap. This causes cache lookups to fail. This commit restores the previous behavior of using `ObjectCache_GetKey`, which discards the lower 2 bits, which are zero. This enables a key to be stored as a Fixnum on both 32 and 64-bit platforms. As https://patshaughnessy.net/2014/1/9/how-big-is-a-bignum describes, a Fixnum uses: * 1 bit for the `FIXNUM_FLAG`. * 1 bit for the sign flag. Therefore the largest possible Fixnum value on a 64-bit value is 4611686018427387903 (2^62 - 1). On a 32-bit system, the largest value is 1073741823 (2^30 - 1). For example, a possible VALUE pointer address on a 32-bit system: 0xff5b4af8 => 4284173048 Dropping the lower 2 bits makes up for the loss of range to these flags. In the example above, we see that shifting by 2 turns the value into a 30-bit number, which can be represented as a Fixnum: (0xff5b4af8 >> 2) => 1071043262 This bug can also manifest on a 64-bit system if the upper bits are 0xff. Closes #13481 Closes #13494 COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/13494 from stanhu:sh-fix-ruby-protobuf-32bit d63122a6fc6eb21b81cc37412d4c4f8730f46125 FUTURE_COPYBARA_INTEGRATE_REVIEW=https://github.com/protocolbuffers/protobuf/pull/13494 from stanhu:sh-fix-ruby-protobuf-32bit d63122a6fc6eb21b81cc37412d4c4f8730f46125 PiperOrigin-RevId: 557216800 --- .github/workflows/test_ruby.yml | 33 +++++++++++++++++++++++++++ ruby/ext/google/protobuf_c/protobuf.c | 18 +++++++++++---- 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/.github/workflows/test_ruby.yml b/.github/workflows/test_ruby.yml index 39dfaa9de72c..46f6d12a9bf1 100644 --- a/.github/workflows/test_ruby.yml +++ b/.github/workflows/test_ruby.yml @@ -45,6 +45,39 @@ jobs: bazel-cache: ruby_linux/${{ matrix.ruby }}_${{ matrix.bazel }} bazel: test //ruby/... //ruby/tests:ruby_version --test_env=KOKORO_RUBY_VERSION --test_env=BAZEL=true ${{ matrix.ffi == 'FFI' && '--//ruby:ffi=enabled --test_env=PROTOCOL_BUFFERS_RUBY_IMPLEMENTATION=FFI' || '' }} + linux-32bit: + name: Linux 32-bit + runs-on: ubuntu-latest + steps: + - name: Checkout pending changes + uses: actions/checkout@ac593985615ec2ede58e132d2e21d2b1cbd6127c # v3.3.0 + with: + ref: ${{ inputs.safe-checkout }} + submodules: recursive + + - name: Cross compile protoc for i386 + id: cross-compile + uses: protocolbuffers/protobuf-ci/cross-compile-protoc@v2 + with: + image: us-docker.pkg.dev/protobuf-build/containers/common/linux/bazel:6.3.0-91a0ac83e968068672bc6001a4d474cfd9a50f1d + credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }} + architecture: linux-i386 + + - name: Run tests + uses: protocolbuffers/protobuf-ci/docker@v2 + with: + image: i386/ruby:2.7.3-buster + skip-staleness-check: true + credentials: ${{ secrets.GAR_SERVICE_ACCOUNT }} + command: >- + /bin/bash -cex ' + gem install bundler; + cd /workspace/ruby; + bundle; + PROTOC=/workspace/${{ steps.cross-compile.outputs.protoc }} rake; + rake clobber_package gem; + PROTOC=/workspace/${{ steps.cross-compile.outputs.protoc }} rake test' + linux-aarch64: name: Linux aarch64 runs-on: ubuntu-latest diff --git a/ruby/ext/google/protobuf_c/protobuf.c b/ruby/ext/google/protobuf_c/protobuf.c index c9354db4e340..39062cfca65a 100644 --- a/ruby/ext/google/protobuf_c/protobuf.c +++ b/ruby/ext/google/protobuf_c/protobuf.c @@ -276,17 +276,25 @@ static void ObjectCache_Init(VALUE protobuf) { rb_const_set(protobuf, rb_intern("SIZEOF_VALUE"), INT2NUM(SIZEOF_VALUE)); } -VALUE ObjectCache_TryAdd(const void *key, VALUE val) { +static VALUE ObjectCache_GetKey(const void *key) { VALUE key_val = (VALUE)key; PBRUBY_ASSERT((key_val & 3) == 0); - return rb_funcall(weak_obj_cache, item_try_add, 2, LL2NUM(key_val), val); + // Ensure the key can be stored as a Fixnum since 1 bit is needed for + // FIXNUM_FLAG and 1 bit is needed for the sign bit. + VALUE new_key = LL2NUM(key_val >> 2); + PBRUBY_ASSERT(FIXNUM_P(new_key)); + return new_key; +} + +VALUE ObjectCache_TryAdd(const void *key, VALUE val) { + VALUE key_val = ObjectCache_GetKey(key); + return rb_funcall(weak_obj_cache, item_try_add, 2, key_val, val); } // Returns the cached object for this key, if any. Otherwise returns Qnil. VALUE ObjectCache_Get(const void *key) { - VALUE key_val = (VALUE)key; - PBRUBY_ASSERT((key_val & 3) == 0); - return rb_funcall(weak_obj_cache, item_get, 1, LL2NUM(key_val)); + VALUE key_val = ObjectCache_GetKey(key); + return rb_funcall(weak_obj_cache, item_get, 1, key_val); } /*