PVS-Studio rummaged in the core of FreeBSD / PVS-Studio Blog

About a year ago, we were able to test the Linux kernel. This was one of the most talked-about articles about checking an open-source project of all time. Suggestions to pay attention to FreeBSD were actively received then, but only now there was enough time to do this.

About the checked project

FreeBSD is a modern operating system for servers, desktops and embedded computer platforms. Its code went through more than thirty years of a continuous process of development, improvement and optimization. It has established 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 FreeBSD is regularly checked by Coverity , I have no regrets that I worked with this project, as found a lot of suspicious places. About 40 pieces will be presented in the article, and for the developers (who received the verification report before the start of writing this article) I prepared a list of ~ 1000 serious warnings from the analyzer.

In my opinion, from the issued warnings of the analyzer, many places are real errors, but I can not say anything about criticality, because I am not an operating system developer. I think this is a good occasion for discussion 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 assembly for different platforms, but I only checked the kernel, which I put together like this:

make buildkernel KERNCONF=MYKERNEL

How to check

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

For convenience, I installed PC-BSD and wrote a small utility in C ++ that saved the working environment of compiler launches during kernel build. The obtained information was used to obtain preprocessed files and analyze them using PVS-Studio. This method allowed me to quickly check the project without studying an unfamiliar assembly system for integrating the analyzer. And checking the preprocessed files allows you to do 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 a similar way, and for Windows users this verification mode is available in the Standalone utility included in the PVS-Studio distribution kit. But for developers who want to integrate the analyzer into their project, usually there is no problem 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 the analyzer started, even before I assembled the kernel, because the assembly was interrupted by a link error. Going to the file indicated in the error, I saw the following:



Pay attention to the selected fragment. To format the indentation, a tab character is used and two operators are shifted under the condition. But the last statement is not really a condition and will always be executed. You may have forgotten to add braces here.

There was a comment to one article that we simply rewrite the analyzer warnings, but this is not so. Before checking the project, you need to make sure that it compiles correctly, and after receiving the report, analyzer warnings, you need to study / disassemble and explain 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 a real mistake.

Capy-poste and flakes

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



V501 There are identical sub-expressions '(uintptr_t) b-> handler' to the left and to the right of the '>' operator. 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 name variables briefly and uninformatively. Now, due to 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 to the right of the '! =' 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 fields of the structure is compared with itself, therefore, the result of this logical operation will always be 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 variable “PIM_NOBUSRESET” 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 with 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. Such a large piece of code was copied, 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 just 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;
    }
  }
  ....
}

A very strange code, if we simplify it conditionally, 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 different code here.

Another similar place:

• V571 Recurring check. This condition was already verified in line 1940. if_rl.c 1941

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, deciding that it was 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 such a macro:

#define osti_strtoul(nptr, endptr, base)    \
          strtoul((char *)nptr, (char **)endptr, 0)

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

V733 It is possible that macro expansion resulted 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 about this piece of code. Sometimes the value 'chan' is used, sometimes it is one less: 'chan - 1', but look at the definition of the 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 a binary expression is transmitted to a macro, the logic of calculations changes dramatically there. The alleged expression "(chan - 1) * 20" turns into "chan - 1 * 20", i.e. in "chan - 20", and then the program uses an incorrectly calculated size.

About Operations Priorities

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 test yourself by building a truth table of a logical expression.



V502 Perhaps the '?:' Operator works in a different way than it was expected. The '?:' Operator has a lower priority 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);
  }
  ....
}

Operator priority '?:' Below bitwise OR '|'. As a result, in bit operations, in addition to numerical constants, the result of the expression "(ctlr-> chip-> cfg1 == SWKS_100)" also takes part, which unexpectedly changes the logic of calculations. Perhaps, 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 works in a different way than it was expected. The '?:' Operator has a lower priority 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, because of which 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 excessive or contains a 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 piece of code is that the conditional expression is independent of the result "error == 0". Most likely, something is wrong here:



Three more cases:

• V590 Consider inspecting the 'error == 0 || error! = 35 'expression. The expression is excessive or contains a misprint. if_ipw.c 1855
• V590 Consider inspecting the 'error == 0 || error! = 27 'expression. The expression is excessive or contains a misprint. if_vmx.c 2747
• V547 Expression is always true. Probably the '&&' operator should be used here. igmp.c 1939

V590 Consider inspecting this expression. The expression is excessive or contains a 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's how it looks in the table:



In this chapter I have given as many as 3 ways to make mistakes in seemingly simple formulas. Think ...

V593 Consider reviewing the expression of the 'A = B! = C' kind. The expression is calculated as 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 spoils the error code when assignment is performed in the 'if' statement. Those. in the expression "error = copyin (...)! = 0" first, "copyin (...)! = 0" is calculated, and then the result (value 0 or 1) is written to 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 equal to '1' will be saved in the error code, and this is EPERM - a completely different error status.

Unfortunately, there are many such places:

• V593 Consider reviewing the expression of the 'A = B! = C' kind. The expression is calculated as following: 'A = (B! = C)'. aacraid.c 2861
• V593 Consider reviewing the expression of the 'A = B! = C' kind. The expression is calculated as following: 'A = (B! = C)'. if_age.c 591
• V593 Consider reviewing the expression of the 'A = B! = C' kind. The expression is calculated as following: 'A = (B! = C)'. if_alc.c 1535
• V593 Consider reviewing the expression of the 'A = B! = C' kind. The expression is calculated as following: 'A = (B! = C)'. if_ale.c 606
• V593 Consider reviewing the expression of the 'A = B! = C' kind. The expression is calculated as following: 'A = (B! = C)'. if_jme.c 807
• V593 Consider reviewing the expression of the 'A = B! = C' kind. The expression is calculated as following: 'A = (B! = C)'. if_msk.c 1626
• V593 Consider reviewing the expression of the 'A = B! = C' kind. The expression is calculated as following: 'A = (B! = C)'. if_stge.c 511
• V593 Consider reviewing the expression of the 'A = B! = C' kind. The expression is calculated as following: 'A = (B! = C)'. hunt_filter.c 973
• V593 Consider reviewing the expression of the 'A = B! = C' kind. The expression is calculated as following: 'A = (B! = C)'. if_smsc.c 1365
• V593 Consider reviewing the expression of the 'A = B! = C' kind. The expression is calculated as following: 'A = (B! = C)'. if_vte.c 431
• V593 Consider reviewing the expression of the 'A = B! = C' kind. The expression is calculated as following: 'A = (B! = C)'. zfs_vfsops.c 498

Lines

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 they 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. Everything seems to be simple.

To explain why an unexpected result will be obtained here, I will quote a simple and understandable example from the documentation for this diagnosis:

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, a line will be formed in the buffer:

N = 123, S = N = 123, S =

In other situations, a similar code can lead not only to display incorrect text, but also to crash the program. The code can be fixed by using a new buffer to save the result. The correct option:

char s1[100] = "test";
char s2[100];
sprintf(s2, "N = %d, S = %s", 123, s1);

V512 A call of the 'strcpy' function will lead to 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. Arrays do not have space for a null-terminal character , which can cause serious problems during 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 place for a line terminator, so you need to increase the value of SID_REVISION_SIZE at least by one.

Of course, it’s hard for me to judge this code. Perhaps terminal zero is not needed, and everything is designed for a certain 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:

• V583 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: td-> td_name. subr_turnstile.c 1112
• V583 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: td-> td_name. subr_turnstile.c 1196

Memory operations

In this section, I will talk about the misuse of the following functions:

void bzero(void *b, size_t len);

The bzero () function fills in the zeros of 'len' bytes at the pointer 'b'.

int copyout(const void *kaddr, void *uaddr, size_t len);

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

V579 The bzero function receives the pointer and its size as arguments. 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 nullify the structure, you must pass a pointer to the structure and the size of the resettable memory in bytes to the bzero () function, but here the size of the pointer is passed to the function, not the size of the structure.

The correct option should look like this:

bzero(&csio->sense_data, sizeof(csio->sense_data));

V579 The bzero function receives the pointer and its size as arguments. 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: the size of the pointer, not the object, was again passed to the 'bzero' function.

The correct option should look like this:

bzero(dst, sizeof(*dst));

V579 The copyout function receives the pointer and its size as arguments. 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 type 'u_char' is 1 byte in 32-bit and 64-bit applications, and the size of the type 'u_long' 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 we call ch [2] and ch [3], we go beyond the boundaries of the array.

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 using the 503rd diagnosis. It makes no practical sense to verify that the pointer value is greater than or equal to 0. Most likely, they forgot to dereference the sdno pointer here to compare the value stored there.

Two more pointer comparisons with zero:

• V503 This is a nonsensical comparison: pointer> = 0. geom_vinum_raid5.c 602
• V503 This is a nonsensical comparison: pointer> = 0. geom_vinum_raid5.c 610

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 null, then the function exits. But here it is not clear why try to dereference such a pointer “sc-> mrsas_dev”.

List of strange places:

• V522 Dereferencing of the null pointer 'sc' might take place. mrsas.c 1279
• V522 Dereferencing of the null pointer 'sc' might take place. tws_cam.c 1066
• V522 Dereferencing of the null pointer 'sc' might take place. blkfront.c 677
• V522 Dereferencing of the null pointer 'dev_priv' might take place. radeon_cs.c 153
• V522 Dereferencing of the null pointer 'ha' might take place. ql_isr.c 728

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 check "m == NULL" is in the wrong place. First, you need to check the pointer, and only then call the pfil_run_hooks () function.

Cycles

V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't 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);
  }
  ....
}

There was such an interesting and incorrect loop in the source code of FreeBSD. It is not known why, but here the decrement of the loop counter is done, instead of incrementing. It turns out that the loop can run much more than the value AE_IDLE_TIMEOUT until the 'break' statement is executed.

If the cycle is not stopped in time, then the sign variable 'i' will overflow. Overflowing a signed variable is nothing more than the undefined behavior of a program. And this is not an abstract theoretical danger, but a very real one. My colleague recently wrote an article on this topic: " Undefined behavior is closer than you think ."

Another interesting point. Exactly the same error was detected.me in the code of the Haiku operating system (see section "Warnings # 17, # 18"). I don’t know who borrowed the file “if_ae.c” from anyone, but the error is clearly propagated by copying :).

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++);
  }
  ....
}

This small code is most likely used to create a delay, only in total 10,000 iterations are performed here, and not 10 * 10,000, then why use two cycles?

I specifically cited this example, because it is most obvious when using one variable in external and nested loops leads to unexpected results.

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;
    }
  }
  ....
}

This is too complex an example to understand if the code is executing correctly. But according to the previous example, we can conclude that here, possibly, the wrong number of iterations is also performed.

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;
  ....
}

There are two dangerous cycles in this function. Because Since the variable 'j' (cycle counters) is of unsigned type, then the check "j> = 0" is always true and the cycles are "eternal". Another problem is that values ​​are constantly subtracted from this counter, therefore, if there is an attempt to overcome the zero value, then the variable 'j' will take the maximum type value.

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--);            // <=
  ....
}

A variable declaration was found in the body of the loop that matches the variable used to control the loop. I have a suspicion that due to the creation of a local pointer with the same name 'pst', the value of the external pointer with the name 'pst' does not change. Perhaps, in the condition of the do .... while () loop, the same value “pst-> cupid” is always checked. Developers need to double-check this place and be sure to give different names to the variables.

Miscellaneous

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 that is not used by any 'goto' statements. 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, they forgot to add the transition to the label or deleted it over time, but the label was accidentally left.

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;
  ....
}

In the end, I want to talk about the suspicious formatting that I already encountered at the very beginning of the project verification. Here the code is designed in such a way that the absence of the keyword 'else' looks suspicious.

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 { 
    ....
  }
  ....
}

There is no error in this code yet, but someday it will definitely appear. Leaving such a big comment in front of 'else', you can accidentally forget that somewhere there was this keyword, and unknowingly make an erroneous edit to the code in the future.

Conclusion

The FreeBSD project was tested with a special version of PVS-Studio, which showed excellent results! All the material received could not be fit into this article alone. However, the FreeBSD development team received the entire list of analyzer warnings that are worth paying attention to.

I suggest everyone to try PVS-Studio on their projects. The analyzer runs in a 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 for adapting 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 delved into the FreeBSD kernel .