Incorrect log unlock

[marcstern]

In modsecurity.c, line 143, a lock can fail:

rc = apr_global_mutex_create(&msce->auditlog_lock, auditlog_lock_name, APR_LOCK_DEFAULT, mp);
if (rc != APR_SUCCESS) {
    //ap_log_error(APLOG_MARK, APLOG_ERR, rv, s, "mod_security: Could not create modsec_auditlog_lock");
    //return HTTP_INTERNAL_SERVER_ERROR;
    return -1;
}

In msc_logging.c, line1454, we always try to unlock it:
rc = apr_global_mutex_unlock(msr->modsecurity->auditlog_lock);
This leads to errors in the log.

We should add a watchdog:

+ msce->auditlog_lock = NULL;
rc = apr_global_mutex_create(&msce->auditlog_lock,...

+ if (msr->modsecurity->auditlog_lock) {
rc = apr_global_mutex_unlock(msr->modsecurity->auditlog_lock);
...
+ }

[zimmerle]

Hi @marcstern, thank you for the report.

modsecurity.c

that is what we have on the line 143 -

https://github.com/SpiderLabs/ModSecurity/blob/12cefbd70f2aab802e1bff53c50786f3b8b89359/apache2/modsecurity.c#L143

I believe you don't have the most recent version or a custom one. The code that you are referring to is available here:

https://github.com/SpiderLabs/ModSecurity/blob/12cefbd70f2aab802e1bff53c50786f3b8b89359/apache2/modsecurity.c#L137-L142

msc_logging.c

As for the msc_logging.c this what we have on line 1454 -

https://github.com/SpiderLabs/ModSecurity/blob/12cefbd70f2aab802e1bff53c50786f3b8b89359/apache2/msc_logging.c#L1454-L1456

I believe that you meant to point towards here -
https://github.com/SpiderLabs/ModSecurity/blob/12cefbd70f2aab802e1bff53c50786f3b8b89359/apache2/msc_logging.c#L1459-L1464

The common scenario where the mutex is not created is due to permission issues, what leads you to believe that a brute force approach, like you are suggesting, will solve the mutex creation?

Block the initialization of ModSecurity is not a good idea either, as that may imply to force Apache to not start due to the lack of AuditLog support. I think that as the main motivation of having this initialization code commented out 14 years ago -
3f80fda.

I am going to close this issue, as i think I already exposed the limitation of having this as is. The idea of having this watchdog will not solve the kernel of the problem, yet, is fair the have the user informed about the reason why the auditlogs is not being generated. If I am missing something, please, re-open the issue.

[marcstern]

Why don't we log the creation failure but the unlock failure?
The other way around seems much more logical, no?

[zimmerle]

Why don't we log the creation failure but the unlock failure?
The other way around seems much more logical, no?

Either way the user will be informed about the issue. But, the second check is there for others reasons, we hake to keep it. So, adding a new check is possible and will improve the usability for the user. Not a bug fix but an improvement.

It won't improvement to the usability if we shown the error twice (config: I. check II. loading) and if we somehow interfere on Apache life cycle, particular the loading time (e.g. refuse to load due the utilization of an ap_error_xxx). Having said that, what is your practical suggestion to have this treated as an warning?

[marcstern]

We should log the real problem: the mutex creation => uncomment lines 139 & 140.
If we don't stop (I won't discuss this choice), then we should not log the mutex deletion failure if the creation was logged => my fix is inline with that.

[zimmerle]

@marcstern

• I complete agree with you, we should warn the user that the mutex failed to be created -- As part of apache configuration parser I am afraid that there are some limitations. We can improve the run time warning by having a better warning message.
• I complete disagree in hiding error messages from the user. How the user will be able to understand that logging is failing if we don't show an error message? If one is not using AuditLogs, it can be disabled.

[marcstern]

I complete disagree in hiding error messages from the user
You misunderstood my remark.
I said that we should warn the user about the mutex creation failure and then suppress the warning about the deletion when it's a consequence of the creation failure (that was logged).

[marcstern]

I think I understand the reason the creation failure was not logged: it's only because, in modsecurity_init(), we have no access to "server_rec" struct to use ap_log_error().
This seems to me a technical limitation only.