Using ntintsafe.h is a great idea, but I don’t know how readable the results are


The addition of ntintsafe.h for detecting integer overflow/underflow is a great addition to the WDK. It unifies how everyone detects these math errors, leading to common code that anyone can pickup and see what it does…BUT, I have found it does have a “tax.” What is actually being computed can be become unclear! For instance, let’s take this sample before we convert it to detect any math errors:

    USHORT cbImageNameLength;

cbImageNameLength = ((USHORT) wcslen(wsImageName)) * sizeof(WCHAR) + sizeof(UNICODE_NULL);

It is relatively straightforward and clear (depends on the viewer I guess) that we are computing the number of bytes to wsImageName requires. Here is the safe (and longer!) version of the unsafe computation:
    USHORT cbImageNameLength;
NTSTATUS status;

do {
status = RtlSizeTToUShort(wcslen(wsImageName), &cbImageNameLength);
if (!NT_SUCCESS(status)) {
break;
}

status = RtlUShortMult(cbImageNameLength, sizeof(WCHAR), &cbImageNameLength);
if (!NT_SUCCESS(status)) {
break;
}

status = RtlUShortAdd(cbImageNameLength, sizeof(UNICODE_NULL), &cbImageNameLength);
if (!NT_SUCCESS(status)) {
break;
}

// cbImageNameLength has now been computed…
} while (FALSE);

Needless to say, that is not the most transparent code I have read or written. I certainly would not go back to the unsafe vesion and I cannot use safeint object which throws execptions on error because this code runs in the kernel. The only remedy I could think of is to create a comment at the top of the block which contains the plain version. (This rule is in the KMDF coding style guidelines.) As a result, the final source would look like this:
        //
// cbImageNameLength = ((USHORT) wcslen(wsImageName)) * sizeof(WCHAR) + sizeof(UNICODE_NULL);
//
status = RtlSizeTToUShort(wcslen(wsImageName), &cbImageNameLength);
[…]
I think this is a decent tradeoff between correctness and readability. Another change you could make is to add begin/end comments around the entire computation block to further clarify the scope of which calls belong to which overall computation.

Comments (13)

  1. dispensa says:

    Has this raised any issues with keeping comments in synch with the longer code block that follows? Stale comments are an old problem, but the decrease in transparency in the safe block would seem to intensify the problem.

  2. cnettel says:

    Wouldn’t it be possible to create a "safenonthrowint", which would maintain an error bool, which you could check in the end? Any operation between two values would "OR" the error bit, so if we reached overflow/underflow in any previous step, it will be noted in the final value. If you don’t really care which step failed, and don’t object to using C++ per se, it could be much cleaner.

  3. doronh says:

    a safenothrowint is an interesting idea, but would you have the bool explicitly declared on the stack or part of a class?  if you have a class, then you are moving away from native types (ULONG/USHORT/etc) and into implicit operators and/or templates.  it can get messy.   I would have to see an implementation or spec before i bought into using such a class.

  4. doronh says:

    Steve, yes stale comments become more problematic.  Note that in the simple case, you can still have stale comments if you change the unsafe computation without touching the comments as well.  One thing I tought of after writing this post is to put a comment on each subcompuation indicating what part of the overall computation is being made, e.g.

           // cast length of the string to a USHORT

           status = RtlSizeTToUShort(wcslen(wsImageName), &cbImageNameLength);

           // string length * sizeof(WCHAR) = number of bytes

           status = RtlUShortMult(cbImageNameLength, sizeof(WCHAR), &cbImageNameLength);

           // add sizeof(UNICODE_NULL) to number of bytes

           status = RtlUShortAdd(cbImageNameLength, sizeof(UNICODE_NULL), &cbImageNameLength);

  5. Ray Trent says:

    This is all fine and good and all, but wouldn’t this kind of approach be more readable and work just as well:

    #ifndef MAXUSHORT

    #define MAXUSHORT ((USHORT)~(USHORT)0)

    #endif

    USHORT cbImageNameLength;

    size_t length = wcslen(wsImageName);

    if (length <= MAXUSHORT &&  length * sizeof(WCHAR) + sizeof(UNICODE_NULL) <= MAXUSHORT)

    {

     cbImageNameLenth = length * sizeof(WCHAR) + sizeof(UNICODE_NULL);

    } else {

     // error handling

    }

    Any half-decent compiler will only do the calculation once, BTW, but feel free to use a temp in a nested if. Also, if you feel it necessary in this day and age to assert that sizeof(size_t) > sizeof(USHORT) please feel free.

    If you feel compelled to use the RTL routines, I would at least avoid source code bloat:

    if (NT_SUCCESS(RtlSizeTToUShort(wcslen(wsImageName), &cbImageNameLength)) &&

    NT_SUCCESS(RtlUShortMult(cbImageNameLength, sizeof(WCHAR), &cbImageNameLength)) &&

    NT_SUCCESS(RtlUShortAdd(cbImageNameLength, sizeof(UNICODE_NULL), &cbImageNameLength)))

    {

     // cbImageNameLength = ((USHORT) wcslen(wsImageName)) * sizeof(WCHAR) + sizeof(UNICODE_NULL) successfully calculated here

    }

    The do-while(FALSE)-break paradigm has never done much for me. IMNSHO, this is one of those psuedo-high-level things people made up solely so they wouldn’t have to use goto. I personally find it even less readable.

  6. doronh says:

    Ray, i like your suggestion.  the check against MAXUSHORT works if you have a larger native type in which you can store the partially computed value and then compare against the smaller maximum.  If length were a USHORT, your checks would never detect failure…so if you apply this to a 64 bit unsigned type, you still need to go the Rtl route, which would lead me to just keep things simple and use the Rtl routines all the time so that I never picked too small of a native type size for length.

    I do like the combination of all the Rtl calls into one if().  That style had not occurred to me, I rather like it (except for the loss of the actual NTSTATUS returned so you can propagate it on error, but by looking at ntintsafe.h, you can always return STATUS_INTEGER_OVERFLOW in the error case ;).

    I too personally hate the do { } while (FALSE); construct and using breaks as an informal goto.  I just copied some code that I didn’t write for my example (and admittedly, do{} while is a little more clear in a code snippet vs referencing a label which doesn’t exist)

    d

  7. doronh says:

    I have started combining all of the calls into one if() like Ray suggested and I really like the results.  Thansk for the great suggestion Ray!

    d

  8. mgrier says:

    You could write:

    if (!NT_SUCCESS(status = RtlFoo(…))) break;

    or if you moved to use automatic cleanup code instead of explicit cleanup code:

    if (!NT_SUCCESS(status = RtlFoo(…))) return status;

    Finally, if you don’t have an irrational fear of macros you can write:

    #define IFNOTNTSUCCESS_RETURN(e) do { const NTSTATUS status = (e); if (!NT_SUCCESS(status)) return status; } while (0)

    and then

    IFNOTNTSUCCESS_RETURN(RtlFoo(…));

    I guess some people like writing it all out but it’s so error prone.

  9. doronh says:

    I personally find that returns hidden by a macro are not maintainable nor transparent, so for me at least, IFNOTSUCCESS_RETURN is not an option (although the name does try to be clear about what it is doing being the mask).  I also find that assignments in an if() tend to get lost during a quick code persual or code review, leading to mistakes down the line as well so I avoid that as well.

    If I could only use RIAA objects in code I would :), but being kernel mode, I limit what C++ features I use to a minimum to avoid weird suprises at runtime due to linker/compiler quirks that only expose themselves in KM.

    I think all of these different methods have their place with the right dev/situation, I have certainly picked up some good pointers from this topic.

    d

  10. mgrier says:

    RAII does require exceptions in its strict definition but getting rid of explicit cleanup code is always a good thing.  I don’t recommend using exceptions, ever, so my recommendation to use destructors for cleanup is entirely independent of the use of exceptions.

    w.r.t. hiding control flow, well, then we should get rid of all those "break" and "continue" statements and replace them with gotos since these things are all just irregular syntactic sugar over doing things the hard way.  I hear this issue raised all the time and it really doesn’t hold water IMO.  Once your code becomes clear, direct and imperative you don’t ever want to go back to being so tentative about checking error codes and handling cleanup.

  11. doronh says:

    RAII may require execeptions in its strictest sense, but I just use the desctructor pattern like you mention so that any exit point cleans up the allocations/opens/etc.

    Now you are being a little over the top.  break/continue/continue are a part of the language and anyone who reads the code will know what they do.  By your logic, we should just go to asm to get of any sugar in mix ;).    I have issue with unintended or unapparent side affects.  If all of the code uses IFNOTSUCCESS_RETURN, then its use is more acceptable b/c it is an apparent pattern.  If one developer decided to use that macro, but the rest of project did not, I have issue with that (and personally would not allow such a pattern to be used in KMDF) b/c now its behavior is not expected.

    BTW, I personally hate the do {} while (FALSE) pattern with breaks on error, it is there for people who dogmatically thing goto is a bad idea b/c it was pounded into them through 4 years of a CSc degree.

    d

  12. mgrier says:

    do { /* */ } while (false) isn’t there for goto or anything it’s to make macros act more like functions.  If you just use braces and you write

    if (cond) MACRO(); else { whatever }

    you’ll get a syntax error since you can’t follow a brace with a semicolon before an else.

    Functions and macros extend the language and the language is already extended past its minimal required set of expressiveness so IMO the rest is based on taste and ease of programming.  Like I said once you stop writing "if" statements explicitly on every single line of code, even at the cost of macros, you won’t go back.  It’s similar to the syntactic benefits of exceptions but doesn’t draw in their unsolvable problems.

  13. doronh says:

    I think we’ll leave it as a matter of taste.  I tried the hide my if’s behind a macro and it didn’t work for me.   Ideally you want an environment which you have a bunch of straight line code and when you hit an error, you return the error and things around you cleanup properly. KMDF callbacks work this way.  When building a WDFDEVICE, you just return the error and things get cleanup for you by the framework.

    d