Skip to content

Commit

Permalink
Fix show_backtraces: false leaking Exception#cause
Browse files Browse the repository at this point in the history
This code was likely fine back when it was written against Ruby 1.9.3,
but since then `Kernel#raise` now automatically attach the currently rescued error
as the new exception `cause`.

Since this code was re-raising the same instance over and once a `cause` was
attached it would never be de-associated, which in some contrived scenario could
leak to information leak across requests / test etc.

I ran a benchmark and the fastest way to raise exception I could find is:

```ruby
raise ErrorClass, "message".freeze, EMPTY_ARRAY, cause: nil
```
  • Loading branch information
byroot committed Jan 24, 2022
1 parent b1372bf commit a553ff0
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 28 deletions.
16 changes: 1 addition & 15 deletions lib/memcached/exceptions.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,21 +45,7 @@ class Memcached
* Memcached::WriteFailure
=end
class Error < RuntimeError
attr_accessor :no_backtrace

def initialize(*args)
super(*args)
end

def set_backtrace(*args)
no_backtrace ? [] : super
end

def backtrace(*args)
no_backtrace ? [] : super
end
end
Error = Class.new(RuntimeError)

#:stopdoc:

Expand Down
17 changes: 4 additions & 13 deletions lib/memcached/memcached.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
=end
class Memcached
FLAGS = 0x0
EMPTY_ARRAY = [].freeze

DEFAULTS = {
:hash => :fnv1_32,
Expand Down Expand Up @@ -174,17 +175,7 @@ def initialize(servers = nil, opts = {})
set_servers(servers)

# Not found exceptions
if options[:show_backtraces]
# Raise classes for full context
@not_found = NotFound
@not_stored = NotStored
else
# Raise fast context-free exception instances
@not_found = NotFound.new
@not_found.no_backtrace = true
@not_stored = NotStored.new
@not_stored.no_backtrace = true
end
@show_backtraces = options[:show_backtraces] ? nil : EMPTY_ARRAY
end

# Set the server list.
Expand Down Expand Up @@ -597,9 +588,9 @@ def check_return_code(ret, key = nil) #:doc:
if ret == 0 # Lib::MEMCACHED_SUCCESS
elsif ret == 32 # Lib::MEMCACHED_BUFFERED
elsif ret == 16
raise @not_found # Lib::MEMCACHED_NOTFOUND
raise NotFound, "NOTFOUND".freeze, @show_backtraces, cause: nil # Lib::MEMCACHED_NOTFOUND
elsif ret == 14
raise @not_stored # Lib::MEMCACHED_NOTSTORED
raise NotStored, "NOTSTORED".freeze, @show_backtraces, cause: nil # Lib::MEMCACHED_NOTSTORED
else
reraise(key, ret)
end
Expand Down
27 changes: 27 additions & 0 deletions test/unit/memcached_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,33 @@ def test_initialize_without_backtraces
end
end

def test_without_backtrace_no_cause
cache = Memcached.new @servers, show_backtraces: false

# show_backtraces: false was implemented by always re-raising the same instance.
# This was likely fine back in Ruby 1.9.3. But since then now Ruby automatically
# attach a `cause` if you raise from a `rescue` or `ensure` and it's never cleared
# from the instance causing information to leak.

rescued = false

begin
raise "foo"
rescue RuntimeError => error
begin
cache.get key
rescue Memcached::NotFound
rescued = true
end
end
assert rescued

error = assert_raises Memcached::NotFound do
cache.get key
end
assert_nil error.cause
end

def test_initialize_with_backtraces
cache = Memcached.new @servers, :show_backtraces => true
cache.delete key rescue nil
Expand Down

0 comments on commit a553ff0

Please sign in to comment.