Allow goto in switch statements nested in while loops without reporting S907

Clean Code Report False-positive / False-negative...
1

Template for a good false-positive report, formatted with Markdown:

  • versions used (SonarQube, Scanner, language analyzer) I use SecurityCodeScan (3.5.3) and SonarAlayzer.CSharp (8.17.0.26580) with the .NET 5.0.102 SDK (I also use other analyzers by default like IDisposableAnalyzers, the one now included with the .NET 5 SDK too, as well as stylecop to help with things). Sometimes I do have to suppressing some things but I do not like suppressing too much.
  • minimal code sample to reproduce (with analysis parameter, and potential instructions to compile).
    A minimal sample is to clone https://github.com/Elskom/Sdk/ recursively then open the solution in vs, then under zlib.managed take a look at these 3 files where it reports said warning: InfBlocks.cs, InfCodes.cs, and Inflate.cs.

Changing the goto’s in the switch to let’s say continue breaks the code as then it gets tied to the while loop, which is not intended at all, likewise putting break in each case breaks zlib compression implementation as well. I been trying to better version of the zlib.net library originally from componentace due to the fact it gone unmaintained for years and want to eventually bring it up to date with the latest C version of 1.2.11 with a ton of bug fixes and Intel based optimizations to things internally inside of zlib itself.

Using git to clone the sources I use is far better than sending an actual code block as it goes to show sometimes the rules in the analyzer can break code that exists (even the specs required to implement for deflate/inflate). While I know it can be shorted up to reproduce for your side, I still think it’s better to just look at those files in zlib as they never go over 2k lines of code each so they should do just fine.

Besides if you look in Inflate.cs from line 85~333 it should basically tell you everything I am saying.

Could it be that I might be overreacting and could be wrong with this? possibly, but some of the code contains a switch within a switch within a while loop too.

Also likewise could there be something added to report possible insecure vulnerabilites with possible packages or referenced assembly versions that could be for example “possible DDoS attack in System.Net.Http, etc” when it sees vulnerable versions of their respective packages, etc referenced (and possibly a way to interface it with the github security tab that would be a bonus on all of this as well too).

Also I was told by someone on discord that using goto’s in switches is ok because then it makes it into a form of state machine.

What could be done is to check for goto default; and goto case inside of switches then do not report the goto statements there.

2

Also reported in Rule RSPEC-907 reports for goto's used in switch statements to provide a state machine. · Issue #3996 · SonarSource/sonar-dotnet · GitHub with a code snippet. Original answer:

Hi @AraHaan

Thank you for the code snippet. This goto usage is basically the problem that the rule is trying to prevent from. You have a state machine, that’s not driven by it’s state but manual jumps. And these jumps do not ensure that they jump to appropriate state. You can have something like this as a copy/paste mistake:

this.Mode = LEN;
goto case DIST;

If you replace all your goto statements with break; the while(true) will take care of the state and the state machine will be driven by it’s state without a risk of error.

It’s obvious from your description that the code is really hard to maintain and needs major refactoring to make it readable and maintainable. So the rule is actually useful and preventing developers from such practice and code patterns.