Skip to content

msr_global_mutex_lock: handle errors from apr_global_mutex_lock#3257

Merged
marcstern merged 9 commits into
owasp-modsecurity:v2/masterfrom
marcstern:v2/pr/msr_global_mutex_lock
Oct 2, 2024
Merged

msr_global_mutex_lock: handle errors from apr_global_mutex_lock#3257
marcstern merged 9 commits into
owasp-modsecurity:v2/masterfrom
marcstern:v2/pr/msr_global_mutex_lock

Conversation

@marcstern

@marcstern marcstern commented Sep 12, 2024 •
edited
Loading

Copy link
Copy Markdown

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 marcstern mentioned this pull request Sep 12, 2024
Closed
@marcstern

marcstern commented Sep 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Author

#3255 acknowledged to be solved by the PR

fzipi
fzipi approved these changes Sep 21, 2024
View reviewed changes

@fzipi fzipi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

theseion
theseion previously requested changes Sep 22, 2024
View reviewed changes
Comment thread apache2/modsecurity.c
Comment thread apache2/modsecurity.c
Comment thread apache2/modsecurity.c
Comment thread apache2/modsecurity.c
@airween

airween commented Sep 22, 2024

Copy link
Copy Markdown
Member

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

marcstern commented Sep 25, 2024

Copy link
Copy Markdown
Author

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

Marc Stern and others added 2 commits September 27, 2024 09:26
@theseion
Updated comment
339d6df 
Co-authored-by: Max Leske <250711+theseion@users.noreply.github.com>
@theseion
Updated log message
95dc594 
Co-authored-by: Max Leske <250711+theseion@users.noreply.github.com>
@airween

airween commented Sep 27, 2024

Copy link
Copy Markdown
Member

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

marcstern commented Sep 27, 2024

Copy link
Copy Markdown
Author

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

@airween

airween commented Sep 27, 2024

Copy link
Copy Markdown
Member

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.

airween
airween requested changes Sep 30, 2024
View reviewed changes

@airween airween left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please see my comments.

Comment thread apache2/modsecurity.c Outdated
Comment thread apache2/modsecurity.c Outdated
Comment thread apache2/msc_logging.c Outdated
Comment thread apache2/msc_logging.c Outdated
@marcstern

marcstern commented Sep 30, 2024

Copy link
Copy Markdown
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

airween commented Sep 30, 2024

Copy link
Copy Markdown
Member

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.

@sonarqubecloud

sonarqubecloud Bot commented Sep 30, 2024

Copy link
Copy Markdown

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

marcstern commented Oct 2, 2024

Copy link
Copy Markdown
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

airween
airween approved these changes Oct 2, 2024
View reviewed changes
Comment thread apache2/modsecurity.c
@airween

airween commented Oct 2, 2024

Copy link
Copy Markdown
Member

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
@marcstern marcstern deleted the v2/pr/msr_global_mutex_lock branch October 3, 2024 14:50
@marcstern marcstern restored the v2/pr/msr_global_mutex_lock branch October 22, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@airween airween airween approved these changes
@fzipi fzipi fzipi approved these changes
@theseion theseion theseion left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@marcstern @airween @theseion @fzipi