A new version of the PVS-Studio analyzer 6.23 is working under macOS, which allows you to check the projects written in C and C++. Our team decided to perform a XNU Kernel check to coincide it with this event.
With the release of the analyzer version for macOS, PVS-Studio can now be boldly called a cross-platform static code analyzer for C and C++ code.
Originally, there was only a Windows version. About two years ago our team supported Linux: "The Development History of PVS-Studio for Linux". Also, attentive readers of our blog should remember the articles about the FreeBSD Kernel check (1st article, 2nd article). At that time, the analyzer has been built to be launched in PC-BSD and TrueOS. Now, finally, we got to macOS!
Is it easy to develop a cross-platform product?
This issue has economic and technical component.
From the economic point of view, it was the right decision to make a cross-platform analyzer. Software development has long been moving in this direction, and a tool for developers of such projects must be relevant. However, if something is useful, it does not mean that it is worth doing straight away. In the beginning, we always make sure we have enough forces to implement something in a new direction, and then maintain it.
Technically, it is difficult only in the very beginning, if the project is not directly intended as cross-platform. We spent a few months on the adaptation of the analyzer in a Linux system. Compilation of a project under a new platform didn't take much time: we have no GUI and the code is practically not connected with using of the system API. Analyzer adaptation under new compilers and improvement of analysis quality took most of the time. In other words, preventing false positives requires many efforts.
What's with the development under macOS?
At this point, we already had the analyzer project file for CMake, which was easily adaptable under different operating systems. Testing systems of different types were also cross-platform. All this has helped to start out on macOS.
The Apple LLVM Compiler became the feature of the analyzer development under macOS. Although the analyzer was building perfectly using GCC and worked magnificently, but it still could have an impact on analyzer compatibility with users' computers. To avoid creating problems for potential users, we have decided to support the distribution build using this compiler that comes with Xcode.
C++ development greatly helps in the creating and development of cross-platform projects, but different compilers add such capabilities unevenly, so conditional compilation is still actively used in several places.
In general, everything went smoothly and easily. As before, most of the time was spent on refinement of the exceptions, site modification, testing and other related issues. As a first project, checked using PVS-Studio for macOS we'd like to present you the XNU Kernel.
Distribution package
Please click here, for further information about the ways to download and install PVS-Studio for macOS.
How to start demonstrating the abilities of PVS-Studio for macOS? No doubts, the check of the kernel of this system is the best variant! Therefore, the first project, checked using the new version of the analyzer, became the XNU Kernel.
XNU is a kernel of computer operating systems developed by Apple and used in OS X operating systems (macOS, iOS, tvOS, watchOS). Read more.
It is considered that the kernel is written in C and C++, but in fact, it is C. I counted 1302 *.c-files and only 97 *.cpp-files. The size of the codebase is 1929 KLOC. It turns out that this is a relatively small project. For comparison, the codebase of the Chromium project is in 15 times larger and contains 30 MLOC.
The source code can be conveniently downloaded from a mirror on GitHub: xnu.
Although the XNU Kernel is relatively small, it's a challenge to study the analyzer warnings alone, which takes much time. False positives make the check more complicated, since I haven't performed the preliminary analyzer configuration. I just quickly looked through the warnings, writing out code fragments that, in my opinion, represent interest. This is more than enough for writing a quite large article. PVS-Studio analyzer easily finds a large number of interesting bugs.
Note for XNU Kernel developers. I didn't have an objective to find as many bugs as possible. Therefore, you should not be guided by the article to fix them. Firstly, it's awkward, because there is no possibility to navigate along the warnings. Sure, it is much better to use one of the formats, which can generate PVS-Studio, for example, the HTML report with the possibility of navigation (it is similar to something that Clang can generate). Secondly, I skipped many errors simply because I studied the report superficially. I recommend developers to perform a more thorough analysis of the project with the help of PVS-Studio themselves.
As I said, I was bothered with false positives, but in fact, they are no problem. If you configure the analyzer, it is possible to reduce the number of false positives to 10-15%. As analyzer configuration also requires time and restarting the process of analyzing, I skipped this step - it wasn't difficult for me to gather errors for the article even without it. If you plan to perform the analysis carefully, of course, you should take time to make configurations.
Mostly, false positives occur due to macros and functions marked not qualitatively enough. For example, in the XNU Kernel, most of them is associated with using of panic.
This is how this function is declared:
extern void panic(const char *string, ...)
__attribute__((__format__ (__printf__, 1, 2)));The function is annotated the way its arguments are interpreted by analogy with the arguments of the printf function. This enables compilers and analyzers to find errors of incorrect strings formatting. However, the function is not marked as the one that doesn't return control. As a result, the following code produces false positives:
if (!ptr)
panic("zzzzzz");
memcpy(ptr, src, n);Here the analyzer issues the warning that a dereferencing of a null pointer is possible. From its point of view, after calling the panic function, the memcpy function will be called as well.
To avoid similar false positives, you must change the annotation of the function by adding __attribute__((noreturn)):
extern __attribute__((noreturn)) void panic(const char *string, ...)
__attribute__((__format__ (__printf__, 1, 2)));Now let's see what interesting things I managed to notice in the code of the XNU Kernel. In total, I noted 64 errors and decided to stop at this beautiful number. I have grouped the defects according to Common Weakness Enumeration, this classification is quite well-known and it will be easier to understand what errors are a question of this or that chapter.
Various errors can lead to CWE-570/CWE-571, i.e. situations where a condition or part of a condition is always false/true. In the case of the XNU Kernel, all these errors, in my opinion, are related to typos. PVS-Studio is generally great at identifying typos.
Fragment N1
int
key_parse(
struct mbuf *m,
struct socket *so)
{
....
if ((m->m_flags & M_PKTHDR) == 0 ||
m->m_pkthdr.len != m->m_pkthdr.len) {
ipseclog((LOG_DEBUG,
"key_parse: invalid message length.\n"));
PFKEY_STAT_INCREMENT(pfkeystat.out_invlen);
error = EINVAL;
goto senderror;
}
....
}PVS-Studio warning: V501 CWE-570 There are identical sub-expressions 'm->M_dat.MH.MH_pkthdr.len' to the left and to the right of the '!=' operator. key.c 9442
Due to a typo, a class member is compared with itself:
m->m_pkthdr.len != m->m_pkthdr.lenPart of the condition is always false, and as a result, the length of a message is checked incorrectly. It turns out that the program will continue handling incorrect data. Perhaps it's not that scary, but many vulnerabilities are just related to the fact that some input data was unchecked or insufficiently checked. So this fragment of code is clearly worth paying attention of developers.
Fragment N2, N3
#define VM_PURGABLE_STATE_MASK 3
kern_return_t
memory_entry_purgeable_control_internal(...., int *state)
{
....
if ((control == VM_PURGABLE_SET_STATE ||
control == VM_PURGABLE_SET_STATE_FROM_KERNEL) &&
(((*state & ~(VM_PURGABLE_ALL_MASKS)) != 0) ||
((*state & VM_PURGABLE_STATE_MASK) >
VM_PURGABLE_STATE_MASK)))
return(KERN_INVALID_ARGUMENT);
....
}PVS-Studio warning: V560 CWE-570 A part of conditional expression is always false: ((* state & 3) > 3). vm_user.c 3415
Let's consider in more detail this part of expression:
(*state & VM_PURGABLE_STATE_MASK) > VM_PURGABLE_STATE_MASKIf you substitute the value of the macro, you'll get:
(*state & 3) > 3Bitwise AND operation may result in only the values 0, 1, 2, or 3. It is pointless to check whether 0, 1, 2 or 3 is more than 3. It is very likely that the expression contains a typo.
As in the previous case, a status is checked incorrectly, which can result in incorrect processing of incorrect (tainted) data.
The same error is detected in the file vm_map.c. Apparently, a part of the code was written using Copy-Paste. Warning: V560 CWE-570 A part of conditional expression is always false: ((* state & 3) > 3). vm_map.c 15809
Fragment N4
void
pat_init(void)
{
boolean_t istate;
uint64_t pat;
if (!(cpuid_features() & CPUID_FEATURE_PAT))
return;
istate = ml_set_interrupts_enabled(FALSE);
pat = rdmsr64(MSR_IA32_CR_PAT);
DBG("CPU%d PAT: was 0x%016llx\n", get_cpu_number(), pat);
/* Change PA6 attribute field to WC if required */
if ((pat & ~(0x0FULL << 48)) != (0x01ULL << 48)) {
mtrr_update_action(CACHE_CONTROL_PAT);
}
ml_set_interrupts_enabled(istate);
}PVS-Studio warning: V547 CWE-571 Expression is always true. mtrr.c 692
Let's go through a pointless check, which is likely to have a typo:
(pat & ~(0x0FULL << 48)) != (0x01ULL << 48)Let's calculate some expressions:
The expression (pat & [0xFFF0FFFFFFFFFFFF]) can not result in the value 0x0001000000000000. The condition is always true. As a result, the function mtrr_update_action is always called.
Fragment N5
Here is a typo which, in my opinion, is very beautiful.
typedef enum {
CMODE_WK = 0,
CMODE_LZ4 = 1,
CMODE_HYB = 2,
VM_COMPRESSOR_DEFAULT_CODEC = 3,
CMODE_INVALID = 4
} vm_compressor_mode_t;
void vm_compressor_algorithm_init(void) {
....
assertf(((new_codec == VM_COMPRESSOR_DEFAULT_CODEC) ||
(new_codec == CMODE_WK) ||
(new_codec == CMODE_LZ4) || (new_codec = CMODE_HYB)),
"Invalid VM compression codec: %u", new_codec);
....
}PVS-Studio warning: V768 CWE-571 The expression 'new_codec = CMODE_HYB' is of enum type. It is odd that it is used as an expression of a Boolean-type. vm_compressor_algorithms.c 419
In the process of checking the condition, the variable new_codec is assigned a value of 2. As a result, the condition is always true and the assert-macro actually checks nothing.
The error could be harmless. Well, big deal, macro assert didn't check something - no problem. However, in addition, the debug version also doesn't work correctly. The value of the variable new_codec goes bad and the wrong codec is used, not the one, which was required.
Fragment N6, N7
void
pbuf_copy_back(pbuf_t *pbuf, int off, int len, void *src)
{
VERIFY(off >= 0);
VERIFY(len >= 0);
VERIFY((u_int)(off + len) <= pbuf->pb_packet_len);
if (pbuf->pb_type == PBUF_TYPE_MBUF)
m_copyback(pbuf->pb_mbuf, off, len, src);
else
if (pbuf->pb_type == PBUF_TYPE_MBUF) {
if (len)
memcpy(&((uint8_t *)pbuf->pb_data)[off], src, len);
} else
panic("%s: bad pb_type: %d", __func__, pbuf->pb_type);
}PVS-Studio warning: V517 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 340, 343. pf_pbuf.c 340
To clarify, I'll highlight the main point:
if (A)
foo();
else
if (A)
Unreachable_code;
else
panic();If the A condition is true, then the body of the first if operator is executed. If not, a repeated check doesn't make sense and the panic function is called immediately. A part of the code is generally unattainable.
Here is an error either in logic, or a typo in one of the conditions.
Later in this same file, there is the function pbuf_copy_data, which apparently was written by using Copy-Paste and contains the same error. Warning: V517 CWE-570 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 358, 361. pf_pbuf.c 358
The defect CWE-670 says that, most likely, in the code something is not working as intended.
Fragment N8, N9, N10
static void
in_ifaddr_free(struct ifaddr *ifa)
{
IFA_LOCK_ASSERT_HELD(ifa);
if (ifa->ifa_refcnt != 0) {
panic("%s: ifa %p bad ref cnt", __func__, ifa);
/* NOTREACHED */
} if (!(ifa->ifa_debug & IFD_ALLOC)) {
panic("%s: ifa %p cannot be freed", __func__, ifa);
/* NOTREACHED */
}
if (ifa->ifa_debug & IFD_DEBUG) {
....
}PVS-Studio warning: V646 CWE-670 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. in.c 2010
Perhaps, there is no error in this code. However, this place looks very suspiciously:
} if (!(ifa->ifa_debug & IFD_ALLOC)) {It's not normal as it's not the done thing. It would be more logical to start writing if on a new line. Code authors should check out this place. Perhaps, the key word else is omitted here and the code should be as follows:
} else if (!(ifa->ifa_debug & IFD_ALLOC)) {Or you just need to add a line break, so that this code would confuse neither the analyzer, nor the colleagues maintaining this code.
Similar suspicious fragments can be found here:
Fragment N11, N12, N13, N14
int
dup2(proc_t p, struct dup2_args *uap, int32_t *retval)
{
....
while ((fdp->fd_ofileflags[new] & UF_RESERVED) == UF_RESERVED)
{
fp_drop(p, old, fp, 1);
procfdtbl_waitfd(p, new);
#if DIAGNOSTIC
proc_fdlock_assert(p, LCK_MTX_ASSERT_OWNED);
#endif
goto startover;
}
....
startover:
....
}PVS-Studio warning: V612 CWE-670 An unconditional 'goto' within a loop. kern_descrip.c 628
This code is very strange. Note that the body of the while operator ends with the goto operator. In doing so, the operator 'continue' is not used the body of the loop. This means that the body of the loop will be executed no more than once.
Why create a loop, if it does not perform more than one iteration? Really, it would be better to use the operator 'if', then it would not raise any questions. I think that's an error, and in the cycle something is written wrong. For example, perhaps, before the operator 'goto' there is no condition.
Similar "one-time" loops are found 3 more times:
There are various reasons because of which null pointer dereferencing may happen and PVS-Studio analyzer, depending on the situation, can assign them various CWE-ID:
When writing the article I considered it reasonable to collect all the errors of this type in one section.
Fragment N15
I'll start with complex and large functions. First, we'll look at the function netagent_send_error_response in which the pointer, passed in the session argument, gets dereferenced.
static int
netagent_send_error_response(
struct netagent_session *session, u_int8_t message_type,
u_int32_t message_id, u_int32_t error_code)
{
int error = 0;
u_int8_t *response = NULL;
size_t response_size = sizeof(struct netagent_message_header);
MALLOC(response, u_int8_t *, response_size,
M_NETAGENT, M_WAITOK);
if (response == NULL) {
return (ENOMEM);
}
(void)netagent_buffer_write_message_header(.....);
if ((error = netagent_send_ctl_data(session->control_unit,
(u_int8_t *)response, response_size))) {
NETAGENTLOG0(LOG_ERR, "Failed to send response");
}
FREE(response, M_NETAGENT);
return (error);
}Note that the pointer session is dereferenced in the expression session->control_unit without any preliminary check. Whether a dereference of a null pointer occurs or not, depends on what actual arguments will be passed to this function.
Now let's see how the function netagent_send_error_response discussed above, is used in the function netagent_handle_unregister_message.
static void
netagent_handle_unregister_message(
struct netagent_session *session, ....)
#pragma unused(payload_length, packet, offset)
u_int32_t response_error = NETAGENT_MESSAGE_ERROR_INTERNAL;
if (session == NULL) {
NETAGENTLOG0(LOG_ERR, "Failed to find session");
response_error = NETAGENT_MESSAGE_ERROR_INTERNAL;
goto fail;
}
netagent_unregister_session_wrapper(session);
netagent_send_success_response(session, .....);
return;
fail:
netagent_send_error_response(
session, NETAGENT_MESSAGE_TYPE_UNREGISTER, message_id,
response_error);
}PVS-Studio warning: V522 CWE-628 Dereferencing of the null pointer 'session' might take place. The null pointer is passed into 'netagent_send_error_response' function. Inspect the first argument. Check lines: 427, 972. network_agent.c 427
Here Data Flow analysis, implemented in PVS-Studio, shows itself. The analyzer notes that if the session pointer was equal to NULL, then some information would be written to the log, and then it goes to a label fails.
Next, a call to the function netagent_send_error_response will follow:
fail:
netagent_send_error_response(
session, NETAGENT_MESSAGE_TYPE_UNREGISTER, message_id,
response_error);Note that the ill-fated session pointer that is equal to NULL is passed to the function as an actual argument.
As we know, in the function netagent_send_error_response there is no protection in this case and a null pointer dereference will occur.
Fragment N16
The next situation is similar to the previous one. The function code is shorter, but we'll have to deal with it the same slowly and thoroughly.
void *
pf_lazy_makewritable(struct pf_pdesc *pd, pbuf_t *pbuf, int len)
{
void *p;
if (pd->lmw < 0)
return (NULL);
VERIFY(pbuf == pd->mp);
p = pbuf->pb_data;
if (len > pd->lmw) {
....
}Note that the pointer pbuf is dereferenced without prior check for NULL. In code there is a check "VERIFY(pbuf == pd->mp)". However, pd-> mp may be equal to NULL, so the check cannot be seen as protection against NULL.
Note. Please, remember that I'm not familiar with the XNU Kernel code and I may be wrong. Possibly pd->mp will never store the NULL value. Then all my reasoning is wrong and there is no error here. However, such code still needs to be checked again.
Let's continue and see how to the described function pf_lazy_makewritable is used.
static int
pf_test_state_icmp(....)
{
....
if (pf_lazy_makewritable(pd, NULL,
off + sizeof (struct icmp6_hdr)) ==
NULL)
return (PF_DROP);
....
}PVS-Studio warning: V522 CWE-628 Dereferencing of the null pointer 'pbuf' might take place. The null pointer is passed into 'pf_lazy_makewritable' function. Inspect the second argument. Check lines: 349, 7460. pf.c 349
NULL is passed to the function pf_lazy_makewritable as the second actual argument. This is very strange.
Let's say, a programmer thinks that "VERIFY(pbuf == pd->mp)" will protect the program from the null pointer. Then the question arises: why write such code? Why call a function passing clearly incorrect argument?
Therefore, it seems to me that actually, the function pf_lazy_makewritable must be able to accept a null pointer and handle this case in a special way, but it doesn't do so. This code deserves thorough verification by a programmer, and the PVS-Studio analyzer is definitely right, drawing our attention to it.
Fragment N17
Let's relax for a while and consider a simple case.
typedef struct vnode * vnode_t;
int
cache_lookup_path(...., vnode_t dp, ....)
{
....
if (dp && (dp->v_flag & VISHARDLINK)) {
break;
}
if ((dp->v_flag & VROOT) ||
dp == ndp->ni_rootdir ||
dp->v_parent == NULLVP)
break;
....
}PVS-Studio warning: V522 CWE-690 There might be dereferencing of a potential null pointer 'dp'. vfs_cache.c 1449
Look at the check:
if (dp && (dp->v_flag & VISHARDLINK))It tells us that a pointer dp can be null. However further on, the pointer is dereferenced before the preliminary check:
if ((dp->v_flag & VROOT) || ....)Fragment N18
In the previous example, we saw a situation where the pointer was checked before dereference, and then check in code was forgotten. But much more often you may come across a situation when pointer is dereferenced first, and only then is checked. The code of the XNU Kernel project was no exception. First, let's consider a synthetic sample for better understanding what it is about:
p[n] = 1;
if (!p) return false;Now let's see how these errors look like in reality. We'll start with the function of names comparison. The comparison functions are very insidious :).
bool
IORegistryEntry::compareName(....) const
{
const OSSymbol * sym = copyName();
bool isEqual;
isEqual = sym->isEqualTo( name ); // <=
if( isEqual && matched) {
name->retain();
*matched = name;
}
if( sym) // <=
sym->release();
return( isEqual );
}PVS-Studio warnings: V595 CWE-476 The 'sym' pointer was utilized before it was verified against nullptr. Check lines: 889, 896. IORegistryEntry.cpp 889
I've marked with comments like "//< =" lines of code which are of interest for us. As you can see, the first pointer is dereferenced. Further, in code, there is a check for pointer equality to nullptr. But it is clear at once that if the pointer is null, then there will be a null pointer dereferencing and function, in fact, is not ready for such a situation.
Fragment N19
The following error occurred because of a typo.
static int
memorystatus_get_priority_list(
memorystatus_priority_entry_t **list_ptr, size_t *buffer_size,
size_t *list_size, boolean_t size_only)
{
....
*list_ptr = (memorystatus_priority_entry_t*)kalloc(*list_size);
if (!list_ptr) {
return ENOMEM;
}
....
}PVS-Studio warning: V595 CWE-476 The 'list_ptr' pointer was utilized before it was verified against nullptr. Check lines: 7175, 7176. kern_memorystatus.c 7175
The analyzer sees that the variable is first dereferenced, and in the following line is checked for equality to nullptr. This interesting error occurred due to the fact that the programmer forgot to write the character '*'. Actually, correct code should be as follows:
*list_ptr = (memorystatus_priority_entry_t*)kalloc(*list_size);
if (!*list_ptr) {
return ENOMEM;
}We can say that the error was identified indirectly. However, it does not matter, because the most important thing is that the analyzer drew our attention to abnormal code and we saw the error.
Fragment N20 - N35
In the XNU Kernel code there are many errors identified thanks to the V595 diagnostic. However, considering all of them will be boring. So, I will regard just one case, and cite a list of messages that indicate errors.
inline void
inp_decr_sndbytes_unsent(struct socket *so, int32_t len)
{
struct inpcb *inp = (struct inpcb *)so->so_pcb;
struct ifnet *ifp = inp->inp_last_outifp;
if (so == NULL || !(so->so_snd.sb_flags & SB_SNDBYTE_CNT))
return;
if (ifp != NULL) {
if (ifp->if_sndbyte_unsent >= len)
OSAddAtomic64(-len, &ifp->if_sndbyte_unsent);
else
ifp->if_sndbyte_unsent = 0;
}
}PVS-Studio warning: V595 CWE-476 The 'so' pointer was utilized before it was verified against nullptr. Check lines: 3450, 3453. in_pcb.c 3450
I suggest the reader to independently follow the fate of the pointer so and make sure, that the code is written incorrectly.
Other errors:
Fragment N36, N37
And the last couple bugs on the use of NULL pointers.
static void
feth_start(ifnet_t ifp)
{
....
if_fake_ref fakeif;
....
if (fakeif != NULL) {
peer = fakeif->iff_peer;
flags = fakeif->iff_flags;
}
/* check for pending TX */
m = fakeif->iff_pending_tx_packet;
....
}PVS-Studio warning: V1004 CWE-476 The 'fakeif' pointer was used unsafely after it was verified against nullptr. Check lines: 566, 572. if_fake.c 572
I think, this code doesn't need any comments. Just look how the pointer fakeif is checked and used.
The last similar case: V1004 CWE-476 The 'rt->rt_ifp' pointer was used unsafely after it was verified against nullptr. Check lines: 138, 140. netsrc.c 140
I came across a couple of errors, related to the buffer overrun. A very unpleasant kind of error for such a responsible project, like XNU Kernel.
Different variants of array overrun can be classified with different CWE ID, but in this case, the analyzer chose CWE-119.
Fragment N38
For a start, let's see how some macros are declared.
#define IFNAMSIZ 16
#define IFXNAMSIZ (IFNAMSIZ + 8)
#define MAX_ROUTE_RULE_INTERFACES 10It is important for us to remember that:
And now we'll look at the function where the buffer overrun is possible when using the snprintf and memset functions. So, 2 errors take place here.
static inline const char *
necp_get_result_description(....)
{
....
char interface_names[IFXNAMSIZ][MAX_ROUTE_RULE_INTERFACES];
....
for (index = 0; index < MAX_ROUTE_RULE_INTERFACES; index++) {
if (route_rule->exception_if_indices[index] != 0) {
ifnet_t interface = ifindex2ifnet[....];
snprintf(interface_names[index],
IFXNAMSIZ, "%s%d", ifnet_name(interface),
ifnet_unit(interface));
} else {
memset(interface_names[index], 0, IFXNAMSIZ);
}
}
....
}PVS-Studio warnings:
Notice how the two-dimensional array interface_names is declared:
char interface_names[IFXNAMSIZ][MAX_ROUTE_RULE_INTERFACES];
// i.g.: char interface_names[24][10];But this array is used as if it is as follows:
char interface_names[MAX_ROUTE_RULE_INTERFACES][IFXNAMSIZ];
// i.g.: char interface_names[10][24];In the result we get a mush of data.
Someone may say without thinking, that there is nothing to worry about, because both arrays hold the same number of bytes.
No, it's bad. The elements of the array interface_names[10..23][....] are not used, because the variable index in the loop takes values [0..9]. But the elements of interface_names[0..9][....] begin to overlap each other. I.e. some data overwrites the other.
The result is just nonsense. A part of the array remains uninitialized, and the other part contains a "mush", when data was written over the already written data.
Fragment N39
Later in this same file necp_client.c there is another function that contains very similar errors.
#define IFNAMSIZ 16
#define IFXNAMSIZ (IFNAMSIZ + 8)
#define NECP_MAX_PARSED_PARAMETERS 16
struct necp_client_parsed_parameters {
....
char prohibited_interfaces[IFXNAMSIZ]
[NECP_MAX_PARSED_PARAMETERS];
....
};
static int
necp_client_parse_parameters(....,
struct necp_client_parsed_parameters *parsed_parameters)
{
....
u_int32_t length = ....;
....
if (length <= IFXNAMSIZ && length > 0) {
memcpy(parsed_parameters->prohibited_interfaces[
num_prohibited_interfaces],
value, length);
parsed_parameters->prohibited_interfaces[
num_prohibited_interfaces][length - 1] = 0;
....
}PVS-Studio warning:
All the same. The array:
char prohibited_interfaces[IFXNAMSIZ][NECP_MAX_PARSED_PARAMETERS];is handled as if it is:
char prohibited_interfaces[NECP_MAX_PARSED_PARAMETERS][IFXNAMSIZ];Defects CWE-563 detected by PVS-Studio are often the consequences of typos. Now we'll consider one such beautiful typo.
Fragment N40
uint32_t
gss_krb5_3des_unwrap_mbuf(....)
{
....
for (cflag = 1; cflag >= 0; cflag--) {
*minor = gss_krb5_3des_token_get(
ctx, &itoken, wrap, &hash, &offset, &length, reverse);
if (*minor == 0)
break;
wrap.Seal_Alg[0] = 0xff;
wrap.Seal_Alg[0] = 0xff;
}
....
}PVS-Studio warning: V519 CWE-563 The 'wrap.Seal_Alg[0]' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2070, 2071. gss_krb5_mech.c 2071
The value 0xff is written in the same element of the array twice. I looked at the code and concluded that the programmers actually wanted to write here:
wrap.Seal_Alg[0] = 0xff;
wrap.Seal_Alg[1] = 0xff;Judging by the name of the function, it is associated with a network authentication protocol. And such a kludge. Just terrifying.
You can buy PVS-Studio here. Our analyzer will help prevent many of these errors!
Fragment N41, N42, N43, N44
static struct mbuf *
pf_reassemble(struct mbuf *m0, struct pf_fragment **frag,
struct pf_frent *frent, int mff)
{
....
m->m_pkthdr.csum_flags &= ~CSUM_PARTIAL;
m->m_pkthdr.csum_flags =
CSUM_DATA_VALID | CSUM_PSEUDO_HDR |
CSUM_IP_CHECKED | CSUM_IP_VALID;
....
}PVS-Studio warning: V519 CWE-563 The 'm->M_dat.MH.MH_pkthdr.csum_flags' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 758, 759. pf_norm.c 759
String:
m->m_pkthdr.csum_flags &= ~CSUM_PARTIAL;has no practical meaning. In the next string the variable m->m_pkthdr.csum_flags will be assigned a new value. I don't know how the correct code should actually look like, but I would venture to guess that the symbol '|' was lost. In my humble opinion, your code should look like this:
m->m_pkthdr.csum_flags &= ~CSUM_PARTIAL;
m->m_pkthdr.csum_flags |=
CSUM_DATA_VALID | CSUM_PSEUDO_HDR |
CSUM_IP_CHECKED | CSUM_IP_VALID;There are 3 warnings pointing at similar errors:
A very insidious type of defect that is invisible in the debug version. If the reader is not familiar with it yet, before you continue reading, I suggest to be acquainted with the following links:
If the reader wonders why overwrite private data that is stored in the memory, I recommend the article "Overwriting memory - why?".
So, it is important to overwrite private data in memory, but sometimes the compiler removes the corresponding code, because, from its point of view, it is redundant. Let's see what interesting things were found in the XNU Kernel on this topic.
Fragment N45
__private_extern__ void
YSHA1Final(unsigned char digest[20], YSHA1_CTX* context)
{
u_int32_t i, j;
unsigned char finalcount[8];
....
/* Wipe variables */
i = j = 0;
memset(context->buffer, 0, 64);
memset(context->state, 0, 20);
memset(context->count, 0, 8);
memset(finalcount, 0, 8); // <=
#ifdef SHA1HANDSOFF
YSHA1Transform(context->state, context->buffer);
#endif
}PVS-Studio warning: V597 CWE-14 The compiler could delete the 'memset' function call, which is used to flush 'finalcount' buffer. The memset_s() function should be used to erase the private data. sha1mod.c 188
The compiler may remove the line of code which I marked with the comment "// <=" in order to optimize the Release-version. Almost certainly, it will act in this way.
Fragment N46
__private_extern__ void
YSHA1Transform(u_int32_t state[5], const unsigned char buffer[64])
{
u_int32_t a, b, c, d, e;
....
state[0] += a;
state[1] += b;
state[2] += c;
state[3] += d;
state[4] += e;
/* Wipe variables */
a = b = c = d = e = 0;
}PVS-Studio warning: V1001 CWE-563 The 'a' variable is assigned but is not used until the end of the function. sha1mod.c 120
The compiler may not generate code that resets the variables, since they are not used in the function.
I would like to draw your attention to the fact that PVS-Studio analyzer interpreted this suspicious situation as CWE-563. The fact of the matter is that the same defect can often be interpreted as different CWE and in this case, the analyzer chose CWE-563. However, I decided to include this code to CWE-14 because it explains more accurately, what's wrong with this code.
The defect CWE-783 occurs where the programmer confused priorities of the operations and wrote code that works not the way he had planned. Often these errors are made because of carelessness or missing parentheses.
Fragment N47
int
getxattr(....)
{
....
if ((error = copyinstr(uap->attrname, attrname,
sizeof(attrname), &namelen) != 0)) {
goto out;
}
....
out:
....
return (error);
}PVS-Studio warning: V593 CWE-783 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. vfs_syscalls.c 10574
A classic error. I meet a lot of such bugs in various programs (proof). The root cause is that for some reason programmers seek to cram more just in one line.
As a result, instead of:
Status s = foo();
if (s == Error)
return s;they write:
Status s;
if (s = foo() == Error)
return s;And contribute the error to the code.
As a result, the return operator returns incorrect error status equal to 1, but not the value that is equal to a constant Error.
I regularly criticize such code and recommend not to "shove in" in one line more than one action. "Stuffing in" doesn't really reduce the code size, but provokes a different error. See the chapter 13 from the book "The Ultimate Question of Programming, Refactoring, and Everything" for more details. See the chapters:
Let's get back to code from the XNU Kernel. In case of an error, the function getxattr will return value of 1, not the actual error code.
Fragment N48-N52
static void
memorystatus_init_snapshot_vmstats(
memorystatus_jetsam_snapshot_t *snapshot)
{
kern_return_t kr = KERN_SUCCESS;
mach_msg_type_number_t count = HOST_VM_INFO64_COUNT;
vm_statistics64_data_t vm_stat;
if ((kr = host_statistics64(.....) != KERN_SUCCESS)) {
printf("memorystatus_init_jetsam_snapshot_stats: "
"host_statistics64 failed with %d\n", kr);
memset(&snapshot->stats, 0, sizeof(snapshot->stats));
} else {
+ ....
}PVS-Studio warning: V593 CWE-783 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. kern_memorystatus.c 4554
Variable kr can be assigned only two values: 0 or 1. Due to this printf function always prints the number 1 instead of the actual status, which the function host_statistics64 returned.
Article turns out to be large. I guess I'm tiring not only myself, but also the readers. So I'm reducing the number of fragments regarded in the article.
Other similar defects are uninteresting to be considered, and I shall confine myself to the message list:
There is an enormous number of ways how to get undefined or unspecified behavior in program written in C or C++. Therefore, PVS-Studio provides quite a lot of diagnostics aimed at identifying such problems: V567, V610, V611, V681, V704, V708, V726, V736.
In the case of XNU, the analyzer has identified only two weaknesses CWE-758, related to undefined behavior caused by a shift of negative numbers.
Fragment N53, N54
static void
pfr_prepare_network(union sockaddr_union *sa, int af, int net)
{
....
sa->sin.sin_addr.s_addr = net ? htonl(-1 << (32-net)) : 0;
....
}PVS-Studio warning: V610 CWE-758 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. pf_table.c 976
Shift of a negative number to the left leads to undefined behavior. In practice, this code may work well exactly as the programmer expects. But still, this code is incorrect and should be corrected. This can be done in the following way:
htonl((unsigned)(-1) << (32-net))PVS-Studio analyzer finds another shift here: V610 CWE-758 Undefined behavior. Check the shift operator '<<'. The left operand '-1' is negative. pf_table.c 983
XNU Kernel developers should be praised for the fact that the analyzer could not find any problems with memory leaks (CWE-401). There are only 3 suspicious places when the delete operator is not called when the object initialization error. While I'm not sure that this is an error.
Fragment N55, N56, N57
IOService * IODTPlatformExpert::createNub(IORegistryEntry * from)
{
IOService * nub;
nub = new IOPlatformDevice;
if (nub) {
if( !nub->init( from, gIODTPlane )) {
nub->free();
nub = 0;
}
}
return (nub);
}V773 CWE-401 The 'nub' pointer was assigned values twice without releasing the memory. A memory leak is possible. IOPlatformExpert.cpp 1287
If the function init is not able to initialize an object, possibly a memory leak will occur. In my opinion, it lacks the operator delete, and should have been written like this:
if( !nub->init( from, gIODTPlane )) {
nub->free();
delete nub;
nub = 0;
}I'm not sure that I'm right. Perhaps, the function free destroys the object itself, performing the operation "delete *this;". I didn't carefully sort all that out, because by the time I reached those warnings I was already tired.
Similar analyzer warnings:
The defect CWE-129 says that the variables, used for indexing of elements in the array, are incorrectly or insufficiently verified. Consequently, the array overrun may occur.
Fragment N58-N61
IOReturn
IOStateReporter::updateChannelValues(int channel_index)
{
....
state_index = _currentStates[channel_index];
if (channel_index < 0 ||
channel_index > (_nElements - state_index)
/ _channelDimension) {
result = kIOReturnOverrun; goto finish;
}
....
}PVS-Studio warning: V781 CWE-129 The value of the 'channel_index' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 852, 855. IOStateReporter.cpp 852
Negative values protection is implemented improperly. First, the element is retrieved from an array, and only after that, the check follows that the index isn't negative.
I think this code should be rewritten as follows:
IOReturn
IOStateReporter::updateChannelValues(int channel_index)
{
....
if (channel_index < 0)
{
result = kIOReturnOverrun; goto finish;
}
state_index = _currentStates[channel_index];
if (channel_index > (_nElements - state_index)
/ _channelDimension) {
result = kIOReturnOverrun; goto finish;
}
....
}You may need to add checks that the value channel_index is not greater than the size of the array. I'm not familiar with the code, so I'll leave it to the discretion of the XNU Kernel developers.
Similar errors:
CWE-480 defects are commonly related to some typos in expressions. There are usually not very much of them, but they are very fun. You just look at the errors and wonder how they could be done. However, as we have already demonstrated in the articles that no one is insured from such errors, even highly skilled programmers.
Fragment N62
#define NFS_UC_QUEUE_SLEEPING 0x0001
static void
nfsrv_uc_proxy(socket_t so, void *arg, int waitflag)
{
....
if (myqueue->ucq_flags | NFS_UC_QUEUE_SLEEPING)
wakeup(myqueue);
....
}PVS-Studio warning: V617 CWE-480 Consider inspecting the condition. The '0x0001' argument of the '|' bitwise operation contains a non-zero value. nfs_upcall.c 331
An essence "awakes" more often that it's needed. Rather, it "is woken" constantly, regardless of the conditions. Most likely, the code here is supposed to be as follows:
if (myqueue->ucq_flags & NFS_UC_QUEUE_SLEEPING)
wakeup(myqueue);PVS-Studio analyzer was unable to classify the following error according to CWE. From my point of view, we are dealing with CWE-665.
Fragment N63
extern void bzero(void *, size_t);
static struct thread thread_template, init_thread;
struct thread {
....
struct thread_qos_override {
struct thread_qos_override *override_next;
uint32_t override_contended_resource_count;
int16_t override_qos;
int16_t override_resource_type;
user_addr_t override_resource;
} *overrides;
....
};
void
thread_bootstrap(void)
{
....
bzero(&thread_template.overrides,
sizeof(thread_template.overrides));
....
}PVS-Studio warning: V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'thread_template.overrides' class object. thread.c 377
A programmer took the address of the variable, containing a pointer and nullified the variable, using the bzero function. In fact, just recorded nullptr in the pointer.
To use the bzero function is a very strange unnatural way to reset the value of the variable. It would be much easier to write:
thread_template.overrides = NULL;Hence, I conclude that a programmer wanted to reset the buffer, but occasionally nullified the pointer. Therefore, correct code should be like this:
bzero(thread_template.overrides,
sizeof(*thread_template.overrides));CWE-691 reveals anomalies in the sequence of instructions execution. Another anomaly is also possible - the code presentation doesn't correspond to the way it works. I faced exactly this case in the XNU Kernel code.
Fragment N64
Hooray, we got to the last code fragment! There may be other errors that I didn't notice when viewing the report, issued by the analyzer, but I'd like to remind that it was not my purpose to identify as many errors as possible. In any case, developers of the XNU Kernel will be able to study the report better, because they are familiar with the project code. So let's stop at the beautiful number 64 that is consonant with the name of our site viva64.
Note. For those who wonder where "viva64" came from, I suggest to get acquainted with the section "PVS-Studio project - 10 years of failures and successes.
void vm_page_release_startup(vm_page_t mem);
void
pmap_startup(
vm_offset_t *startp,
vm_offset_t *endp)
{
....
// -debug code remove
if (2 == vm_himemory_mode) {
for (i = 1; i <= pages_initialized; i++) {
....
}
}
else
// debug code remove-
/*
* Release pages in reverse order so that physical pages
* initially get allocated in ascending addresses. This keeps
* the devices (which must address physical memory) happy if
* they require several consecutive pages.
*/
for (i = pages_initialized; i > 0; i--) {
if(fill) fillPage(....);
vm_page_release_startup(&vm_pages[i - 1]);
}
....
}PVS-Studio warning: V705 CWE-691 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. vm_resident.c 1248
Perhaps there is no error here. However, I'm very confused by the keyword else. The code is formatted in such a way as if the loop is always executed. Actually the loop is executed only when the condition (2 == vm_himemory_mode) is false.
In the macOS world a new powerful static code PVS-Studio analyzer appeared that is able to detect errors and potential vulnerabilities in C, and C++. I invite everyone to try out our analyzer on your projects and to assess its abilities.
Thanks for your attention and don't forget to share the information with colleagues that PVS-Studio is now available for macOS.