Our website uses cookies to enhance your browsing experience.
Our team provides quick and effective customer support. User requests are handled solely by programmers since our clients are programmers themselves and they often ask tricky questions. Today I'm going to tell you about a recent request concerning one false positive that even forced me to carry out a small investigation to solve the problem.
We work hard to reduce the number of false positives generated by PVS-Studio to a minimum. Unfortunately, static analyzers are often unable to tell correct code from a bug because they just don't have enough information. False positives are, therefore, inevitable. However, it's not a problem since you can easily customize the analyzer so that 9 out of 10 warnings will point to genuine bugs.
While false positives might not seem a big deal, we never stop fighting them by improving our diagnostics. Some blatant false positives are caught by our team; others are reported by our clients and free-version users.
One of our clients recently sent us an email reading something like this:
For some reason, the analyzer says that a certain pointer is always null, while it's not. Moreover, its behavior on a test project is weird and unstable: sometimes it issues a warning, and sometimes it doesn't. Here's a synthetic example reproducing that 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;
}It's not hard to guess how our users see false positives like that. The GetNamedSecurityInfo function obviously modifies the value of the variable pDACL. What prevented the developers from making a handler for simple cases like that? And why isn't the warning issued in every session? Maybe it's a bug in the analyzer itself, say, an uninitialized variable?
Alas... Supporting users of a static code analyzer is not an easy job, but it was my own choice to do that. So, I rolled up my sleeves and got down to investigating the problem.
I started off by checking the description of the GetNamedSecurityInfo function and making sure that its call indeed implied modifying the value of the pDACL variable. Here's the description of the 6th 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 PVS-Studio should obviously be able to handle such simple code without generating a false warning. At that point, my intuition was already telling me that the case wasn't a trivial one and that it was going to take quite a while to solve.
My misgivings were confirmed when I failed to reproduce the false positive either with our current alpha version of the analyzer or the version installed on the user's computer. No matter what I did, the analyzer kept silent.
I asked the client to send me the preprocessed i-file generated for the example program. He did that, and I went on with my investigation.
The analyzer did produce the false positive on that file right away. On the one hand, it was good that I had finally managed to reproduce it. On the other hand, I had a feeling that could be best illustrated by this picture:
Why this feeling? You see, I know perfectly well how both the analyzer and the V547 diagnostic work. There's simply no way they could generate a false positive like that, ever!
OK, let's make some tea and continue.
The call to the GetNamedSecurityInfo function expands into the following code:
::GetNamedSecurityInfoW(L"ObjectName", SE_FILE_OBJECT,
(0x00000004L), 0, 0, &pDACL, 0, &pSD);This code looks the same both in the i-file preprocessed on my computer and the file sent by the user.
Hmm... OK, let's look at the declaration of this function. Here's what I've got in my file:
__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
);All is logical and clear. Nothing unusual.
Then I peek into the user's file and ...
What I see there doesn't belong to our 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 parameter ppDacl is marked as const.
WAT? WTF? WAT? WTF?
What's that const!? What is it doing here!?
Well, at least I know for sure that the analyzer is innocent and I can defend its honor.
The argument is a pointer to a constant object. It turns out that, from the analyzer's point of view, the GetNamedSecurityInfoW function can't modify the object referred to by the pointer. Therefore, in the following code:
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 can't change, which the analyzer rightly warns us about (Expression 'pDACL == 0' is always true.).
OK, now we know what triggers the warning. What we still don't know is where that const keyword came from. It just can't be there!
Well, I have one guess, and it is confirmed by what I find on the Internet. It turns out that there's an old version of the file aclapi.h with an incorrect function description. I have also run across a couple of interesting links:
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 changed the type of the pObjectName parameter but messed up the types of the pointers along the way by adding the const keyword. And the aclapi.h file (6.1.7601.23418-Windows 7.0) ended up as follows:
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
);
Now it was clear that our user had been working with that very incorrect version of aclapi.h, which he then confirmed in his email. I couldn't reproduce the bug because I was using a more recent version.
This is what the fixed function description looks like in the latest aclapi.h file (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 is still the same, but the extra const's are gone. All is good again, but there are still broken headers in use somewhere out there.
I explain all of that to the customer, and he is happy to see the problem solved. Moreover, he has found out why the false positive didn't occur regularly:
I now recall experimenting with toolsets on this test project some time ago. The Debug configuration was set to Platform Toolset by default for Visual Studio 2017 - "Visual Studio 2017 (v141)", while the Release configuration was set to "Visual Studio 2015 - Windows XP (v140_xp)". I was simply switching between the configurations yesterday, and the warning would appear and disappear accordingly.
That's all. The investigation is over. We discuss the issue with the client and decide not to add any kludge to the analyzer to make it able to handle this header-file bug. The most important thing is that we have figured out the problem. "Case dismissed", as they say.
Conclusion
PVS-Studio is a complex software product, which gathers large amounts of information from programs' code and utilizes it in various analysis techniques. In this particular case, it turned out to be too smart, ending up with a false positive because of an incorrect function description.
Become our clients, and you are guaranteed to get prompt professional support from me and my teammates.
0
0
0
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
0