Empty catch blocks in C/C++ ruleset

Hi There,
We have come across the same issue with empty catch blocks in C++ as per the one at S108 - Empty catch blocks are not always bad · Issue #1218 · SonarSource/sonar-dotnet · GitHub with C#. It looks like that exception was only defined for C# rule base in SonarQube, and I was wondering how we can get a similar exception for C++ rule base as well. Could you please help me with that?

PS. //NOSONAR worked, but we prefer a solution based on an exception as per the C# case for it to be more informative and not to blindly skip the complete catch block, as well.

Hello @rami013 ,

You are right that for the rule " Exceptions should not be ignored", we don’t have an exception for the catch block when it contains only comments. We require some real exception handling code.

Could you please share with us an example of code where you believe it is perfectly valid to silently ignore an exception in C++ (even with a comment)? As of now, I would be very reluctant to add an exception for such a case.

Please note that if you believe an issue we reported is not relevant in your case, as an alternative to // NOSONAR, you can also mark it is “won’t fix” of “false positive” in the UI of SonarQube/SonarCloud.

try
{
	std::map<std::string, std::string> mapping;
	...
       m_logger.LogTrace("<some text>", func1.c_str(), count);
}
catch (const std::bad_alloc&) // NOSONAR
	{
		// logging here is useless since it allocates memory, which will almost certainly fail
		// if either the std::string constructor throws or the map insertion throws,
		// so really there isn't anything useful that we can do.
		// note that memory exceptions in the logger are handled internally to it.

		// in the (unecessary but illustrative) catch blocks below, we can log, so SONAR doesn't
		// complain, but here, where there is nothing we can really do, we want to enable a comment
		// to say "nothing we can do"
	}
	catch (const std::invalid_argument& arg)
	{
		// this block is here as an example - it isn't necessary due to the code above, but if it were,
		// our coding standard REQUIRES that we use distinct catches for all possible types of exceptions
		// that the code above may throw.  It's here to highlight that the error handling code below
		// the catch blocks makes sense.
		m_logger.LogTrace("invalid argument to something: %s", arg.what());
	}
	catch (const std::exception& ex)
	{
		// this block is also here as an example, for the same reason as above, to illustrate
		// that there may be several catch blocks, and we don't want to repeat the same
		// error handling in each case.
		m_Logger.LogTrace("unexpected exception: %s", ex.what());
	}

Hello @rami013,

I think there are potentially two things you can do in this catch (std::bad_alloc&) that would be useful (both could be useful simultaneously):

  • If you really want to log, a classical scheme for bad_alloc is to preallocate some memory (maybe at program start), and the in the bad_alloc catch clause, you start by freeing this block and then log.
  • Anyways, there is a serious issue with your program, since you are running out of memory… I don’t believe that swallowing the exception and ignoring it is the right way to go. The minimum that should be done is escalating the problem to higher levels of the application, where it might be handled (maybe because at this higher level, some memory has been freed, thank to the aborted operation). If I wrote this code, the catch clause would probably contain a throw; statement to propagate the exception.
    Additionally, the other catch clauses would contain it too, because they do not really handle the related exceptions, they just log it.

Hi guys,

Rami has been attempting to make the request on my behalf. I’m joining to hopefully help clarify the issue.

The code snippet posted needs a bit of context. We have an application that in written in C# that calls into registered callbacks functions provided in a C++ shared library (dll). These callbacks must follow a particular signature (short of a very big refactoring). The context is as follows:

The signature provides for returning a boolean and an output parameter. We cannot allow exceptions to cross the callback boundary because we’ll be transiting both a shared library boundary (which may have linked to it a different C++ run time - don’t ask) as well as a language boundary (C++ into managed C++ into C#). Thus, we have to catch all expected exceptions. In this case, std::bad_alloc is an expected exception. The others above are provided as examples.

The code below the exception handlers is common and does the required cleanup. The exception blocks are there only to satisfy our mandatory coding requirements wherein, if we catch exceptions, each expected exception must be explicitly caught. It makes not sense to replicate the common cleanup code in each of the catch blocks. The general pattern we’ve agreed upon is as follows:

bool mycallback(/*input and output parameters */)
{
bool success = false;
// any variables that may need cleanup later
try
{
// do stuff that may throw exceptions
success = true;
}
catch (exception1)
{
// log in each catch block
}
catch (exception2)
{
// log in each catch block
}

// do common required cleanup based on above variables

return success;
}

The calling program doesn’t care why the function failed. In this case, if a bad_alloc happens, we have nothing useful that we can do based on the calling requirements & expectations.

Note that if the allocation failure happens in a callback, and we suppress it, then it will happen later in the main code where we can handle it gracefully. The restriction is that this function must meet callback signatures and functional expectations.

Also, the rule to allow a comment in a catch block to satisfy SonarQube is present for C#. Why is it not acceptable for C++ as well? We’d prefer to use a comment to explain what’s going on rather than a // NOSONAR comment that doesn’t convey any information.

Please note that we’ve never seen the bad_alloc exception occur for any reason, either in a callback or in the rest of the codebase (we have millions of installations that have been running for years and the processes are fairly long-lived). Its explicit presence is an artifact of the need to not allow exception propagation combined with our coding standard. So please don’t infer from the fact that the catch is there to mean that the exception actually happens during execution.

The suggestion to deallocate a pre-allocated chunk of memory on allocation failure only postpones the problem. The only place where it would make sense to use it is in the logger itself to ensure that a log line is output. We’re open to that, but it doesn’t actually solve the allocation problem (especially when threads are involved, there is no guarantee of who gets the freed memory). It would allow us to bypass this particular issue because we could then log in the bad_alloc catch, but we do have cases where we can’t use loggers as well. We’re still stuck using // NOSONAR for those cases.

1 Like

Hello @jdaviss,

Thank you for your detailed explanation.

Yes, the purpose of this suggestion was only to allow logging in case of low memory conditions.

Because those two languages have some differences, and exceptions are not used in exactly the same cases in C++ and in C#. And also probably because people working on different language analyzers have different sensibilities. I’ve seen so many code where a swallowed exception led to a program behaving badly, and maybe corrupting data, that I consider this a high risk problem.

That being said, I understand your case, and I think there are ways to handle it more nicely than with // NOSONAR, for instance:

  • Personally, I would write the code relying exclusively on RAII to do the required cleanup based on above variables phase. Which means the catch clause could be written:

      catch(ExceptinIDontWantToLog&) {
        return false;
      }
    

    I understand this might require quite some changes throughout the code, so it’s probably not really applicable in practice in your case.

  • I usually prefer code over comments, especially when something is going to be repeated and becomes a pattern in the code. Therefore, I could do something like the following:

    void reportLowMemoryConditions() {
    // There is nothing we can do currently, because of this or that reason
    }
    
    void f() {
      try { doStuff() }
      catch(ExceptionIDontWantToLog&) {
        reportLowMemoryConditions();
      }
    }
    

    This allows many positive things I think:

    • It documents the intended behaviour cleanly, and allows to put a long comment without duplicating it,
    • The “comment” inside the catch clause will be spell-checked by the compiler,
    • It’s now possible to change the behaviour of reporting in a more centralized way, if it ever becomes required,
    • Our analyzers will stop reporting this case :slight_smile:
    • It allows to set a unique breakpoint on the reportLowMemoryConditions, which could be interesting for some debugging scenarios.

By the way, have you ever heard of the pattern named Lippincott functions? If you have many functions in your code that must log and swallow a bunch of exceptions, this allows to factor-out this exception handling code in just one place.

Hope this helps!

Thank you for the feedback. Unfortunately, the only case that makes sense for us here is the empty function call in the catch block, which doesn’t really add any value. The Lippincott function is a very clever idea, but won’t pass our requirements (we have dumb scanners). The RAII approach is fine, but we still have the empty catch block to deal with.

For now, we’ll go with just making the scanner happy without adding value.