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

Put the ModSecurity interception logic onto separated threads #289

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wfjsw
Copy link

@wfjsw wfjsw commented Aug 30, 2022

This PR is created to describe my patch to fix #227 and it is by no means a complete patch ready for merge.

The patch contains several unrelated changes, namely:

  • Logger change, from dd to ngx_log_error to accomodate my own debugging need
  • Removal of ngx_http_modsecurity_pcre_malloc_init and ngx_http_modsecurity_pcre_malloc_done. They are not used in my configuration where PCRE2 is used, and it looks suspicious for SEGVs so I commented them out as a precaution.
  • Why use NGX_OK in logging handler? Changed to NGX_DECLINED.

Not yet implemented:

  • I'm not sure whether logging phase would take advantage of this. I did not change that yet.
  • Missing a NGX_THREADS guard for Nginx setup without threading support.

Currently it passes all test suites and performs well in production.

Benchmarking is welcomed.

@airween
Copy link
Member

airween commented Apr 16, 2024

Hi @wfjsw, there is a new CI workflow test in this repository. Could you pick up the modifications to enable run those tests? Thanks!

@jeremyjpj0916
Copy link

jeremyjpj0916 commented Jun 17, 2024

@airween @wfjsw I wonder if multi threading is at least the partial answer to fix the long known problem of ModSecurity+nginx being a performance killer for throughput.

@wfjsw
Copy link
Author

wfjsw commented Jun 17, 2024

I no longer have a ModSecurity install on my machine so I'm unable to investigate further :(

Re performance issue: From the CPU loads it still seems heavy. I'd say there is real computing constraint in WAF. (Or it might be a problem caused by PCRE2. Who knows)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Synchronous process of the request destroys performance
3 participants