False positives in PVS-Studio: how deep the rabbit hole

    Unicorn PVS-Studio and GetNamedSecurityInfo

    Our team provides fast and efficient customer support. Only programmers take part in support, as programmers also ask questions and we have to think about many of them. I would like to describe one of the recent support calls on the subject of false positives, which led to a whole small study of the problem described in the letter.


    We are working hard to reduce the number of false positives generated by the PVS-Studio analyzer. Unfortunately, static analyzers often cannot distinguish the correct code from the error, since they simply do not have enough information. As a result, there are false positives anyway. However, this is not a problem, since having tuned the analyzer it is easy to achieve a situation where 9 out of 10warnings will indicate true errors.

    Although false alarms are not such a big problem as it might seem at first glance, we are constantly struggling with them, improving diagnostics. We notice some flagrant false positives ourselves, some of which are written to us by customers and free users.

    Recently, one of our clients wrote a letter of approximately the following content: For

    some reason, the analyzer says that the pointer is always null, although this is clearly not so. Moreover, the analyzer behaves strangely and unstably on a test project: it either gives a warning, or it does not. A synthetic example that reproduces a false positive:

    #include 
    #include 
    #include 
    int main()
    {
      PACL pDACL = NULL;
      PSECURITY_DESCRIPTOR pSD = NULL;
      ::GetNamedSecurityInfo(_T("ObjectName"), SE_FILE_OBJECT,
         DACL_SECURITY_INFORMATION, NULL, NULL, &pDACL, NULL, &pSD);
      auto test = pDACL == NULL; // V547 Expression 'pDACL == 0' is always true.
      return 0;
    }

    I can imagine how similar responses look from our users. It’s immediately clear that the GetNamedSecurityInfo function changes the value of the pDACL variable . Could the developers really not be able to write a handler for such simple situations? Moreover, it is not clear why the analyzer either issues a message or not. Maybe they themselves have some kind of bug in the tool, such as an uninitialized variable?

    Eh ... It’s not an easy job to maintain a static code analyzer. But what to do, I myself chose such a fate. Rolling up my sleeves, I proceeded to investigate the cause of the false positive.

    First, I looked at the GetNamedSecurityInfo function description .and made sure that its call should really lead to a change in the value of the pDACL variable . Here is a description of the 6th function argument:
    ppDacl

    A pointer to a variable that receives a pointer to the DACL in the returned security descriptor or NULL if the security descriptor has no DACL. The returned pointer is valid only if you set the DACL_SECURITY_INFORMATION flag. Also, this parameter can be NULL if you do not need the DACL.

    I know that the PVS-Studio analyzer should definitely correctly process such simple code and not give a meaningless warning. Already at this moment, my intuition told me that this would be an unusual case, which will have to spend time.

    I confirmed my fears when I could not reproduce a false positive either on the current alpha-version of the analyzer, or on exactly the version that was installed on the client. So and so, but the analyzer is silent.

    I asked the client to send me a preprocessed i-file generated for a program with a synthetic example. He generated, sent the file, and I started to examine it in detail.

    On the file sent, the analyzer immediately issued a false positive. On the one hand, this is good, since the bug is reproduced. On the other hand, I experienced the feelings that this picture best describes.

    headquarters


    Why exactly these? I know very well how the analyzer and diagnostics of the V547 work . Well, there can be no such actuation!

    Ok, make tea and continue.

    The call to the GetNamedSecurityInfo function expands to:

    ::GetNamedSecurityInfoW(L"ObjectName", SE_FILE_OBJECT,
      (0x00000004L), 0, 0, &pDACL, 0, &pSD);

    This code looks the same both in my own preprocessed i-file and in the file sent by the client.

    Hmm ... Okay, now we’ll examine how this function is declared. In my file I see:

    __declspec(dllimport)
    DWORD
    __stdcall
    GetNamedSecurityInfoW(
           LPCWSTR               pObjectName,
           SE_OBJECT_TYPE         ObjectType,
           SECURITY_INFORMATION   SecurityInfo,
                PSID         * ppsidOwner,
                PSID         * ppsidGroup,
                PACL         * ppDacl,
                PACL         * ppSacl,
          PSECURITY_DESCRIPTOR   * ppSecurityDescriptor
        );

    Everything is logical, everything is clear. Nothing unexpected.

    Next, I look at the client file and ...

    headquarters ???


    There I see something from parallel reality:

    __declspec(dllimport)
    DWORD
    __stdcall 
    GetNamedSecurityInfoW(
          LPCWSTR               pObjectName,
          SE_OBJECT_TYPE         ObjectType,
          SECURITY_INFORMATION   SecurityInfo,
         const PSID         * ppsidOwner,
         const PSID         * ppsidGroup,
         const PACL         * ppDacl,
         const PACL         * ppSacl,
         PSECURITY_DESCRIPTOR   * ppSecurityDescriptor
        );

    Note that the formal argument to ppDacl is marked as const .

    Wat? WTF? Wat? WTF?

    What a const !? Where is he from !?

    At least it’s immediately clear that the analyzer is not to blame here and I can defend his honor.

    The argument is a pointer to a constant object. It turns out that, from the analyzer's point of view, the GetNamedSecurityInfoW function cannot change the object that the pointer refers to. Hence here:

    PACL pDACL = NULL;
    PSECURITY_DESCRIPTOR pSD = NULL;
    ::GetNamedSecurityInfo(_T("ObjectName"), SE_FILE_OBJECT,
       DACL_SECURITY_INFORMATION, NULL, NULL, &pDACL, NULL, &pSD);
    auto test = pDACL == NULL; // V547 Expression 'pDACL == 0' is always true.

    the pDACL variable cannot change and the analyzer generates a reasonable warning (Expression 'pDACL == 0' is always true.).

    Why a warning is issued is understandable. But it is not clear where this const came from . It just can't be there!

    However, there is a hunch, and it is confirmed by searches on the Internet. It turns out that there is an old invalid aclapi.h file with an incorrect function description. I also found two interesting links on the Internet:


    So, once upon a time there was a function description in the aclapi.h file (6.0.6002.18005-Windows 6.0):

    WINADVAPI
    DWORD
    WINAPI
    GetNamedSecurityInfoW(
        __in  LPWSTR                pObjectName,
        __in  SE_OBJECT_TYPE         ObjectType,
        __in  SECURITY_INFORMATION   SecurityInfo,
        __out_opt PSID                 * ppsidOwner,
        __out_opt PSID                 * ppsidGroup,
        __out_opt PACL                 * ppDacl,
        __out_opt PACL                 * ppSacl,
        __out_opt PSECURITY_DESCRIPTOR * ppSecurityDescriptor
        );

    Then someone wanted to fix the type of the formal argument pObjectName , but along the way messed up the types of pointers by writing const . And aclapi.h (6.1.7601.23418-Windows 7.0) became like this:

    WINADVAPI
    DWORD
    WINAPI
    GetNamedSecurityInfoW(
        __in LPCWSTR pObjectName,
        __in SE_OBJECT_TYPE ObjectType,
        __in SECURITY_INFORMATION SecurityInfo,
        __out_opt const PSID * ppsidOwner,
        __out_opt const PSID * ppsidGroup,
        __out_opt const PACL * ppDacl,
        __out_opt const PACL * ppSacl,
        __out PSECURITY_DESCRIPTOR * ppSecurityDescriptor
        );

    diff 1


    It becomes clear that it is precisely the wrong wrong aclapi.h file that is used by the client. He later confirmed this hypothesis in correspondence. I use a more recent version, so the error did not reproduce.

    Here's what the already corrected function description in aclapi.h (6.3.9600.17415-Windows_8.1) looks like.

    WINADVAPI
    DWORD
    WINAPI
    GetNamedSecurityInfoW(
        _In_ LPCWSTR pObjectName,
        _In_ SE_OBJECT_TYPE ObjectType,
        _In_ SECURITY_INFORMATION SecurityInfo,
        _Out_opt_ PSID * ppsidOwner,
        _Out_opt_ PSID * ppsidGroup,
        _Out_opt_ PACL * ppDacl,
        _Out_opt_ PACL * ppSacl,
        _Out_ PSECURITY_DESCRIPTOR * ppSecurityDescriptor
        );

    diff 2


    The argument type pObjectName remained the same, but the extra const were removed. Everything has returned to their places, but header files with incorrect function declarations continue to live in the world.

    All this I tell the client. He is pleased and pleased that the situation has cleared up. Moreover, he finds the reason why he either sees a false positive or not:

    I forgot that I once experimented with toolsets on this test project. In the Debug test project, the configuration is configured on the Platform Toolset by default for Visual Studio 2017 - "Visual Studio 2017 (v141)", but the Release configuration is configured on "Visual Studio 2015 - Windows XP (v140_xp)". Yesterday, I just at some point changed the configuration, and the warning appeared and disappeared.

    All. You can put an end to the investigation. We decide with the client that we will not make any special backup in the analyzer so that it takes this bug into account in the header file. The main thing is that the situation is now clear. As the saying goes, "the case is closed."

    Conclusion

    The PVS-Studio analyzer is a complex software product that collects a lot of information from the program code and uses it for various analysis technologies . Specifically, in this case, the excessive intelligence of the analyzer has led to the fact that, due to an incorrect description of the function, it began to produce a false positive.

    Become our customer and you will receive fast, quality support from me and my colleagues.



    If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. False Positives in PVS-Studio: How Deep the Rabbit Hole Goes .

    Also popular now: