SEGV in Ruby when initializing with nil

[haberman]

Per @tmtrademarked on #8311 (filing here as it is a different bug, needing a different fix):

---

After updating from 3.14.0 to 3.15.3, we started seeing a crash that appears related, even with the fix provided in the above PR. If the Inners field contains nil, you can still get a segfault. (In 3.14.0, this does not happen)

I've modified the repro script here slightly to demonstrate the failure mode:

require 'google/protobuf'

Google::Protobuf::DescriptorPool.generated_pool.build do
  add_file("inner.proto", :syntax => :proto3) do
    add_message "Inner" do
      # Removing either of these fixes the segfault.
      optional :foo, :string, 1
      optional :bar, :string, 2
    end
  end
end

Inner = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Inner").msgclass

Google::Protobuf::DescriptorPool.generated_pool.build do
  add_file("outer.proto", :syntax => :proto3) do
    add_message "Outer" do
      repeated :inners, :message, 1, "Inner"
    end
  end
end

Outer = ::Google::Protobuf::DescriptorPool.generated_pool.lookup("Outer").msgclass

outer_proto = Outer.new(
  inners: [nil]
)
puts outer_proto

With 3.14.0, I get the following output:

tom:branch (master *)$ bundle exec ruby --disable-did_you_mean ./repro.rb 
<Outer: inners: [nil]>

With 3.15.3, I get a segfault. I don't think that's quite what the intended behavior is, right?

[haberman]

Can you clarify, what behavior do you expect from this?

We generally don't allow nil elements in repeated fields. So I am inclined to have this use case raise an exception. Would that break your use case?

[tmtrademarked]

An exception seems perfectly reasonable here. We never intended to put `nil` into the array, and we've fixed the bug in our code that allowed it. But the fact this created segfaults made it harder to track down. :-) So yeah, raising some sort of exception in this case would a fine solution for us!

…

On Mon, Mar 1, 2021, 6:38 PM Joshua Haberman ***@***.***> wrote: Can you clarify, what behavior do you expect from this? We generally don't allow nil elements in repeated fields. So I am inclined to have this use case raise an exception. Would that break your use case? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8359 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AD2RUZ43RV2GR4FNOOFPOIDTBQQQVANCNFSM4YNQCINQ> .