Software Contracts, Part 9: Annotations Outside the Compiler – More Runtime Enforced Annotations


Ok, if it's not become crystal clear that I'm writing this on-the-cuff, this post will finally put a nail in the coffin.

My last post discussed runtime enforced annotations and I used the RPC runtime library as an example of a runtime which enforces contractual annotations.

Of course I totally ignored the single most common example of a runtime enforced contractual annotation in favor of a relatively obscure (but near to my heart) example.

 

Of course, the most common example of a runtime enforced annotation is our old friend ASSERT (and it's brethren).  Assertions are typically used to ensure that invariants are in fact maintained, and a function's contract is always invariant.  Thus assertions can (and IMHO should) be used to enforce contracts. 

Remember my "what's wrong with this code" from yesterday?

:
HRESULT DoSomething(handle_t BindingHandle, const wchar_t *StringParam, int *ReturnedValue)
{
   if (StringParam == NULL || ReturnedValue == NULL)
   {
      return E_POINTER;
   }
   <DoSomething>
}
:

I asserted that the error check was incorrect because the RPC contract ensured that the StringParam and ReturnedValue parameters wouldn't be null.  Norman Diamond called me on it because it was possible that the function would be called incorrectly from inside the module.  Ultimately, he's right, there SHOULD be a check.  But it doesn't deserve to be a full-on check which returns an error, so the check ends up being just dead code.

Dead code is bad for two reasons:

First off, dead code can conceivably be used as an attack vehicle - because it's never been executed, it's never been tested, and a attacker might be able to find a way to use that dead code to exploit some vulnerability.

But more importantly, dead code increases the number of code paths through your system.  One way of measuring the effectiveness of your test strategy is to measure the number of code paths that are executed by your test suites - in theory, the more code paths executed by the tests, the higher the quality of the tests.  But if you have dead code (which thus cannot be executed by the tests), the dead code reduces the code coverage numbers for your tests, leading you to believe that (on this metric) your tests are worse than they actually are.

It's far better to enforce the contract for a situation like this with an assertion.

:

HRESULT DoSomething(handle_t BindingHandle, const wchar_t *StringParam, int *ReturnedValue)
{
   _ASSERT(StringParam != NULL && ReturnedValue != NULL);
   <DoSomething>
}
:

This is a far better implementation - it uses a runtime assertion to enforce the contract, but it doesn't increase the number of code paths.

One of the interesting aspects of assertions as a mechanism for runtime contract enforcement is that they normally aren't checked in all cases.  Instead the assertion is usually present only on debug builds.  Thus assertions don't affect the performance of (and number of code branches in) retail builds.

 

Next: Other kinds of annotations used to allow for contract enforcement.

Comments (9)

  1. Anonymous says:

    Assert that disappears in non-debug build is one of those idiotic ideas that seem nice at the first glance. What is the point of removing the check from the actual software your customers are going to use?? Do you have a miraculous way of testing *every* possibility in debug build?  If you don’t, use an assertion facility that is always there. Now I am sure somebody will mention "performance". Which of course doesn’t matter in 99.9% of cases where you need to assert.

  2. Marvin, I have mixed feelings.  If a customer ever hits the assert, then the assert is by definition bogus (asserts test invariants – if a customer hits it, then it’s not an invariant).

    Asserts don’t only take cycles – they take up space – the debug version of Vista is significantly larger than the non debug version.  It also boots about 10x slower.  So it DOES make a HUGE difference.

    Also, asserts are unactionable – the customer can do absolutely nothing with the assert other than complain about the vendors crappy code.

    Let me give you an example.  One of the multimedia functions is responsible for ensuring that we have a mapping between every error code returned by the current audio subsystem (WASAPI) and the legacy MME APIs.  It has an assertion that fires whenever a WASAPI error is returned that’s not mapped to  MME error.  In the retail builds, it returns a generic "E_FAIL" style error, in the debug builds, it breaks in.

    The assert is invaluable in testing (because it lets us know when there are errors returned that we’ve not yet encountered).  But it’s useless for our customers (the worst case on the assert is that the wrong error code is returned).

  3. Anonymous says:

    If you’re worried about dead code being used as an attack vector, wouldn’t code with no sanity checks be even worse?

  4. Anonymous says:

    I think I wasn’t clear enough and you misunderstood what I meant. All I am saying is: if you think a certain place warrants an assert also make sure that you have some kind of check in the release mode. Your way of returning E_FAIL is perfectly ok. What is not ok is having only the assert and nothing in release mode.

    With regards on assert being bogus if customer hits it I think there is some misunderstanding of terms here too. If a piece of code has a pre-condition, post-condition or invariant then a violation of it is MS bug, not a bogus thing. The violation may be detected by MS testing or by customer running the code but it is still a bug. If you remove the check in released version and the customer performs the sequence of actions that triggers the bug then your code is going to break further down the line. Having the check simply helps you to diagnose the problem earlier and saves MS an embarrassment of breaking horribly somewhere else.

    Regarding performance did you actually check *which* of the tens of thousands of checks in the checked build have an impact on perceivable performance and which don’t. I guess that majority don’t. No doubt there are a few special places that you need to squeeze every bit of performance from but they should be an insignificant drop in the ocean of Windows code 😉

  5. Mike Dunn says:

    Asserts are often misunderstood and get the reaction that Marvin gave. Asserts are not used for error-checking, they are used for *finding bugs*.  The line

    ASSERT(StringParam != NULL);

    does not mean "it’s an error condition if StringParam is null" it means "it is a bug if StringParam is null". The debug build is used for finding bugs, so asserts are only present in debug builds. As Larry said, a failed assert in code running on a customer’s machine is useless anyway – what can the user do about it?

    "Writing Solid Code" has a whole chapter on asserts and why/when to use them.

  6. josh,

     Not really – code with no sanity checks where every code path has been executed is likely to be more correct than code with lots of checks that has never been executed.

  7. Anonymous says:

    > If a customer ever hits the assert, then the assert is by

    > definition bogus (asserts test invariants – if a customer hits

    > it, then it’s not an invariant).

    Exactly.  If the actual behaviour doesn’t match the intended behaviour, then some assumed relationship between the shipping product and the design is bogus.  This is exactly why asserts need to be verifys, and I wish the standard had verifys.

    > the debug version of Vista is significantly larger than the non

    > debug version.  It also boots about 10x slower.

    Because of parameter checking?  Or because of construction of tons of strings to log details in the event log?

    > the customer can do absolutely nothing with the assert other

    > than complain about the vendors crappy code.

    Only a few idiotic customers bother complaining, but the silent majority see the same results of the same crappy code.  The only difference that verifys would make is that when customers see it, you’d see what they see.  Then you could ask them to install debug builds to get a ton of logged events, or you could ask for details of their configuration so you could try to reproduce it in your lab.

    The first time I was in the middle of typing this response, Internet Explorer 6 crashed.  It didn’t offer to send a dump to Microsoft.  Internet Explorer 7 crashed so many times that I had to just uninstall it.  Most of the time IE7 didn’t offer to send crash dumps to Microsoft either.  I don’t think you worked on Internet Explorer, I only think you share your colleagues’ views on whether you should arrange to become informed of what your customers see.  (This paragraph concerns Microsoft’s products for Windows XP Pro Japanese with SP2 and all other available updates.)

    Anyway, the deletion of asserts sure doesn’t stop customers from seeing the results.

  8. Anonymous says:

    Mike Dunn, obviously in the code you write bugs miraculously disappear once you ship the release build to the customer. Gosh. Why is it so hard to understand? You cannot have a check that only works on your dev or QA box. You don’t have to use ASSERT but you need to check.

    ASSERT(p != 0);

    if (p == 0)

      return E_FAIL;

    is fine. Just

    ASSERT( p != 0);

    is NOT.

    But then if you need to check anyway why bother with ASSERT at all. Just check and do something sensible. (Modulo tiny minority of cases where the check interferes with performance)

    Re "what is the user going to do about it", again, we seem to live in different universes. What is user going to do about a crash that will follow if you didn’t check? The user is screwed no matter what because there is a BUG in YOUR code. The only question is how soon it is discovered, at the line where ASSERT was or somehwere down the line once you have corrupted user’s files.

    I suggest you throw away "Writing Solid Code" and read something more useful.

  9. Anonymous says:

    I usually write code of the pattern:

    if (foo == NULL) {

     assert(!"foo is NULL.  wtf did that?!?");

     return false;

    }

    // .. work with foo

    return true;

    Or similarly returning some sensible default in the case that shouldn’t happen, rather than just carrying on and doing something daft.  This way the debug code gets a visible error message so that the bug can be fixed easier, and the release code will do the best it can (and won’t crash unless it really really has to).

Skip to main content