Fixed shared files deadlock in a multi-threaded Windows application

[eduar-hte]

what

Fix potential deadlock in multi-threaded usage of the library on Windows.

why

The shared files Windows implementation introduced in PR #3132 works in multi-process single-threaded contexts but it doesn't work correctly in single-process multi-threaded contexts.

miscellaneous

The issue is that the LockFileEx Win32 function works on a per-handle basis. In a multi-process context, each process will have called SharedFiles::add_new_handler when initializing the SharedFile and obtained a handle, and thus locking will work.

When running ModSecurity in a single process using multiple threads, the initialization of the SharedFile will happen once and the handle will be shared by all threads. Then, if two threads try to write to the same shared file concurrently, they may deadlock as one of them will lock the file (by calling LockFileEx) and then proceed to write to the file. If before writing to the file and unlocking it, another thread calls LockFileEx on the same handle, the attempt to write to the file will lock generating a deadlock.

The new implementation replaces usage of LockFileEx/UnlockFileEx with a named mutex to lock access to the shared file.

• A named mutex is used to support multi-process scenarios.
  • The mutex name is generated using the filename to support multiple shared files (such as that for the debug and audit logs).
• This assumes that both process will initialize the SharedFile instance using the same filename (which is expected as they'd be using the same configuration file)

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[eduar-hte]

An alternative fix still using relying on LockFileEx/UnlockFileEx was evaluated, which would have required opening and closing the file whenever the shared file is written to (in each call to SharedFiles::write) in order for each thread to have a different handle to try and lock the file, as expected by the Win32 APIs.

However, this implementation was discarded for performance reasons (in comparison to the one using the mutex) and to minimize differences between the implementation on different platforms.

[airween]

Hi @eduar-hte,

thanks again for this great PR!

The issue is that the LockFileEx Win32 function works on a per-handle basis. In a multi-process context, each process will have called SharedFiles::add_new_handler when initializing the SharedFile and obtained a handle, and thus locking will work.

When running ModSecurity in a single process using multiple threads, the initialization of the SharedFile will happen once and the handle will be shared by all threads.

Seems you have fixed this issue only if the platform is WIN32, right?

Don't you think this could happen on other platforms too? (I never used libmodsecurity3 on MT environment so I don't have any experiences, but there are some other reported issues: #3138, #3054)

Have ever you faced other problems in MT environment? (See mentioned issues above)

[eduar-hte]

Seems you have fixed this issue only if the platform is WIN32, right?

Yes, this issue was introduced in my implementation of the Windows port. 🤦

Don't you think this could happen on other platforms too?

I'm not sure whether a deadlock will be triggered in a MT environment, but I understand that locking using F_SETLKW is not suitable in that context, as the documentation states:

The threads in a process share locks.  In other words, a
multithreaded program can't use record locking to ensure that
threads don't simultaneously access the same region of a file.

If you look at the history of the SharedFiles implementation, it was previously using a mutex that was shared across shared-memory between processes. This was replaced with the lock fcntl in commit 3d20304.

(I never used libmodsecurity3 on MT environment so I don't have any experiences, but there are some other reported issues: #3138, #3054)

Have ever you faced other problems in MT environment? (See mentioned issues above)

I'm currently working through the integration of libModSecurity in a MT web server on Windows, and have only found this issue (I introduced) at this point.

With regards to #3138, I haven't run into that issue because initialization of rules happens only once and in a single thread before the ModSecurity instance is used to analyze requests.

NOTE: I think it's reasonable for the library to expect configuration changes to be synchronized, while the critical scenario should be to be able to handle transactions concurrently without synchronization for the sake of performance.

I'll keep an eye on the issue mentioned in #3054, thanks for the heads up.

I'd guess that we could say that MT support is at this point not guaranteed (or experimental).

[airween]

Yes, this issue was introduced in my implementation of the Windows port. 🤦

thanks 😃

I'm not sure whether a deadlock will be triggered in a MT environment, but I understand that locking using F_SETLKW is not suitable in that context, as the documentation states:

The threads in a process share locks.  In other words, a
multithreaded program can't use record locking to ensure that
threads don't simultaneously access the same region of a file.

If you look at the history of the SharedFiles implementation, it was previously using a mutex that was shared across shared-memory between processes. This was replaced with the lock fcntl in commit 3d20304.

"Great". So we can state that the library cannot be used in MT environment.

(I never used libmodsecurity3 on MT environment so I don't have any experiences, but there are some other reported issues: #3138, #3054)
Have ever you faced other problems in MT environment? (See mentioned issues above)

I'm currently working through the integration of libModSecurity in a MT web server on Windows, and have only found this issue (I introduced) at this point.

Thanks - a good news at least.

With regards to #3138, I haven't run into that issue because initialization of rules happens only once and in a single thread before the ModSecurity instance is used to analyze requests.

Yes, this is what I suggested the reporter.

I'll keep an eye on the issue mentioned in #3054, thanks for the heads up.

Thanks. I created a new directory with full MT under examples, but I wasn't able to reproduce the mentioned issue. (I suspended the work on that issue and haven't added the example yet.)

I'd guess that we could say that MT support is at this point not guaranteed (or experimental).

Yes, at least.

Thanks again for this great PR, I'm going to merge that.

[eduar-hte]

Thanks. I created a new directory with full MT under examples, but I wasn't able to reproduce the mentioned issue. (I suspended the work on that issue and haven't added the example yet.)

There is already an example that shows some multithreading support in the library, which is reading_logs_via_rule_message. This example launches 100 threads to create and process 10000 transactions concurrently. The update for the example to be multithreaded was done back in 2017 in commit 2a50852.

When I was initially getting to know the library and working on the Windows port, I went through all the examples in order to validate the port, and based on this and some other references in the code to multithreading I assumed it was supported.

I'd guess that we could say that MT support is at this point not guaranteed (or experimental).

Yes, at least.

I think it makes sense to create an issue to keep track of MT support and link to this PR and the issues that you mentioned before. I'll write that up later.

[eduar-hte]

I think it makes sense to create an issue to keep track of MT support and link to this PR and the issues that you mentioned before. I'll write that up later.

#3215

[eduar-hte]

I'll keep an eye on the issue mentioned in #3054, thanks for the heads up.

For the sake of reference, trying to address this in PR #3216.