daurnimator / lredis

A redis client for lua
MIT License
43 stars 7 forks source link

MULTI transactions are not protected against pipelined command injection #10

Open Oozlum opened 3 years ago

Oozlum commented 3 years ago
cq:wrap(function()
  client:multi()
  cqueues.sleep(5)
  client:call('get', 'test')
  client:exec()
end)

cq:wrap(function()
  cqueues.sleep(2)
  client:ping()
end)

cq:loop()

In this example, the pipelined PING command is erroneously inserted into the MULTI. I have fixed this in PR #9 which implements locking around transactions. This requires a small change to the API:

cq:wrap(function()
  local t = client:multi()
  cqueues.sleep(5)
  t:call('get', 'test')
  t:exec()
end)

cq:wrap(function()
  cqueues.sleep(2)
  client:ping() -- this is now blocked until the exec() completes.
end)

cq:loop()
daurnimator commented 3 years ago

In this example, the pipelined PING command is erroneously inserted into the MULTI.

hmmm. I think PING shouldn't be pipelined after the MULTI, as it exists to keep the connection alive. What issue does it cause in the MULTI: an unwanted result?

Oozlum commented 3 years ago

hmmm. I think PING shouldn't be pipelined after the MULTI, as it exists to keep the connection alive. What issue does it cause in the MULTI: an unwanted result?

Worse, it gives the wrong results in ways that are hard to detect and which may break client code. It's not just ping, it will happen with any command. The above example will result in:

client:ping() --=> { ok = "QUEUED" }  -- expected "PONG"

t:call('get', 'test') --=> t:exec()[1] == "PONG" -- we don't know if this is correct or not.