Protect Processor::Context#run with a mutex #2773
Conversation
lloeki
force-pushed
the
add-processor-run-mutex
branch
from
d26a468
to
00f8033
Compare
There was a problem hiding this comment.
This seems reasonable, but it's a very small/subtle code change for a potential big issue. Thus, I would suggest perhaps leaving a few comments in the code explaining why this tiny addition is important, otherwise this may get hard to maintain in the future (since people would need to partially "reverse engineer" why this was added here and what it's protecting exactly).
| @run_mutex.lock | ||
|
|
||
| start_ns = Core::Utils::Time.get_time(:nanosecond) |
There was a problem hiding this comment.
Minor: Consider using @run_mutex.synchronize -- I think that one's a bit easier to maintain and "break" with future changes.
There was a problem hiding this comment.
Rationale:
- blocks carry a cost: def/ensure/end is faster
- it makes the function return value less explicit (you have to konw/remember/lookup that synchronize does return the block value)
- it creates whitespace churn on the code change
- maybe I'm doing too much Go but def/ensure/end is a common pattern that matches Go's
defer;)
There was a problem hiding this comment.
Added a comment to clarify the intent!
lloeki
force-pushed
the
add-processor-run-mutex
branch
from
00f8033
to
20718a5
Compare
lloeki
force-pushed
the
add-processor-run-mutex
branch
from
fd72683
to
2994528
Compare
| @time_ns += res.total_runtime | ||
| @time_ext_ns += (stop_ns - start_ns) | ||
| @timeouts += 1 if res.timeout | ||
|
|
||
| res | ||
| ensure | ||
| @run_mutex.unlock | ||
| end | ||
|
|
||
| def finalize |
There was a problem hiding this comment.
Just a final question -- does @context.finalize not need protecting too?
There was a problem hiding this comment.
Nope, that one is fine.
Context initialization and finalization happen on the same thread and should outlive any transient thread that would theoretically use the same context (you'd get much bigger problems otherwise, that a simple synchronize would not solve). It's all theoretical anyway because no code is currently allowing getting the context from another thread, so this is mostly bulletproofing following a discussion around run.
What does this PR do?
Protect Processor::Context#run with a mutex
Motivation
ddwaf_runmutatesddwaf_contextin a non-thread-safe way.Additional Notes
Sicne there is a 1:1:1 context - request - web worker thread mapping it should very rarely happen in typical apps so the lock will just fly through.
This aims to protect from code that spawns threads within a request and that would theoretically trigger a context run (e.g ActionController::Live). There is currently none but there could be in the future.
How to test the change?
Difficult since there's no such real case currently. Even synthetically (e.g in a spec) it would be hard as it would require producing a reliably failing case of libddwaf stepping onto itself.