0

I am writing a reference-counted linked list of characters data structure in C for practice. I want to try using Sal in it to annotate function parameters for this practice.

I have an input paremeter(named This), which I want to annotate to make it clear that the specified parameter's members must be mutable in order for the function to behave as expected.

The situation is analogous to the code below.

#include <Windows.h>

typedef struct Box {
    ULONG val;
} Box;

ULONG Box_decrement(_In_ Box *This) {
    return InterlockedDecrement(&(This->val));
}

int main(int argc, char **argv) {
    Box b = {2};
    Box_decrement(&b);

    return (BYTE)b.val;
};

Is there an existing Sal annotation that can be used to annotate the This parameter of the Box_increment function to make it clear from the function signature that the function modifies one or more members of the Box that has been passed to it?

Something like _InternallyMutable_(but exist):

#include <Windows.h>

typedef struct Box {
    ULONG val;
} Box;

ULONG Box_decrement(_InternallyMutable_ _In_ Box *This) {
    return InterlockedDecrement(&(This->val));
}

int main(int argc, char **argv) {
    Box b = {2};
    Box_decrement(&b);

    return (BYTE)b.val;
};

Best solution so far(unfortunately, there does not seem to be any equivelent in SAL to denote Internally_mutable, there is Unchanged which is the opposite):

#include <Windows.h>

#define _Internally_mutable_(expr) _At_(expr, _Out_range_(!=, _Old_(expr)))

typedef struct Box {
    ULONG val;
} Box;

ULONG Box_decrement(_In_ _InternallyMutable_(This) Box *This) {
    return InterlockedDecrement(&(This->val));
}

int main(int argc, char **argv) {
    Box b = {2};
    Box_decrement(&b);

    return (BYTE)b.val;
};
Dmytro
  • 5,068
  • 4
  • 39
  • 50

1 Answers1

1

Yes! You can. SAL is a wonderful DSL that lets you do basically anything you want if you're psychic enough to infer it from the little bits in the Windows SDK. I've even in the past been able to write super simple custom annotations to detect invalid HANDLE usage with _Post_satisfies_ and friends.

This code seems to work:

_At_(value, _Out_range_(!=, _Old_(value)))
void change_value_supposed_to(int& value) noexcept {
    //value += 1;
}

...Running with all native rules in code analysis, I get a warning like this:

Warning C28196  The requirement that '_Param_(1)!=(("pre"), _Param_(1))' is not satisfied. (The expression does not evaluate to true.)

(there, substitute value with your variable) For _Internally_mutable_, I can do it in the "above the function" style of SAL:

#define _Internally_mutable_(expr) _At_(expr, _Out_range_(!=, _Old_(expr)))
_Internally_mutable_(value)
void change_value_supposed_to_internally_mutable(int& value) noexcept {
    //value += 1;
    (void)value;
}

...but not inline WITHOUT being repetitive, as you wanted. Not sure why right now - _Curr_ doesn't seem to be working? - I may need another layer of indirection or something. Here's what it looks like:

#define _Internally_mutable_inline_(value) _Out_range_(!=, _Old_(value))
void change_value_supposed_to_internally_mutable_inline(_Internally_mutable_inline_(value) int& value) noexcept {
    //value += 1;
    (void)value;
}

How I figured this out:

sal.h defines an _Unchanged_ annotation (despite doing web dev for several years now and little C++, I remembered this when I saw your question in a google alert for SAL!):

// annotation to express that a value (usually a field of a mutable class)
// is not changed by a function call
#define _Unchanged_(e)              _SAL2_Source_(_Unchanged_, (e), _At_(e, _Post_equal_to_(_Old_(e)) _Const_))

...if you look at this macro closely, you'll see that it just substitutes as:

_At_(e, _Post_equal_to_(_Old_(e)) _Const_)

...and further unrolling it, you'll see _Post_equal_to_ is:

#define _Post_equal_to_(expr)       _SAL2_Source_(_Post_equal_to_, (expr), _Out_range_(==, expr))

Do you see it? All it's doing is saying the _Out_range_ is equal to the expression you specify. _Out_range_ (and all the other range SAL macros) appear to accept all of the standard C operators. That behavior is not documented, but years of reading through the Windows SDK headers shows me it's intentional! Here, all we need to do is use the not equals operator with the _Old_ intrinsic, and the analyzer's solver should be able to figure it out!

_Unchanged_ itself is broken?

To my great confusion, _Unchanged_ itself seems broken:

_Unchanged_(value)
void change_value_not_supposed_to(_Inout_ int& value) noexcept {
    value += 1;
}

...that produces NO warning. Without the _Inout_, code analysis is convinced that value is uninitialized on function entry. This makes no sense of course, and I'm calling this directly from main in the same file. Twiddling with inlining or link time code generation doesn't seem to help

I've played a lot with it, and various combinations of _Inout_, even _Post_satisfies_. I should file a bug, but I'm already distracted here, I'm supposed to be doing something else right now :)

Link back here if anybody does file a bug. I don't even know what the MSVC/Compiler teams use for bug reporting these days.

Fun facts

5-6 years ago I tried to convince Microsoft to open source the SAL patents! It would have been great, I would have implemented them in Clang, so we'd all be able to use it across platforms! I might have even kicked off a career in static-analysis with it. But alas, they didn't want to do it in the end. Open sourcing them would have meant they might have to support it and/or any extensions the community might have introduced, and I kinda understand why they didn't want that. It's a shame, I love SAL, and so do many others!

Alexander Riccio
  • 358
  • 2
  • 12
  • Thanks for the answer! Let me see if I understand you correctly. There is no _Internally_mutable_(This) in SAL, but there is the opposite, _Unchanged_(This) which is flawed, and you are showing how to implement a macro to properly implement _Internally_mutable_(This)? I ask because the main question here is whether there exists an existing annotation to do what I want without extending Sal(although it's nice to know it's possible to extend in such a way), even if warnings do not work(as it is meant to be a human annotation mostly) – Dmytro Jul 24 '22 at 22:00
  • 1
    Essentially, yes, you're correct. There's no `_Changed_` annotation defined in the `` headers. I wouldn't look at this as really *extending* SAL per se - SAL as **we** use it is just a bunch of macros that map to lower level implementations that mean something to the analyzer. Many of the interesting annotations that Microsoft provides are also macros that expand to more complex combinations of lower level annotations! What you would usually do is create a new header that defines this, and have a guard for platforms that have no SAL, that defines them to be empty. – Alexander Riccio Jul 26 '22 at 19:52
  • Here's an example from `specstrings.h` in the `shared` folder of the Windows 10 sdk: `#define _Reallocation_function_(after, before, size) \ _Success_((after) != NULL || (size) == 0) \ _At_((after), _Post_maybenull_ _Post_writable_byte_size_(size) \ _When_(((before) == NULL || (size) > 0), _Must_inspect_result_)) \ _At_((before), _Post_ptr_invalid_ __drv_freesMem(Mem))` – Alexander Riccio Jul 26 '22 at 20:15
  • 1
    That didn't render too well, so see this link: https://github.com/tpn/winsdk-10/blob/master/Include/10.0.16299.0/shared/specstrings.h#L505 I believe those types of functions are used in the DDK, but I don't have it installed right now. I have a bunch of totally unstructured from a few years ago but I don't think I have anything from the DDK there. See: https://github.com/ariccio/SALExamples/blob/master/SALexamples.txt – Alexander Riccio Jul 26 '22 at 20:25