Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsafe implementation of ==(other) method of AttributeSet #49670

Closed
DmitryPogrebnoy opened this issue Oct 17, 2023 · 8 comments · Fixed by #49677
Closed

Unsafe implementation of ==(other) method of AttributeSet #49670

DmitryPogrebnoy opened this issue Oct 17, 2023 · 8 comments · Fixed by #49677

Comments

@DmitryPogrebnoy
Copy link

DmitryPogrebnoy commented Oct 17, 2023

Steps to reproduce

There is a problem with the implementation of method ==(other) of AttributeSet.
See

def ==(other)
attributes == other.attributes
end

So if you try to use it like this: attribute_set == null, then you get an NoMethodException.

The steps for reproduction are clear:

  1. Get an instance of AttributeSet or LazyAttributeSet.
  2. Suppose you have and instance AttributeSet in the variable my_attribute_set. Invoke the code below
my_attribute_set == nil

Expected behavior

It should return false. And be safe for any other instances

Actual behavior

NoMethodException is thrown

System configuration

Rails version: Rails 7

Ruby version: 2.7.8

@fatkodima
Copy link
Member

Can you create a reproduction test script using https://github.com/rails/rails/blob/main/guides/bug_report_templates/active_record_gem.rb template?

@yahonda yahonda added the more-information-needed When reporter needs to provide more information label Oct 17, 2023
@DmitryPogrebnoy
Copy link
Author

Here it is:

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "activerecord", "~> 7.1.0"
  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

class BugTest < Minitest::Test
  def test_attribute_set
    my_attribute_set = ActiveModel::AttributeSet.new({})

    assert_equal false, my_attribute_set == nil
  end
end

@rails-bot rails-bot bot removed the more-information-needed When reporter needs to provide more information label Oct 17, 2023
@fatkodima
Copy link
Member

Would you like to open a PR? An additional check for self.class == other.class and a test case would be enough.

@DmitryPogrebnoy
Copy link
Author

Yep, I will do it.

@fatkodima
Copy link
Member

Update to the suggestion: instead of self.class == other.class it is correct to use other.is_a?(self.class), because there is also a LazyAttributeSet class.

@eugeneius
Copy link
Member

AttributeSet is not part of Rails' public API. We shouldn't fix this problem unless there's a way to trigger it using only methods documented on https://api.rubyonrails.org/.

@fatkodima
Copy link
Member

Good point! @DmitryPogrebnoy How did you got that error? Can you share a real-world example?

@DmitryPogrebnoy
Copy link
Author

I have a debugger gem that does object introspection. And there is a nil check in there like this:
my_attribute_set == nil. So it's quite unnatural to me that it will throw an exception in any case. This is an issue for the context: https://youtrack.jetbrains.com/issue/RUBY-31972.

Also, the current implementation of the ==(other) method violates the symmetry contract, because nil == my_attribute_set returns false without any exception.

From my point of view this is definitely a bug. I don't know why we need to keep it just because this is not a part of public api.

kamipo added a commit that referenced this issue Oct 20, 2023
Make `==(other)` method of AttributeSet safe #49670
kamipo added a commit that referenced this issue Oct 20, 2023
Make `==(other)` method of AttributeSet safe #49670
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants