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

Our team provides fast and efficient customer support. Only programmers take part in the support, as the programmers also ask us questions and we have to think about many of them. I would like to describe one of the recent appeals in support of the topic 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 issued by the PVS-Studio analyzer. Unfortunately, static analyzers often cannot distinguish a correct code from an error, since they simply lack information. As a result, there are false positives anyway. However, this is not a problem, since after setting up the analyzer it is easy to achieve a situation where 9 out of 10 warnings will indicate real errors.

Although false alarms are not as big a problem as it may seem at first glance, we constantly struggle with them, improving diagnostics. We notice some glaring false positives ourselves, some customers and free users write to us.
')
Recently, one of our clients wrote a letter about the following:

The analyzer for some reason says that the pointer is always zero, although this is clearly not the case. Moreover, the analyzer behaves strangely and unstable on a test project: it issues a warning, it does not issue a warning. Synthetic example that reproduces a false positive:

#include <windows.h> #include <aclapi.h> #include <tchar.h> 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 the responses are from our users. It is immediately clear that the GetNamedSecurityInfo function changes the value of the pDACL variable. Couldn't the developers write a handler for such simple situations? Moreover, it is not clear why the analyzer either displays a message or does not. Maybe they themselves have some kind of bug in the tool, such as an uninitialized variable?

Eh ... Not an easy job - to support a static code analyzer. But what to do, I myself have chosen such a fate. Rolling up my sleeves, I proceeded to investigate the cause of the occurrence of a false positive.

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

ppDacl Descriptor or NULL cript If you’re security descriptor has no DACL. The DACL_SECURITY_INFORMATION flag. Also, this parameter can be NULL if you don’t need the DACL.

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

I became convinced of my concerns when I could not reproduce the false positives either on the current alpha version of the analyzer, or on the exact version that is installed on the client. And so, and syak, but the analyzer is silent.

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

On the sent file, 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.

Why exactly these? I know perfectly well how the V547 analyzer and diagnostics work . Well, there can not be such a response!

Ok, make tea and continue.

The call to the GetNamedSecurityInfo function is expanded to:

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

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

Hmm ... Ok, now let's see 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 ...

There I see something from a 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 ); 

Notice that the formal argument ppDacl is marked as const .

WAT? WTF? WAT? WTF?

What const !? Where is he from here !?

At least it is immediately clear that the analyzer is not at fault here and I will be able to protect his honor.

The argument is a pointer to a constant object. It turns out that, from the point of view of the analyzer, the function GetNamedSecurityInfoW cannot change the object to which the pointer refers. Therefore, 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 guess, and it is confirmed by searches on the Internet. It turns out that there is an old incorrect aclapi.h file with an incorrect description of the function. I also found two interesting links on the Internet:

• Headers diff for advapi32.dll between 6.0.6002.18005-Windows 6.0 and 6.1.7601.23418-Windows 7.0 versions
• Headers diff for advapi32.dll between 6.1.7601.23418-Windows_7.0 and 6.3.9600.17415-Windows_8.1 versions

So, there was once in the aclapi.h file (6.0.6002.18005-Windows 6.0) here is the description of the function:

 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 correct the type of the formal argument pObjectName , but at the same time spoiled the pointer types by typing const . And aclapi.h (6.1.7601.23418-Windows 7.0) has become:

 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 ); 

It becomes clear that this is exactly the wrong aclapi.h file used by the client. Later in correspondence, he confirmed this hypothesis. I use a more recent version, so the error was not reproduced.

Here is what the revised feature description looks like in aclapi.h (6.3.9600.17415-Windows_8.1).

 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 ); 

The type of the pObjectName argument remains the same, but the extra const is removed. Everything has returned to its place, but in the world so far the header files continue to live with the wrong function declaration.

I tell all this to the client. He is pleased and pleased that the situation has cleared up. Moreover, he finds the reason why he sees a false positive, then no:

I forgot that I once experimented with toolsets on this test project. In the test project, the Debug configuration is configured on the Platform Toolset by default for Visual Studio 2017 - “Visual Studio 2017 (v141)”, and here 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.

Everything. In the investigation, you can put a full stop. With the client, we decide 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 now the situation is clear. As they say, "the case is closed."

Conclusion

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 led to the fact that, due to an incorrect description of the function, it began to produce a false positive.

Become our clients 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 .