Skip to content

Commit

Permalink
Avoid the constant redefinition warning
Browse files Browse the repository at this point in the history 
The gem and bundle commands first load digest via openssl, so loading
the digest gem would cause this warning every time one of these
commands is run:

```
.../lib/ruby/gems/3.0.0/gems/digest-3.1.0/lib/digest.rb:11: warning: already initialized constant Digest::REQUIRE_MUTEX
.../lib/ruby/3.0.0/digest.rb:7: warning: previous definition of REQUIRE_MUTEX was here
```
  • Loading branch information
@knu
knu committed Oct 1, 2021
1 parent 5184207 commit 1617261
Showing 1 changed file with 15 additions and 1 deletion.
16 changes: 15 additions & 1 deletion lib/digest.rb
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,24 @@
# frozen_string_literal: false

# The gem and bundle commands (except for bundle exec) first load
# digest via openssl and then load gems, so if this is installed via
# gem, we are overwriting the default version of digest. Beware not
# to break it or cause redefinition warnings.
#
# When we introduce incompatible changes and overwriting is not an
# option, and given that the default version does not have security
# defects, we may just give up and let those commands just use the
# default version of digest.
#
# return if defined?(Digest) && caller_locations.any? { |l|
# %r{/(rubygems/gem_runner|bundler/cli)\.rb}.match?(l.path)
# }

require 'digest/loader'

module Digest
# A mutex for Digest().
REQUIRE_MUTEX = Thread::Mutex.new
REQUIRE_MUTEX ||= Thread::Mutex.new

def self.const_missing(name) # :nodoc:
case name
Expand Down

16 comments on commit 1617261

@deivid-rodriguez
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@knu May I ask how this can be reproduced? Does it happen with the latest bundler and/or rubygems? I think it's not a good practice to allow two potentially different copies of the same gem to be mixed together so this warning is a good indication that some issue is happening. I'd like to fix the real culprit instead

@knu
Copy link
Member Author

@knu knu commented on 1617261 Oct 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deivid-rodriguez Sure, build the gem without this change and install it, then run any gem command and you'll get the redefinition error. This is because the rubygem library requires openssl which pulls in digest via the OpenSSL::Digest module and the newly installed digest gem is loaded after the digest library is loaded.

@deivid-rodriguez
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do OpenSSL::Digest and Digest are different implementations of the same functionality. Which one is preferred?

@deivid-rodriguez
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If OpenSSL::Digest does what we need, this patch to rubygems should do the trick and avoid the double load

diff --git a/lib/rubygems/s3_uri_signer.rb b/lib/rubygems/s3_uri_signer.rb
index bba9afc9ff..df4f570785 100644
--- a/lib/rubygems/s3_uri_signer.rb
+++ b/lib/rubygems/s3_uri_signer.rb
@@ -1,4 +1,3 @@
-require 'digest'
 require 'rubygems/openssl'
 
 ##
@@ -87,7 +86,7 @@ def generate_string_to_sign(date_time, credential_info, canonical_request)
       "AWS4-HMAC-SHA256",
       date_time,
       credential_info,
-      Digest::SHA256.hexdigest(canonical_request),
+      OpenSSL::Digest::SHA256.hexdigest(canonical_request),
     ].join("\n")
   end

@deivid-rodriguez
Copy link

@deivid-rodriguez deivid-rodriguez commented on 1617261 Oct 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand the problem better now. I think the problem is that openssl requires digest from the C-extension, so that require is ruby's built-in require which doesn't include rubygems decorations to be able to activate gems, resulting in the default version of digest being required but not activated as a gem.

While the above patch avoids the issue for usages of gem, if end users activate the digest gem, they will get the same error I believe. I think a better solution would be to change openssl to require digest from ruby, so that the proper version is activated as a gem.

@knu
Copy link
Member Author

@knu knu commented on 1617261 Oct 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you got the situation correctly. Actually, OpenSSL::Digest is built with/on top of the digest API so it is not an option to stop requiring digest, but you could possibly change the openssl library so that openssl/digest is a separate module with autoload enabled, triggered by using OpenSSL::Digest. I haven't yet looked into it closely, though.

@deivid-rodriguez
Copy link

@deivid-rodriguez deivid-rodriguez commented on 1617261 Oct 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm going to introduce the patch in rubygems anyways to not require digest explicitly and let openssl deal with that instead, so you might want to revert this commit. But I'll also look into fixing this in general inside openssl. Thanks!

@rhenium
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loading openssl after installing a digest gem already brings digest into a broken state:

$ gem list digest

*** LOCAL GEMS ***

digest (3.1.0.pre2, default: 3.0.1.pre)
$ ruby -ropenssl -e'puts $LOADED_FEATURES.grep(/digest|openssl/)'         
/opt/ruby/master/lib/ruby/gems/3.1.0/gems/digest-3.1.0.pre2/lib/digest.so
/opt/ruby/master/lib/ruby/3.1.0/digest.rb
/opt/ruby/master/lib/ruby/3.1.0/x86_64-linux/openssl.so
[...]

My first thought was to simply declare digest as a runtime dependency in openssl's gemspec, but that won't work because RubyGems activates dependency gems immediately only when exactly one matching version is installed on the system. TIL...

I think a better solution would be to change openssl to require digest from ruby, so that the proper version is activated as a gem.

Agreed.

@deivid-rodriguez
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Loading openssl after installing a digest gem already brings digest into a broken state:

Yes, although that will be fixed by rubygems/rubygems#4979. But even with that, if any user code that tries to load digest, the issue will reappear.

My first thought was to simply declare digest as a runtime dependency in openssl's gemspec, but that won't work because RubyGems activates dependency gems immediately only when exactly one matching version is installed on the system. TIL...

Indeed! Still, in my opinion explicitly declaring dependencies is a good practice, although not everybody agrees there: https://bugs.ruby-lang.org/issues/16951.

@deivid-rodriguez
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@knu Changes have been introduced to both openssl and rubygems to avoid this issue. Should we revert this commit now?

@rhenium
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix in openssl happened in ruby/openssl#463 and it's included in openssl gem 2.1.3/2.2.1.

I think this should be reverted -- the warning in the commit message shows that digest is in an inconsistent state and the user should know that.

@knu
Copy link
Member Author

@knu knu commented on 1617261 Oct 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but does these fixes apply to already released ruby versions?

@deivid-rodriguez
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but digest is not a default gem in those rubies, so I don't think the warning can happen there.

@knu
Copy link
Member Author

@knu knu commented on 1617261 Oct 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean I'm sure you'll see the warning with Ruby version 3.0.[0-2]. The warning would only be likely to occur either while running a gem/bundle command or rubygems/bundler went wrong somehow, so I think it is just annoying more than it is helpful. I just hope the fixes will be part of 3.0.3 and 3.1.0, and I'll add a version guard and limit the hack to 3.0.[0-2].

@deivid-rodriguez
Copy link

@deivid-rodriguez deivid-rodriguez commented on 1617261 Oct 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you're right, digest is a default gem since ruby 3.0. Your plan sounds good to me! As far as I understand, ruby 3.0.3 will include openssl 2.2.1, so it should be all good.

@knu
Copy link
Member Author

@knu knu commented on 1617261 Oct 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I forgot to say thank you so much for your effort! It is much appreciated.

Please sign in to comment.