Share via

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

  • Article

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

  • Anonymous
    October 19, 2006
    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.

  • Anonymous
    October 19, 2006
    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.

  • Anonymous
    October 20, 2006
    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.

  • Anonymous
    October 20, 2006
    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);

  • Anonymous
    October 20, 2006
    The comment has been removed

  • Anonymous
    October 20, 2006
    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

  • Anonymous
    November 01, 2006
    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

  • Anonymous
    November 02, 2006
    The comment has been removed

  • Anonymous
    November 02, 2006
    The comment has been removed

  • Anonymous
    November 02, 2006
    The comment has been removed

  • Anonymous
    November 02, 2006
    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

  • Anonymous
    November 03, 2006
    The comment has been removed

  • Anonymous
    November 03, 2006
    The comment has been removed