Geekly Articles each Day
📜 ⬆️ ⬇️

PVS-Studio rummaged in the FreeBSD kernel

About a year ago we were able to test the Linux kernel. It was one of the most talked about checking open source projects of all time. Proposals to pay attention to FreeBSD were then actively received, but only now enough time has come to do it.

About the checked project


FreeBSD is a modern operating system for servers, desktops, and embedded computer platforms. Its code has gone through more than thirty years of continuous development, improvement and optimization. It has proven itself as a system for building intranets and Internet networks and servers. It provides reliable network services and efficient memory management.

Despite the fact that Coverity regularly checks FreeBSD, I don’t regret that I worked with this project because I found a lot of suspicious places. In the article about 40 of them will be presented, and for developers (who received a verification report even before writing this article) I prepared a list of ~ 1000 serious analyzer warnings.

In my opinion, many of the places issued by the analyzer warnings are real errors, but I can’t say anything about criticality, because I am not an operating system developer. I think this is a good occasion for discussions and communication with the authors of the project.
')
The source code for verification was taken from GitHub from the 'master' branch. The repository contains ~ 23000 files and two dozen configurations for building for different platforms, but I checked only the kernel, which I put together like this:

make buildkernel KERNCONF=MYKERNEL

How did you check


To check the kernel, we used the PVS-Studio static code analyzer version 6.01.

For convenience, I installed PC-BSD and wrote a small C ++ utility that preserved the working environment of compiler launches during the kernel build. The information obtained was used to obtain preprocessed files and analyze them using PVS-Studio. This method allowed me to quickly check the project without studying the assembly system unfamiliar to me to integrate the analyzer. And checking preprocessed files allows you to make a deeper analysis of the code and find more complex and interesting errors, for example, in macros. The article will provide several such examples.

The Linux kernel was tested in the same way, and for Windows users, this verification mode is available in the Standalone utility included in the PVS-Studio distribution. But for developers who want to integrate the analyzer into their project, usually there are no problems with this. They use some kind of integration described in the documentation. The advantage of monitoring utilities is that they allow you to quickly try the analyzer if the project has a non-standard assembly system.

Unusual luck


I found the first possible error even before starting the analyzer, even before I assembled the kernel, because the link was interrupted by the link. Moving to the file specified in the error, I saw the following:



Pay attention to the selection. For formatting indents, the tab character is used and two operators have been moved under the condition. But the last statement is not really related to the condition and will always be executed. Perhaps you forgot to add braces here.

One article had a comment that we simply rewrite analyzer warnings, but this is not the case. Before checking the project, you need to make sure that it is compiled correctly, and after receiving the report, the analyzer’s warnings must be examined / disassembled and explained to the reader. Exactly the same work is done by the analyzer user support team, responding to emails. It is not uncommon for users to send examples of false positives, in their opinion, but in reality this turns out to be the most real mistake.

Capy-poste and ochepyatki


The PVS-Studio analyzer is a powerful static analysis tool that finds a variety of errors in the code, but the first diagnostics were simple and made to find the most common errors associated with typos and copy-paste programming. When viewing the analyzer report, I sort it by error code and usually begin my story with this type of diagnostic rules.



V501 There are identical sub-expressions '(uintptr_t) b-> handler' to the left. ip_fw_sockopt.c 2893
 static int compare_sh(const void *_a, const void *_b) { const struct ipfw_sopt_handler *a, *b; a = (const struct ipfw_sopt_handler *)_a; b = (const struct ipfw_sopt_handler *)_b; .... if ((uintptr_t)a->handler < (uintptr_t)b->handler) return (-1); else if ((uintptr_t)b->handler > (uintptr_t)b->handler) // <= return (1); return (0); }

A small example of how harmful it is to call variables short and uninformative. Now, because of a typo in the letter 'b', part of the condition will never be fulfilled. Thus, the function returns a null status not always as intended.

V501 There are identical sub-expressions to the left and the right! ”= 'Operator: m-> m_pkthdr.len! = M-> m_pkthdr.len key.c 7208
 int key_parse(struct mbuf *m, struct socket *so) { .... if ((m->m_flags & M_PKTHDR) == 0 || m->m_pkthdr.len != m->m_pkthdr.len) { // <= .... goto senderror; } .... }

One of the structure fields is compared with itself, therefore, the result of this logical operation will always be equal to False.

V501 There are identical sub-expressions to the left and to the right of the '|' operator: PIM_NOBUSRESET | PIM_NOBUSRESET sbp_targ.c 1327
 typedef enum { PIM_EXTLUNS = 0x100, PIM_SCANHILO = 0x80, PIM_NOREMOVE = 0x40, PIM_NOINITIATOR = 0x20, PIM_NOBUSRESET = 0x10, // <= PIM_NO_6_BYTE = 0x08, PIM_SEQSCAN = 0x04, PIM_UNMAPPED = 0x02, PIM_NOSCAN = 0x01 } pi_miscflag; static void sbp_targ_action1(struct cam_sim *sim, union ccb *ccb) { .... struct ccb_pathinq *cpi = &ccb->cpi; cpi->version_num = 1; /* XXX??? */ cpi->hba_inquiry = PI_TAG_ABLE; cpi->target_sprt = PIT_PROCESSOR | PIT_DISCONNECT | PIT_TERM_IO; cpi->transport = XPORT_SPI; cpi->hba_misc = PIM_NOBUSRESET | PIM_NOBUSRESET; // <= .... }

In this example, the same PIM_NOBUSRESET variable is involved in the bit operation, which does not affect the result. Most likely they wanted to use a constant with a different value, but forgot to rename the variable.

V523 The 'then' statement is equivalent to the 'else' statement. saint.c 2023
 GLOBAL void siSMPRespRcvd(....) { .... if (agNULL == frameHandle) { /* indirect mode */ /* call back with success */ (*(ossaSMPCompletedCB_t)(pRequest->completionCB))(agRoot, pRequest->pIORequestContext, OSSA_IO_SUCCESS, payloadSize, frameHandle); } else { /* direct mode */ /* call back with success */ (*(ossaSMPCompletedCB_t)(pRequest->completionCB))(agRoot, pRequest->pIORequestContext, OSSA_IO_SUCCESS, payloadSize, frameHandle); } .... }

Two branches of the condition are signed by different comments: / * indirect mode * / and / * direct mode * /, but they are implemented in the same way, which is very suspicious.

V523 The 'then' statement is equivalent to the 'else' statement. smsat.c 2848
 osGLOBAL void smsatInquiryPage89(....) { .... if (oneDeviceData->satDeviceType == SATA_ATA_DEVICE) { pInquiry[40] = 0x01; /* LBA Low */ pInquiry[41] = 0x00; /* LBA Mid */ pInquiry[42] = 0x00; /* LBA High */ pInquiry[43] = 0x00; /* Device */ pInquiry[44] = 0x00; /* LBA Low Exp */ pInquiry[45] = 0x00; /* LBA Mid Exp */ pInquiry[46] = 0x00; /* LBA High Exp */ pInquiry[47] = 0x00; /* Reserved */ pInquiry[48] = 0x01; /* Sector Count */ pInquiry[49] = 0x00; /* Sector Count Exp */ } else { pInquiry[40] = 0x01; /* LBA Low */ pInquiry[41] = 0x00; /* LBA Mid */ pInquiry[42] = 0x00; /* LBA High */ pInquiry[43] = 0x00; /* Device */ pInquiry[44] = 0x00; /* LBA Low Exp */ pInquiry[45] = 0x00; /* LBA Mid Exp */ pInquiry[46] = 0x00; /* LBA High Exp */ pInquiry[47] = 0x00; /* Reserved */ pInquiry[48] = 0x01; /* Sector Count */ pInquiry[49] = 0x00; /* Sector Count Exp */ } .... }

This example is even more suspicious than the previous one. Copied such a large piece of code, but then no changes were made.

V547 Expression is always true. Probably the '&&' operator should be used here. qla_hw.c 799
 static int qla_tx_tso(qla_host_t *ha, struct mbuf *mp, ....) { .... if ((*tcp_opt != 0x01) || (*(tcp_opt + 1) != 0x01) || (*(tcp_opt + 2) != 0x08) || (*(tcp_opt + 2) != 10)) { // <= return -1; } .... }

Here, the analyzer found that the condition "(* (tcp_opt + 2)! = 0x08) || (* (tcp_opt + 2)! = 10)" is always true and this is true if you build a truth table. But, most likely, the '&&' operator is not needed here, but simply made a typo in the address offset. Perhaps the function code should have been like this:
 static int qla_tx_tso(qla_host_t *ha, struct mbuf *mp, ....) { .... if ((*tcp_opt != 0x01) || (*(tcp_opt + 1) != 0x01) || (*(tcp_opt + 2) != 0x08) || (*(tcp_opt + 3) != 10)) { return -1; } .... }

V571 Recurring check. This condition was already verified in line 1946. sahw.c 1949
 GLOBAL bit32 siHDAMode_V(....) { .... if( saRoot->memoryAllocated.agMemory[i].totalLength > biggest) { if(biggest < saRoot->memoryAllocated.agMemory[i].totalLength) { save = i; biggest = saRoot->memoryAllocated.agMemory[i].totalLength; } } .... }

Very strange code, if it is conditionally simplified, we will see the following:
 if( A > B ) { if (B < A) { .... } }

The same condition is checked twice in a row. Most likely, they wanted to write another code here.

Another similar place:

Dangerous Macros


V523 The 'then' statement is equivalent to the 'else' statement. agtiapi.c 829
 if (osti_strncmp(buffer, "0x", 2) == 0) { maxTargets = osti_strtoul (buffer, &pLastUsedChar, 0); AGTIAPI_PRINTK( ".... maxTargets = osti_strtoul 0 \n" ); } else { maxTargets = osti_strtoul (buffer, &pLastUsedChar, 10); AGTIAPI_PRINTK( ".... maxTargets = osti_strtoul 10\n" ); }

I first skipped this analyzer warning, having decided that this is a false positive. But after checking the project, false positives need to be studied and improved by the analyzer. What I did, after which I met this macro:
 #define osti_strtoul(nptr, endptr, base) \ strtoul((char *)nptr, (char **)endptr, 0)

The 'base' parameter is not used at all, and the value of '0' is always passed as the last parameter to the “strtoul” function. Although the values ​​in the macro are '0' and '10'. In the preprocessed file, all macros are expanded, and the code is the same. This macro is used in this way several dozen times. I sent the whole list of such places to the developers.

V733 It is possible that macro expansion in incorrect evaluation order. Check expression: chan - 1 * 20. isp.c 2301
 static void isp_fibre_init_2400(ispsoftc_t *isp) { .... if (ISP_CAP_VP0(isp)) off += ICB2400_VPINFO_PORT_OFF(chan); else off += ICB2400_VPINFO_PORT_OFF(chan - 1); // <= .... }

At first glance there is nothing suspicious in this code snippet. Sometimes the value 'chan' is used, sometimes one less: 'chan - 1', but let's look at the definition of a macro:
 #define ICB2400_VPOPT_WRITE_SIZE 20 #define ICB2400_VPINFO_PORT_OFF(chan) \ (ICB2400_VPINFO_OFF + \ sizeof (isp_icb_2400_vpinfo_t) + \ (chan * ICB2400_VPOPT_WRITE_SIZE)) // <=

When passing a binary expression to a macro, the calculation logic changes drastically there. The supposed expression "(chan - 1) * 20" turns into "chan - 1 * 20", i.e. in “chan - 20”, and then the wrong size is used in the program.

About priorities of operations


In this section, I will tell you how important it is to know the priorities of operations, use extra brackets if you are not sure, and sometimes check yourself by building a truth table of a logical expression.



V502 Perhaps the '?:' Operator was different. The '?:' Operator has a lower than the '|' operator. ata-serverworks.c 166
 ata_serverworks_chipinit(device_t dev) { .... pci_write_config(dev, 0x5a, (pci_read_config(dev, 0x5a, 1) & ~0x40) | (ctlr->chip->cfg1 == SWKS_100) ? 0x03 : 0x02, 1); } .... }

The priority of the operator '?:' Below bitwise OR '|'. As a result, the result of the expression "(ctlr-> chip-> cfg1 == SWKS_100)", which unexpectedly changes the logic of calculations, participates in bit operations, in addition to numeric constants. Probably, in this place a result similar to the truth is often obtained, therefore such an error has not yet been noticed.

V502 Perhaps the '?:' Operator was different. The '?:' Operator has a lower than the '|' operator. in6.c 1318
 void in6_purgeaddr(struct ifaddr *ifa) { .... error = rtinit(&(ia->ia_ifa), RTM_DELETE, ia->ia_flags | (ia->ia_dstaddr.sin6_family == AF_INET6) ? RTF_HOST : 0); .... }

In another file, there was also a place with a similar error with the ternary operator.

V547 Expression 'cdb [0]! = 0x28 || cdb [0]! = 0x2A 'is always true. Probably the '&&' operator should be used here. mfi_tbolt.c 1110
 int mfi_tbolt_send_frame(struct mfi_softc *sc, struct mfi_command *cm) { .... if (cdb[0] != 0x28 || cdb[0] != 0x2A) { // <=' if ((req_desc = mfi_tbolt_build_mpt_cmd(sc, cm)) == NULL) { device_printf(sc->mfi_dev, "Mapping from MFI " "to MPT Failed \n"); return 1; } } else device_printf(sc->mfi_dev, "DJA NA XXX SYSPDIO\n"); .... }

Here the first conditional expression is always true, which is why the 'else' branch never gets control. To prove the error in this and the following examples, I will give a truth table for controversial logical expressions. An example for this case:



V590 Consider inspecting the 'error == 0 || error! = - 1 'expression. The expression is misprint. nd6.c 2119
 int nd6_output_ifp(....) { .... /* Use the SEND socket */ error = send_sendso_input_hook(m, ifp, SND_OUT, ip6len); /* -1 == no app on SEND socket */ if (error == 0 || error != -1) // <= return (error); .... }

The problem with this code snippet is that the conditional expression does not depend on the result of “error == 0”. Most likely, something is wrong here:



Three more cases:
V590 Consider inspecting this expression. The expression is misprint. sig_verify.c 94
 enum uni_ieact { UNI_IEACT_CLEAR = 0x00, /* clear call */ .... } void uni_mandate_epref(struct uni *uni, struct uni_ie_epref *epref) { .... maxact = -1; FOREACH_ERR(e, uni) { if (e->ie == UNI_IE_EPREF) continue; if (e->act == UNI_IEACT_CLEAR) maxact = UNI_IEACT_CLEAR; else if (e->act == UNI_IEACT_MSG_REPORT) { if (maxact == -1 && maxact != UNI_IEACT_CLEAR) // <= maxact = UNI_IEACT_MSG_REPORT; } else if (e->act == UNI_IEACT_MSG_IGNORE) { if (maxact == -1) maxact = UNI_IEACT_MSG_IGNORE; } } .... }

Here the result of the entire conditional expression does not depend on the calculation of the value “maxact! = UNI_IEACT_CLEAR”. Here is how it looks in the table:



In this chapter I have given as many as 3 ways how you can be mistaken in seemingly simple formulas. Think about it ...

V593 Consider reviewing the A = B! = C 'kind. The expression is calculated as the following: 'A = (B! = C)'. aacraid.c 2854
 #define EINVAL 22 /* Invalid argument */ #define EFAULT 14 /* Bad address */ #define EPERM 1 /* Operation not permitted */ static int aac_ioctl_send_raw_srb(struct aac_softc *sc, caddr_t arg) { .... int error, transfer_data = 0; .... if ((error = copyin((void *)&user_srb->data_len, &fibsize, sizeof (u_int32_t)) != 0)) goto out; if (fibsize > (sc->aac_max_fib_size-sizeof(....))) { error = EINVAL; goto out; } if ((error = copyin((void *)user_srb, srbcmd, fibsize) != 0)) goto out; .... out: .... return(error); }

This function corrupts the error code when assignment is performed in the 'if' statement. Those. in the expression “error = copyin (...)! = 0”, first the “copyin (...)! = 0” is calculated, and then the result (value 0 or 1) is written into the variable 'error'.

The documentation for the 'copyin' function says that in case of an error, it returns the EFAULT code (value 14), and after such a check, the result of the logical operation, which is equal to '1', will be stored in the error code, and this is EPERM - a completely different error status.

Unfortunately, there are many such places:

Strings




V541 It is dangerous to print the string 'buffer' into itself. ata-highpoint.c 102
 static int ata_highpoint_probe(device_t dev) { .... char buffer[64]; .... strcpy(buffer, "HighPoint "); strcat(buffer, idx->text); if (idx->cfg1 == HPT_374) { if (pci_get_function(dev) == 0) strcat(buffer, " (channel 0+1)"); if (pci_get_function(dev) == 1) strcat(buffer, " (channel 2+3)"); } sprintf(buffer, "%s %s controller", buffer, ata_mode2str(idx->max_dma)); .... }

Here form a certain line in the buffer. Then they want to get a new line, keeping the previous value of the line, and add two more words to it. It seems simple.

To explain why an unexpected result will be obtained here, I will quote a simple and understandable example from the documentation for this diagnostic:
 char s[100] = "test"; sprintf(s, "N = %d, S = %s", 123, s);

As a result of this code, I want to get the line:
 N = 123, S = test

But in practice, the string will be formed in the buffer:
 N = 123, S = N = 123, S =

In other situations, a similar code can lead not only to the output of incorrect text, but also to the program crash. The code can be corrected by using a new buffer to save the result. Correct option:
 char s1[100] = "test"; char s2[100]; sprintf(s2, "N = %d, S = %s", 123, s1);

V512 A call to the 'strcpy' function will lead to the overflow of the buffer 'p-> vendor'. aacraid_cam.c 571
 #define SID_VENDOR_SIZE 8 char vendor[SID_VENDOR_SIZE]; #define SID_PRODUCT_SIZE 16 char product[SID_PRODUCT_SIZE]; #define SID_REVISION_SIZE 4 char revision[SID_REVISION_SIZE]; static void aac_container_special_command(struct cam_sim *sim, union ccb *ccb, u_int8_t *cmdp) { .... /* OEM Vendor defines */ strcpy(p->vendor,"Adaptec "); // <= strcpy(p->product,"Array "); // <= strcpy(p->revision,"V1.0"); // <= .... }

All three lines here are filled incorrectly. In arrays there is no place for a null-terminal character , which can cause serious problems with further work with such strings. In the case of "p-> vendor" and "p-> product" you can remove one space. Then the terminal zero will fit, which the strcpy () function adds to the end of the line. But for "p-> revision" there is absolutely no room for the end-of-line character, so you need to increase the value of SID_REVISION_SIZE by at least one.

I, of course, difficult to judge this code. Perhaps the terminal zero is not needed, and everything is designed for a specific buffer size. Then the strcpy () function is incorrectly selected. In this case, you should write something like this:
 memcpy(p->vendor, "Adaptec ", SID_VENDOR_SIZE); memcpy(p->product, "Array ", SID_PRODUCT_SIZE); memcpy(p->revision, "V1.0", SID_REVISION_SIZE);

V583 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: td-> td_name. subr_turnstile.c 1029
 static void print_thread(struct thread *td, const char *prefix) { db_printf("%s%p (tid %d, pid %d, ....", prefix, td, td->td_tid, td->td_proc->p_pid, td->td_name[0] != '\0' ? td->td_name : td->td_name); }

Suspicious place. Despite checking “td-> td_name [0]! = '\ 0'”, this line is still printed.

All such places:

Memory operations


In this section, I will discuss the misuse of the following functions:
 void bzero(void *b, size_t len);

The bzero () function fills zeros with 'len' bytes at the 'b' pointer.
 int copyout(const void *kaddr, void *uaddr, size_t len);

The copyout () function copies 'len' bytes from 'kaddr' to 'uaddr'.

It is a V579 . It is possibly a mistake. Inspect the second argument. osapi.c 316
 /* Autosense storage */ struct scsi_sense_data sense_data; void ostiInitiatorIOCompleted(....) { .... bzero(&csio->sense_data, sizeof(&csio->sense_data)); .... }

To reset the structure, the bzero () function must pass a pointer to the structure and size of the reset memory in bytes, but here the pointer size, not the structure size, is passed to the function.

The correct version should look like this:
 bzero(&csio->sense_data, sizeof(csio->sense_data));

It is a V579 . It is possibly a mistake. Inspect the second argument. acpi_package.c 83
 int acpi_PkgStr(...., void *dst, ....) { .... bzero(dst, sizeof(dst)); .... }

In this example, a similar situation: in the 'bzero' function the size of the pointer was transferred again, not of the object.

The correct version should look like this:
 bzero(dst, sizeof(*dst));

It is a V579 . It is possibly a mistake. Inspect the third argument. if_nxge.c 1498
 int xge_ioctl_stats(xge_lldev_t *lldev, struct ifreq *ifreqp) { .... *data = (*data == XGE_SET_BUFFER_MODE_1) ? 'Y':'N'; if(copyout(data, ifreqp->ifr_data, sizeof(data)) == 0) // <= retValue = 0; break; .... }

In this example, the memory is copied from 'data' to 'ifreqp-> ifr_data', while the size of the copied memory is equal to sizeof (data), i.e. 4 or 8 bytes depending on the capacity of the architecture.

Pointers




V557 Array overrun is possible. The '2' index is pointing beyond array bound. if_spppsubr.c 4348
 #define AUTHKEYLEN 16 struct sauth { u_short proto; /* authentication protocol to use */ u_short flags; #define AUTHFLAG_NOCALLOUT 1 /* callouts */ #define AUTHFLAG_NORECHALLENGE 2 /* do not re-challenge CHAP */ u_char name[AUTHNAMELEN]; /* system identification name */ u_char secret[AUTHKEYLEN]; /* secret password */ u_char challenge[AUTHKEYLEN]; /* random challenge */ }; static void sppp_chap_scr(struct sppp *sp) { u_long *ch, seed; u_char clen; /* Compute random challenge. */ ch = (u_long *)sp->myauth.challenge; read_random(&seed, sizeof seed); ch[0] = seed ^ random(); ch[1] = seed ^ random(); ch[2] = seed ^ random(); // <= ch[3] = seed ^ random(); // <= clen = AUTHKEYLEN; .... }

The size of the 'u_char' type is 1 byte in 32-bit and 64-bit applications, and the size of the 'u_long' type is 4 bytes in a 32-bit application and 8 bytes in a 64-bit application. Then, in a 32-bit application, when performing the operation “u_long * ch = (u_long *) sp-> myauth.challenge”, the array 'ch' will consist of 4 elements of 4 bytes each. And in a 64-bit application, the array 'ch' will consist of 2 elements of 8 bytes each. Therefore, if we assemble a 64-bit kernel, then when accessing ch [2] and ch [3] occurs, the output goes beyond the array boundaries.

V503 This is a nonsensical comparison: pointer> = 0. geom_vinum_plex.c 173
 gv_plex_offset(...., int *sdno, int growing) { .... *sdno = stripeno % sdcount; .... KASSERT(sdno >= 0, ("gv_plex_offset: sdno < 0")); .... }

A very interesting place was found with the help of the 503rd diagnostics. There is no practical reason to check that the pointer value is greater than or equal to 0. Most likely, they forgot to dereference the “sdno” pointer in order to compare the value stored there.

Two more pointer comparisons with zero:
V522 Dereferencing of the null pointer 'sc' might take place. mrsas.c 4027
 void mrsas_aen_handler(struct mrsas_softc *sc) { .... if (!sc) { device_printf(sc->mrsas_dev, "invalid instance!\n"); return; } if (sc->evt_detail_mem) { .... }

If the “sc” pointer is zero, the function is exited. , «sc->mrsas_dev».

:
V713 The pointer m was utilized in the logical expression before it was verified against nullptr in the same logical expression. ip_fastfwd.c 245
 struct mbuf * ip_tryforward(struct mbuf *m) { .... if (pfil_run_hooks( &V_inet_pfil_hook, &m, m->m_pkthdr.rcvif, PFIL_IN, NULL) || m == NULL) goto drop; .... }

The “m == NULL” check is in the wrong place. First you need to check the pointer, and only then call the function pfil_run_hooks ().

Cycles




V621 Consider inspecting the 'for' operator. It is possible that you will not be executed at all. if_ae.c 1663
 #define AE_IDLE_TIMEOUT 100 static void ae_stop_rxmac(ae_softc_t *sc) { int i; .... /* * Wait for IDLE state. */ for (i = 0; i < AE_IDLE_TIMEOUT; i--) { // <= val = AE_READ_4(sc, AE_IDLE_REG); if ((val & (AE_IDLE_RXMAC | AE_IDLE_DMAWRITE)) == 0) break; DELAY(100); } .... }

FreeBSD . , , , . , , AE_IDLE_TIMEOUT, 'break'.

, 'i'. , . , . : " Undefined behavior , ".

. Haiku (. « #17, #18»). , «if_ae.c», :).

V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 182, 183. mfi_tbolt.c 183
 mfi_tbolt_adp_reset(struct mfi_softc *sc) { .... for (i=0; i < 10; i++) { for (i = 0; i < 10000; i++); } .... }

, 10000 , 10*10000, ?

, .. , .

V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 197, 208. linux_vdso.c 208
 void __elfN(linux_vdso_reloc)(struct sysentvec *sv, long vdso_adjust) { .... for(i = 0; i < ehdr->e_shnum; i++) { // <= if (!(shdr[i].sh_flags & SHF_ALLOC)) continue; shdr[i].sh_addr += vdso_adjust; if (shdr[i].sh_type != SHT_SYMTAB && shdr[i].sh_type != SHT_DYNSYM) continue; sym = (Elf_Sym *)((caddr_t)ehdr + shdr[i].sh_offset); symcnt = shdr[i].sh_size / sizeof(*sym); for(i = 0; i < symcnt; i++, sym++) { // <= if (sym->st_shndx == SHN_UNDEF || sym->st_shndx == SHN_ABS) continue; sym->st_value += vdso_adjust; } } .... }

, , . , .

V547 Expression 'j >= 0' is always true. Unsigned type value is always >= 0. safe.c 1596
 static void safe_mcopy(struct mbuf *srcm, struct mbuf *dstm, u_int offset) { u_int j, dlen, slen; // <= caddr_t dptr, sptr; /* * Advance src and dst to offset. */ j = offset; while (j >= 0) { // <= if (srcm->m_len > j) break; j -= srcm->m_len; // <= srcm = srcm->m_next; if (srcm == NULL) return; } sptr = mtod(srcm, caddr_t) + j; slen = srcm->m_len - j; j = offset; while (j >= 0) { // <= if (dstm->m_len > j) break; j -= dstm->m_len; // <= dstm = dstm->m_next; if (dstm == NULL) return; } dptr = mtod(dstm, caddr_t) + j; dlen = dstm->m_len - j; .... }

. Since 'j' ( ) , «j >= 0» «». , , , , 'j' .

V711 It is dangerous to create a local variable within a loop with a same name as a variable controlling this loop. powernow.c 733
 static int pn_decode_pst(device_t dev) { .... struct pst_header *pst; // <= .... p = ((uint8_t *) psb) + sizeof(struct psb_header); pst = (struct pst_header*) p; maxpst = 200; do { struct pst_header *pst = (struct pst_header*) p; // <= .... p += sizeof(struct pst_header) + (2 * pst->numpstates); } while (cpuid_is_k7(pst->cpuid) && maxpst--); // <= .... }

, , . , - 'pst', 'pst' . , do....while() «pst->cupid». .

miscellanea


V569 Truncation of constant value -96. The value range of unsigned char type: [0, 255]. if_rsu.c 1516
 struct ieee80211_rx_stats { .... uint8_t nf; /* global NF */ uint8_t rssi; /* global RSSI */ .... }; static void rsu_event_survey(struct rsu_softc *sc, uint8_t *buf, int len) { .... rxs.rssi = le32toh(bss->rssi) / 2; rxs.nf = -96; .... }

It is very suspicious that the unsigned variable "rxs.nf" is assigned a negative value of '-96'. As a result, the variable will have the value '160'.

V729 Function body contains the 'done' label. zfs_acl.c 2023
 int zfs_setacl(znode_t *zp, vsecattr_t *vsecp, ....) { .... top: mutex_enter(&zp->z_acl_lock); mutex_enter(&zp->z_lock); .... if (error == ERESTART) { dmu_tx_wait(tx); dmu_tx_abort(tx); goto top; } .... done: // <= mutex_exit(&zp->z_lock); mutex_exit(&zp->z_acl_lock); return (error); }

The code contains functions that contain labels, but there is no call to the 'goto' operator for these labels. For example, in this code snippet, the label 'top' is used, but 'done' is not used anywhere. Perhaps, you have forgotten to add the transition to the label or have deleted it in time, and the label was left at random.

V646 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. mac_process.c 352
 static void mac_proc_vm_revoke_recurse(struct thread *td, struct ucred *cred, struct vm_map *map) { .... if (!mac_mmap_revocation_via_cow) { vme->max_protection &= ~VM_PROT_WRITE; vme->protection &= ~VM_PROT_WRITE; } if ((revokeperms & VM_PROT_READ) == 0) // <= vme->eflags |= MAP_ENTRY_COW | MAP_ENTRY_NEEDS_COPY; .... }

, . , 'else' .

V705 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. scsi_da.c 3231
 static void dadone(struct cam_periph *periph, union ccb *done_ccb) { .... /* * If we tried READ CAPACITY(16) and failed, * fallback to READ CAPACITY(10). */ if ((state == DA_CCB_PROBE_RC16) && .... } else // <= /* * Attach to anything that claims to be a * direct access or optical disk device, * as long as it doesn't return a "Logical * unit not supported" (0x25) error. */ if ((have_sense) && (asc != 0x25) // <= .... } else { .... } .... }

, - . 'else', , - , .

Conclusion




The FreeBSD project was tested with a special version of PVS-Studio, which showed excellent results! All the material received was impossible to fit in this one article. However, the FreeBSD development team has received the entire list of analyzer warnings that you should pay attention to.

I suggest everyone to try PVS-Studio on their projects. The analyzer works in the Windows environment. To use the analyzer in developing projects for Linux / FreeBSD, we do not have a public version. But we can discuss possible options for concluding a contract on the adaptation of PVS-Studio for your projects and tasks.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. PVS-Studio has delved into the FreeBSD kernel .

Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/277439/

More articles:


All Articles