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

Key integrity enforcement for get implementation #2

Merged
merged 10 commits into from
Mar 15, 2023
3 changes: 3 additions & 0 deletions lib/dalli.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ class DalliError < RuntimeError; end
# socket/server communication error
class NetworkError < DalliError; end

# socket returned unexpected value error
class SocketCorruptionError < NetworkError; end

# no server available/alive error
class RingError < DalliError; end

Expand Down
9 changes: 7 additions & 2 deletions lib/dalli/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ class Client
def initialize(servers = nil, options = {})
@normalized_servers = ::Dalli::ServersArgNormalizer.normalize_servers(servers)
@options = normalize_options(options)

if !@options[:protocol].nil? && @options[:protocol] != :binary
raise NotImplementedError, "This fork does not support the #{@options[:protocol]} protocol because safe_get is not implemented"
end

@key_manager = ::Dalli::KeyManager.new(@options)
@ring = nil
end
Expand All @@ -60,8 +65,8 @@ def initialize(servers = nil, options = {})
##
# Get the value associated with the key.
# If a value is not found, then +nil+ is returned.
def get(key, req_options = nil)
perform(:get, key, req_options)
def get(key, req_options = nil)
perform(:safe_get, key, req_options)
end

##
Expand Down
2 changes: 1 addition & 1 deletion lib/dalli/protocol/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class Base

def_delegators :@value_marshaller, :serializer, :compressor, :compression_min_size, :compress_by_default?
def_delegators :@connection_manager, :name, :sock, :hostname, :port, :close, :connected?, :socket_timeout,
:socket_type, :up!, :down!, :write, :reconnect_down_server?, :raise_down_error
:socket_type, :up!, :down!, :write, :reconnect_down_server?, :raise_down_error, :error_on_request!

def initialize(attribs, client_options = {})
hostname, port, socket_type, @weight, user_creds = ServerConfigParser.parse(attribs)
Expand Down
8 changes: 5 additions & 3 deletions lib/dalli/protocol/binary.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ def response_processor
private

# Retrieval Commands
cornu-ammonis marked this conversation as resolved.
Show resolved Hide resolved
def get(key, options = nil)
req = RequestFormatter.standard_request(opkey: :get, key: key)
def safe_get(key, options = nil)
req = RequestFormatter.standard_request(opkey: :getk, key: key)
write(req)
response_processor.get(cache_nils: cache_nils?(options))
response_processor.getk(key, cache_nils: cache_nils?(options)).last
rescue Dalli::SocketCorruptionError => e
error_on_request!(e)
end

def quiet_get_request(key)
Expand Down
2 changes: 2 additions & 0 deletions lib/dalli/protocol/binary/request_formatter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class RequestFormatter
flush: 0x08,
noop: 0x0A,
version: 0x0B,
getk: 0x0c,
getkq: 0x0D,
append: 0x0E,
prepend: 0x0F,
Expand Down Expand Up @@ -52,6 +53,7 @@ class RequestFormatter

BODY_FORMATS = {
get: KEY_ONLY,
getk: KEY_ONLY,
getkq: KEY_ONLY,
delete: KEY_ONLY,
deleteq: KEY_ONLY,
Expand Down
21 changes: 21 additions & 0 deletions lib/dalli/protocol/binary/response_processor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,27 @@ def get(cache_nils: false)
unpack_response_body(resp_header, body, true).last
end

# returns [key, value]
# raises SocketCorruptionError if the response key does not match the requested key.
# otherwise mirrors the get implementation
def getk(key, cache_nils: false)
resp_header, body = read_response

return false if resp_header.not_stored? # Not stored, normal status for add operation
return cache_nils ? ::Dalli::NOT_FOUND : [key, nil] if resp_header.not_found?

raise_on_not_ok!(resp_header)
return true unless body

res = unpack_response_body(resp_header, body, true)

if key != res.first
raise Dalli::SocketCorruptionError, "Socket corruption detected - key does not match response"
end

res
end

##
# Response for a storage operation. Returns the cas on success. False
# if the value wasn't stored. And raises an error on all other error
Expand Down