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

msr_global_mutex_lock: handle errors from apr_global_mutex_lock #3257

Merged

Conversation

marcstern
Copy link
Contributor

@marcstern marcstern commented Sep 12, 2024

apr_global_mutex_lock is sometimes called with a lock that wasn't created (for any reason).
In this case, the pointer is null and apr_global_mutex_lock dereferences a null pointer, leading to a crash.
This PR creates a wrapper around apr_global_mutex_lock to handle checking that and correct logging.
Same for msr_global_mutex_unlock.

@marcstern
Copy link
Contributor Author

marcstern commented Sep 20, 2024

#3255 acknowledged to be solved by the PR

Copy link
Contributor

@fzipi fzipi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

apache2/modsecurity.c Show resolved Hide resolved
apache2/modsecurity.c Show resolved Hide resolved
apache2/modsecurity.c Show resolved Hide resolved
apache2/modsecurity.c Show resolved Hide resolved
@airween
Copy link
Member

airween commented Sep 22, 2024

Before we merge, I want to check why the lock acquire was failed (see #3255 - the user reported that this issue came with the new version where we introduced this method).

@marcstern
Copy link
Contributor Author

The problem was already happening in the previous versions. Looks to me linked to concurrency.

marcstern and others added 2 commits September 27, 2024 09:26
Co-authored-by: Max Leske <[email protected]>
Co-authored-by: Max Leske <[email protected]>
@airween
Copy link
Member

airween commented Sep 27, 2024

The problem was already happening in the previous versions. Looks to me linked to concurrency.

May be - but now the audit.log is empty (with this patch too).

I think we should investigate this issue. I already installed FreeBSD and could reproduce the behavior, so I'm on it.

@marcstern
Copy link
Contributor Author

the audit.log is empty, but you should have an entry in the error log (about the problem at creation time)

@airween
Copy link
Member

airween commented Sep 27, 2024

the audit.log is empty, but you should have an entry in the error log (about the problem at creation time)

Indeed, but I think we should investigate the issue.

Copy link
Member

@airween airween left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comments.

apache2/modsecurity.c Outdated Show resolved Hide resolved
apache2/modsecurity.c Outdated Show resolved Hide resolved
apache2/msc_logging.c Outdated Show resolved Hide resolved
apache2/msc_logging.c Outdated Show resolved Hide resolved
@marcstern
Copy link
Contributor Author

the audit.log is empty, but you should have an entry in the error log (about the problem at creation time)

Indeed, but I think we should investigate the issue.
Correct. We should open a new ticket for this I guess (or continue #3255)

@airween
Copy link
Member

airween commented Sep 30, 2024

Indeed, but I think we should investigate the issue.
Correct. We should open a new ticket for this I guess (or continue #3255)

yes, I made some investigation - see my comment there.

Copy link

sonarcloud bot commented Sep 30, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
5.9% Duplication on New Code (required ≤ 3%)
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

@marcstern
Copy link
Contributor Author

I propose to accept this PR first, then create one that doesn't use the temp filename, as it's not the right way to create a mutx, see https://lists.apache.org/thread/ykb26kg4lgcqnldvxwd9p6hv16fy4z9l

apache2/modsecurity.c Show resolved Hide resolved
@airween
Copy link
Member

airween commented Oct 2, 2024

I propose to accept this PR first, then create one that doesn't use the temp filename, as it's not the right way to create a mutx, see https://lists.apache.org/thread/ykb26kg4lgcqnldvxwd9p6hv16fy4z9l

Okay, I approved this PR and resolved all conversation. You can merge this PR now.

@marcstern marcstern dismissed theseion’s stale review October 2, 2024 15:09

Implemented all requested changes but a comment

@marcstern marcstern merged commit 090e4d3 into owasp-modsecurity:v2/master Oct 2, 2024
40 of 41 checks passed
@marcstern marcstern deleted the v2/pr/msr_global_mutex_lock branch October 3, 2024 14:50
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.

4 participants