Fix database mssql mode deadlocks

[rmorandell-pgum]

Community contributors

Description

According to my analysis, the mode for database::mssql::mode::lockswaits and database::mssql::mode::deadlocks is incorrectly designed.

Here is the Microsoft documentation about this performance counters.

1. Deadlocks cannot be filtered at the database level using performance counters. The instance here is not the DB instance or the database, but a lock type.

In my case the query of the plugin executed on the MSSQL Server looks like this.

There is always a “_Total”. So, the filter should be –filter-instance instead of –filter-database and the default value should be “_Total” which is the total of all other single instances.

2. The counter “Lock Waits/sec” isn’t a real time counter. You would expect the value to represent “per second”, but instead the number keeps increasing continuously. Although the counter is named Lock Waits/sec, the value returned in sys.dm_os_performance_counters is a cumulative raw counter, not an already calculated per-second rate. cntr_value stores the total number of lock waits since SQL Server was last started. It continues to increase over time. It resets only when the SQL Server service restarts.

This gives us the difference from the last measurement.

Type of change

• Patch fixing an issue (non-breaking change)
• New functionality (non-breaking change)
• Functionality enhancement or optimization (non-breaking change)
• Breaking change (patch or feature) that might cause side effects breaking part of the Software

How this pull request can be tested ?

Checklist

• I have followed the coding style guidelines provided by Centreon
• I have commented my code, especially hard-to-understand areas of the PR.
• I have rebased my development branch on the base branch (develop).
• I have provide data or shown output displaying the result of this code in the plugin area concerned.

Summary by Aikido

Security Issues: 0	🔍 Quality Issues: 2	Resolved Issues: 0

⚡ Enhancements

• Replaced --filter-database with --filter-instance defaulting to '_Total'.
• Converted cumulative performance counters to per-second and added totals.
• Enabled statefile caching and generated unique cache names using md5.
• Added verbose debug messages when skipping or reporting performance instances.

^{More info}

[aikido-pr-checks]

User-provided filter_instance is directly interpolated into a regex ($$row[0] !~ /$self->{option_results}->{filter_instance}/), enabling regex injection or ReDoS. Validate or escape (e.g., quotemeta) filter_instance before use.

Details

✨ AI Reasoning
​The code now uses the CLI option filter_instance (defaulting to '_Total' but still user-controlled) directly inside a Perl regex match: $$row[0] !~ /$self->{option_results}->{filter_instance}/. Interpolating unvalidated user input into a regex can lead to regex injection, syntax errors, or ReDoS issues. This was introduced by the PR when replacing filter_database with filter_instance and should be flagged. The problematic expression appears in the newly added filtering logic in both changed files.

🔧 How do I fix it?
Use parameterized queries with placeholders, array-based command execution (no shell interpretation), or properly escaped arguments using vetted libraries. Avoid dynamic queries/commands built with user input concatenation.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info}

[aikido-pr-checks]

User-provided filter_instance is directly interpolated into a regex ($$row[0] !~ /$self->{option_results}->{filter_instance}/), enabling regex injection or ReDoS. Validate or escape (e.g., quotemeta) filter_instance before use.

Details

✨ AI Reasoning
​In lockswaits.pm the PR added the same direct regex interpolation using the CLI-provided filter_instance option: $$row[0] !~ /$self->{option_results}->{filter_instance}/. This introduces the same risk of regex injection or denial-of-service via crafted patterns. The change replaced the previous filter_database usage, so this new usage should be reviewed.

🔧 How do I fix it?
Use parameterized queries with placeholders, array-based command execution (no shell interpretation), or properly escaped arguments using vetted libraries. Avoid dynamic queries/commands built with user input concatenation.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info}

[aikido-pr-checks]

User-provided filter_instance is used directly in a regex (/.../), enabling regex injection or ReDoS. Validate or escape the pattern before using it, or use a safe comparison method.

Details

✨ AI Reasoning
​The deadlocks mode changes mirror the lockswaits changes: option_results.filter_instance is matched via a regex built from user-supplied input without escaping/validation. This can lead to regex injection or performance denial-of-service via catastrophic backtracking. The risk stems from newly added code that uses the raw option value in a regex match.

🔧 How do I fix it?
Use parameterized queries with placeholders, array-based command execution (no shell interpretation), or properly escaped arguments using vetted libraries. Avoid dynamic queries/commands built with user input concatenation.

_{Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info}

[omercier]

Hi @rmorandell-pgum,
Do you agree what is done here?