Fix S2583 FP: nested try catch blocks

[bart-vmware]

Description

We've found cases in our codebase where Sonar incorrectly suggests replacing a condition with true, claiming that the code is unreachable, which is incorrect.

Repro steps

1. dotnet new console -f net6.0
2. dotnet add package SonarAnalyzer.CSharp
3. Paste the code below in Program.cs:

#define TOGGLE

using System.Data;

List<Exception>? failures = null;

try
{
    try
    {
        DoThrow(new InvalidOperationException());
    }
    catch (InvalidOperationException)
    {
        try
        {
            DoThrow(new TimeoutException());
        }
        catch (TimeoutException)
        {
            // Intentionally left empty.
        }
    }
}
catch (DataException exception)
{
    failures ??= new List<Exception>();
    failures.Add(exception);
}

if (failures != null) // S2583: Conditionally executed code should be reachable
{
    Console.WriteLine("Found failures.");
}

static void DoThrow(Exception? exception)
{
    if (exception != null)
    {
#if TOGGLE
        throw new DataException();
#else
        throw exception;
#endif
    }

    Console.WriteLine();
}

Running this program prints:

Found failures.

Now comment out the first line (#define TOGGLE) and run again. It now prints nothing. This shows there's a difference in behavior, ie the condition in the marked if statement is not redundant.

Changing the if line to if (true), as suggested by the rule description, changes the output to always print:

Found failures.

irrespective of the toggle. Therefore the Sonar warning is incorrect.

Expected behavior

No warning, because the code is reachable.

Actual behavior

Warning that suggests breaking the program behavior.

Related information

• C#/VB.NET Plugins version: N/A
• Visual Studio version: v17.4.4
• MSBuild / dotnet version: .NET 6, SDK v7.0.401
• SonarScanner for .NET version: v9.11
• Operating System: Windows

[antonioaversa]

Hi @bart-vmware, thanks for reporting the issue and the detailed reproduction steps.

I reproduce the warning on failures != null, and I confirm it is False Positive. As explained in my comment on the other issue on rule S2583, this rule requires symbolic execution.

In the specific case, there are two issues with our analysis:

1. we currently don't support interprocedural execution, so we don't run the execution across different methods
2. we currently don't support conditional flows based on #define and #if

I am going to add a reproducer for this specific scenario in our code base, and I am adding this issue to our backlog, to be included in the planning of the hardening sprint on rules S2583 and S2589, which should happen later this year.

As for the other issue you have been experiencing with rule S2583, you can:

• you can mark the issue as a false-positive in the UI of SonarQube or SonarCloud, if you are using SonarLint in connected mode. The rule can also be disabled in your Quality Profile by an administrator if it gets particularly noisy.
• you can use #pragma warning disable and #pragma warning restore to locally disable the rule in the source, if you are using SonarLint in Standalone Mode. You can also use the .editorconfig files to disable or change the severity of the rule if it gets particularly noisy.

[bart-vmware]

1. we currently don't support interprocedural execution, so we don't run the execution across different methods

That's what I would expect. But how is that relevant here? I intentionally extracted DoThrow into a separate method to prevent it from influencing analysis. DoThrow might throw or not, which are the two branches to consider during analysis. Whether it actually does is irrelevant to the analysis this rule performs.

2. we currently don't support conditional flows based on #define and #if

I only added the toggle to make the reproduction steps easier to perform. You can delete them and modify the code in-place, which still produces the bug. But again, the toggle only affects the implementation of DoThrow, which should not be taken into account.

[antonioaversa]

@bart-vmware Indeed, I can reproduce even without using the preprocessor directives for conditional compilation.

Regarding interprocedural execution: while we don't run symbolic execution across methods, for try catch scenarios we actually perform some analyses of methods invoked, at the syntactical level only. However, such analyses are not working correctly in the case of nested try catch blocks.

For example, I can reproduce the False Positive with just two nested try catch blocks:

using System.Data;

List<Exception> failures = null;

try
{
    try
    {
        ThrowMaybe();
    }
    catch (InvalidOperationException)
    {
        // Do nothing
    }
}
catch (DataException)
{
    failures = new List<Exception>();
}

if (failures != null) // S2583: Conditionally executed code should be reachable
{
    Console.WriteLine("Found failures.");
}

void ThrowMaybe() { }

But I can't with a multi-catch statement:

using System.Data;

List<Exception> failures = null;

try
{
    ThrowMaybe();
}
catch (InvalidOperationException)
{
    // Do nothing
}
catch (DataException)
{
    failures = new List<Exception>();
}

if (failures != null) // No report in this case
{
    Console.WriteLine("Found failures.");
}

void ThrowMaybe() { }

And this, as you pointed out, regardless of whether ThrowMaybe is empty or throws a DataException.

DoThrow might throw or not, which are the two branches to consider during analysis. Whether it actually does is irrelevant to the analysis this rule performs.

The implementation of DoThrow always throws an exception: it's a matter of which type of exception is thrown, as this affects the flow. So I will change the title of the issue, mentioning nested try-catch blocks, and add a reproducer for that instead.

A deeper analysis to identify and fix the root cause of the issue will be run during the hardening sprint on this rule.

[bart-vmware]

Thanks for the explanation. I'm surprised that the analyzer looks into the implementation of called methods. That sounds pretty expensive and unreliable. I've never heard of a compiler doing that.

[Tim-Pohlmann]

After looking at this again: We do not look into the implementation of called methods. I apologize for the confusion. The issue is related to how the SE engine handles exception flows, which seems not to work correctly in this case. This needs a more profound investigation at some point.

[bart-vmware]

Please prioritize fixing this, it is a very dangerous bug.