Skip to content
This repository was archived by the owner on Apr 14, 2021. It is now read-only.

Check if 'search' is initialized.#6688

Merged
bundlerbot merged 1 commit intorubygems:masterfrom
voxik:check-search
Sep 8, 2018
Merged

Check if 'search' is initialized.#6688
bundlerbot merged 1 commit intorubygems:masterfrom
voxik:check-search

Conversation

@voxik
Copy link
Copy Markdown
Contributor

@voxik voxik commented Sep 6, 2018

The search was checked previously, so maybe it should be checked also at this place.

This issue was identified by Coverity scanner:

Error: FORWARD_NULL (CWE-476):
rubygem-bundler-1.16.1/usr/share/gems/gems/bundler-1.16.1/lib/bundler/lazy_specification.rb:77: null_check: Comparing "search" to a null-like value implies that "search" might be null-like.
rubygem-bundler-1.16.1/usr/share/gems/gems/bundler-1.16.1/lib/bundler/lazy_specification.rb:83: property_access: Accessing a property of null-like value "search".
#   81|             search = source.specs.search(self).last
#   82|           end
#   83|->         search.dependencies = dependencies if search.is_a?(RemoteSpecification) || search.is_a?(EndpointSpecification)
#   84|           search
#   85|         end

@colby-swandale
Copy link
Copy Markdown
Member

Thanks @bundlerbot r+

@bundlerbot
Copy link
Copy Markdown
Collaborator

📌 Commit ed46f24 has been approved by colby-swandale

@bundlerbot
Copy link
Copy Markdown
Collaborator

⌛ Testing commit ed46f24 with merge 0aa5ea7...

bundlerbot added a commit that referenced this pull request Sep 8, 2018
Check if 'search' is initialized.

The search was checked previously, so maybe it should be checked also at this place.

This issue was identified by Coverity scanner:

~~~
Error: FORWARD_NULL (CWE-476):
rubygem-bundler-1.16.1/usr/share/gems/gems/bundler-1.16.1/lib/bundler/lazy_specification.rb:77: null_check: Comparing "search" to a null-like value implies that "search" might be null-like.
rubygem-bundler-1.16.1/usr/share/gems/gems/bundler-1.16.1/lib/bundler/lazy_specification.rb:83: property_access: Accessing a property of null-like value "search".
#   81|             search = source.specs.search(self).last
#   82|           end
#   83|->         search.dependencies = dependencies if search.is_a?(RemoteSpecification) || search.is_a?(EndpointSpecification)
#   84|           search
#   85|         end
~~~
@bundlerbot
Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-travis
Approved by: colby-swandale
Pushing 0aa5ea7 to master...

@bundlerbot bundlerbot merged commit ed46f24 into rubygems:master Sep 8, 2018
@colby-swandale colby-swandale added this to the 1.16.5 milestone Sep 10, 2018
colby-swandale pushed a commit that referenced this pull request Sep 14, 2018
Check if 'search' is initialized.

The search was checked previously, so maybe it should be checked also at this place.

This issue was identified by Coverity scanner:

~~~
Error: FORWARD_NULL (CWE-476):
rubygem-bundler-1.16.1/usr/share/gems/gems/bundler-1.16.1/lib/bundler/lazy_specification.rb:77: null_check: Comparing "search" to a null-like value implies that "search" might be null-like.
rubygem-bundler-1.16.1/usr/share/gems/gems/bundler-1.16.1/lib/bundler/lazy_specification.rb:83: property_access: Accessing a property of null-like value "search".
#   81|             search = source.specs.search(self).last
#   82|           end
#   83|->         search.dependencies = dependencies if search.is_a?(RemoteSpecification) || search.is_a?(EndpointSpecification)
#   84|           search
#   85|         end
~~~

(cherry picked from commit 0aa5ea7)
colby-swandale pushed a commit that referenced this pull request Sep 18, 2018
* 1-16-stable:
  Version 1.16.5 with changelog
  scope TruffleRuby platform specs to be RubyGems >= 2.1.0
  Auto merge of #6689 - bundler:colby/fix-bundler-load-error, r=colby-swandale
  Auto merge of #6695 - bundler:segiddins/6684-gvp-prefer-non-pres, r=colby-swandale
  Auto merge of #6693 - eregon:truffleruby, r=colby-swandale
  Auto merge of #6692 - eregon:simplify-autoload-require-deprecate, r=segiddins
  Auto merge of #6688 - voxik:check-search, r=colby-swandale
  Auto merge of #6682 - bundler:bundle_env_formatting, r=colby-swandale
  Auto merge of #6675 - MaxLap:master, r=greysteil
  Auto merge of #6669 - ChrisBr:fix_dep_proxy, r=segiddins
  Auto merge of #6664 - greysteil:avoid-printing-git-error, r=colby-swandale
@rafaelfranca
Copy link
Copy Markdown
Contributor

I know this is a ok change but is not this scanner broken? I don't see how this code would end up in an error if search is nil. nil is not a RemoteSpecification or EndpointSpecification so search.dependencies = dependencies would not be executed.

>> class RemoteSpecification; end
=> nil
>> class EndpointSpecification; end
=> nil
>> search = nil
=> nil
>> search.dependencies = dependencies if search.is_a?(RemoteSpecification) || search.is_a?(EndpointSpecification)
=> nil

@deivid-rodriguez
Copy link
Copy Markdown
Contributor

You're right!

@voxik
Copy link
Copy Markdown
Contributor Author

voxik commented Sep 23, 2018

I think the extra check makes the code more readable and actually more safe, since next time, somebody extending the condition for whatever reason, won't forget this check.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants