In his abode of R'laih, the dead Cthulhu sleeps awaiting its time. In the C code of the DPDK project a myriad of errors sleeps awaiting their time. Let's see which ones the PVS-Studio analyzer can detect.
DPDK
Data Plane Development Kit is an open source project, which is a set of libraries for handling networks. DPDK was designed to move TCP/IP packet processing from an operating system kernel level to a user space. This approach virtually eliminates the use of system interrupts to access network data, which provides higher efficiency of utilized resources.
- Language: C.
- Git repository: DPDK.
Let's get straight to the errors. There's a lot of them.
Bug 1-10: bugs in unit tests
First ten bugs (or even more) relate to unit tests and were covered in the previous article "Finding Bugs in Unit Tests".
Bug 11-23: "insecure security" or disappearing memset
void nthw_hif_delete(nthw_hif_t *p)
{
if (p) {
memset(p, 0, sizeof(nthw_hif_t));
free(p);
}
}
PVS-Studio warning:
V597 The compiler could delete the 'memset' function call, which is used to flush 'p' object. The memset_s() function should be used to erase the private data. nthw_hif.c 27
Classic: incorrect filling of a buffer containing private data. Years go by, but security weaknesses of this type are still as common. The number of such errors that we have collected while checking open source projects has already exceeded 300.
The compiler sees that the zeroed buffer is then immediately freed. Due to the as-if rule, it can remove the memset call, since this optimization won't change the observed behavior of the program.
Private data, which the developer wanted to erase just in case, remains in memory.
If this is your first time encountering this type of defect, I recommend the following materials:
- A tale of how uncleared private data can suddenly be sent to the outside world over the network. Overwriting memory - why??
- How to erase the data anyway. Safe Clearing of Private Data.
- Common Weakness Enumeration. CWE-14: Compiler Removal of Code to Clear Buffers.
This defect occurs 12 more times in the project.
Bug 24-27: "insecure security, part 2", here comes memset again
I came across a function related to cryptography. It has a buffer that the author wants to flush before use, but due to a simple error, it won't happen. Random garbage data will remain in the buffer.
The point is that at the moment of calling the memset function, the state1_size variable specifying the number of bytes to be filled, remains equal to zero. Accordingly, memset fills zero bytes.
int qat_sym_cd_auth_set(....)
{
....
uint16_t state1_size = 0, state2_size = 0, cd_extra_size = 0;
....
switch (cdesc->qat_hash_alg) {
....
case ICP_QAT_HW_AUTH_ALGO_SHA3_224:
/* Plain SHA3-224 */
memset(cdesc->cd_cur_ptr, 0, state1_size); // <= BUG N1
state1_size = qat_hash_get_state1_size(
cdesc->qat_hash_alg);
break;
case ICP_QAT_HW_AUTH_ALGO_SHA3_256:
/* Plain SHA3-256 */
memset(cdesc->cd_cur_ptr, 0, state1_size); // <= BUG N2
state1_size = qat_hash_get_state1_size(
cdesc->qat_hash_alg);
break;
case ICP_QAT_HW_AUTH_ALGO_SHA3_384:
/* Plain SHA3-384 */
memset(cdesc->cd_cur_ptr, 0, state1_size); // <= BUG N3
state1_size = qat_hash_get_state1_size(
cdesc->qat_hash_alg);
break;
case ICP_QAT_HW_AUTH_ALGO_SHA3_512:
/* Plain SHA3-512 */
memset(cdesc->cd_cur_ptr, 0, state1_size); // <= BUG N4
state1_size = qat_hash_get_state1_size(
cdesc->qat_hash_alg);
break;
....
}
....
}
PVS-Studio warnings:
- V575 The 'memset' function processes '0' elements. Inspect the third argument. qat_sym_session.c 2622
- V575 The 'memset' function processes '0' elements. Inspect the third argument. qat_sym_session.c 2628
- V575 The 'memset' function processes '0' elements. Inspect the third argument. qat_sym_session.c 2634
- V575 The 'memset' function processes '0' elements. Inspect the third argument. qat_sym_session.c 2640
All four fragments are of the same type. Let's look again at the first of them:
memset(cdesc->cd_cur_ptr, 0, state1_size);
state1_size = qat_hash_get_state1_size(cdesc->qat_hash_alg);
I strongly suspect that these two lines are simply misplaced. First, we need to calculate the value of the state1_size variable and then use it when calling memset:
state1_size = qat_hash_get_state1_size(cdesc->qat_hash_alg);
memset(cdesc->cd_cur_ptr, 0, state1_size);
Bug 28: error in error handler
static int
parse_repr(const char *key __rte_unused, const char *value, void *args)
{
....
const char *str = value;
....
if (str == NULL) {
PMD_DRV_LOG(ERR, "wrong representor format: %s", str);
return -1;
}
....
}
PVS-Studio warning:
V576 Incorrect format. Consider checking the fifth actual argument of the 'rte_log' function. A null pointer is used. cpfl_ethdev.c 1583
Suppose a pointer to a string is null. A programmer will try to print it when forming an error message. It seems to be a bad idea.
Bug 29: forgot to write break
s32 e1000_set_mac_type(struct e1000_hw *hw)
{
....
switch (hw->device_id) {
....
case E1000_DEV_ID_PCH_ADL_I219_LM16:
case E1000_DEV_ID_PCH_ADL_I219_V16:
case E1000_DEV_ID_PCH_RPL_I219_LM23:
case E1000_DEV_ID_PCH_RPL_I219_V23:
mac->type = e1000_pch_tgp; // <=
case E1000_DEV_ID_PCH_ADL_I219_LM17:
case E1000_DEV_ID_PCH_ADL_I219_V17:
case E1000_DEV_ID_PCH_RPL_I219_LM22:
case E1000_DEV_ID_PCH_RPL_I219_V22:
mac->type = e1000_pch_adp; // <=
break;
case E1000_DEV_ID_82575EB_COPPER:
case E1000_DEV_ID_82575EB_FIBER_SERDES:
case E1000_DEV_ID_82575GB_QUAD_COPPER:
mac->type = e1000_82575;
break;
....
}
....
}
PVS-Studio warning:
V796 It is possible that 'break' statement is missing in switch statement. e1000_api.c 297
No need to comment. One of my favorite mistakes of C and C++ programmers.
Bug 30: file descriptor leak
static int
cnxk_gpio_read_attr(char *attr, char *val)
{
FILE *fp;
int ret;
fp = fopen(attr, "r");
if (!fp)
return -errno;
ret = fscanf(fp, "%s", val);
if (ret < 0)
return -errno; // <=
....
}
PVS-Studio warning:
V773 The function was exited without closing the file referenced by the 'fp' handle. A resource leak is possible. cnxk_gpio_selftest.c 46
If one opens the file but will not read a text line from it, the function will exit without closing the file descriptor. As a result:
- Resource leak.
- The file will remain open, which may prevent one from interacting with it somehow until the application terminates. This is perhaps even more critical than a resource leak.
Bugs 31-32: Copy-Paste in a condition
static int
ot_ipsec_sa_common_param_fill(....)
{
....
if (w2->s.enc_type == ROC_IE_OT_SA_ENC_AES_CBC ||
w2->s.enc_type == ROC_IE_OT_SA_ENC_AES_CCM ||
w2->s.enc_type == ROC_IE_OT_SA_ENC_AES_CTR ||
w2->s.enc_type == ROC_IE_OT_SA_ENC_AES_GCM ||
w2->s.enc_type == ROC_IE_OT_SA_ENC_AES_CCM ||
w2->s.auth_type == ROC_IE_OT_SA_AUTH_AES_GMAC) {
....
}
PVS-Studio warning:
V501 There are identical sub-expressions 'w2->s.enc_type == ROC_IE_OT_SA_ENC_AES_CCM' to the left and to the right of the '||' operator. cnxk_security.c 177
Most likely, the code was copied, so there is a double comparison with the ROC_IE_OT_OT_SA_ENC_AES_CCM constant. One check is either superfluous and can be removed, or there should be a comparison with another constant.
What's interesting is that Copy-Paste has showed itself once again. This code was reproduced, so here is exactly the same duplicated check below:
- V501 There are identical sub-expressions 'ctl->enc_type == ROC_IE_ON_SA_ENC_AES_CCM' to the left and to the right of the '||' operator. cnxk_security.c 885
Bug 33: a loop doesn't work
static int
nthw_pci_dev_init(struct rte_pci_device *pci_dev)
{
int num_port_speeds = 0;
....
// The value of the num_port_speeds variable does not change.
....
for (int i = 0; i < num_port_speeds; ++i) {
struct adapter_info_s *p_adapter_info = &p_nt_drv->adapter_info;
nt_link_speed_t link_speed = convert_link_speed(pls_mbps[i].link_speed);
port_ops->set_link_speed(p_adapter_info, i, link_speed);
}
....
}
PVS-Studio warning:
V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. ntnic_ethdev.c 537
The author forgot to change the value of the num_port_speeds variable somewhere in the code, so the loop will perform zero iterations.
Bug 34: I see why code review didn't help
static inline int
i40e_flow_fdir_fill_eth_ip_head(....)
{
....
} else if (cus_pctype->index == I40E_CUSTOMIZED_IPV4_L2TPV3) {
len = fill_ip4_head(fdir_input, raw_pkt, IPPROTO_L2TP,
len, ether_type);
} else if (cus_pctype->index == I40E_CUSTOMIZED_ESP_IPV4) {
len = fill_ip4_head(fdir_input, raw_pkt, IPPROTO_ESP,
len, ether_type);
} else if (cus_pctype->index == I40E_CUSTOMIZED_ESP_IPV4_UDP) {
len = fill_ip4_head(fdir_input, raw_pkt, IPPROTO_UDP,
len, ether_type);
} else if (cus_pctype->index == I40E_CUSTOMIZED_ESP_IPV4_UDP) {
len = fill_ip4_head(fdir_input, raw_pkt, IPPROTO_UDP,
len, ether_type);
} else if (cus_pctype->index == I40E_CUSTOMIZED_ESP_IPV6)
len = fill_ip6_head(fdir_input, raw_pkt, IPPROTO_ESP,
len, ether_type);
else if (cus_pctype->index == I40E_CUSTOMIZED_ESP_IPV6_UDP)
len = fill_ip6_head(fdir_input, raw_pkt, IPPROTO_UDP,
len, ether_type);
....
}
Do you see an error? I don't think you're into searching for it. Such code is boring to read, especially since it is only a part of the code with checks. That's why the error remains unnoticed.
} else if (cus_pctype->index == I40E_CUSTOMIZED_ESP_IPV4_UDP) {
len = fill_ip4_head(fdir_input, raw_pkt, IPPROTO_UDP, len, ether_type);
} else if (cus_pctype->index == I40E_CUSTOMIZED_ESP_IPV4_UDP) {
len = fill_ip4_head(fdir_input, raw_pkt, IPPROTO_UDP, len, ether_type);
The same thing is written twice. Actually, I'm not even sure what to advise to avoid such mistakes. It's good to have the PVS-Studio analyzer that helps you find errors even in boring code.
PVS-Studio warning:
V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 599, 602. i40e_fdir.c 599
Bug 35: strange check and possible array overrun
static const char *const sa_nthw_fpga_bus_type_str[] = {
"ERR", /* NTHW_FPGA_BUS_TYPE_UNKNOWN, */
"BAR", /* NTHW_FPGA_BUS_TYPE_BAR, */
"PCI", /* NTHW_FPGA_BUS_TYPE_PCI, */
"CCIP", /* NTHW_FPGA_BUS_TYPE_CCIP, */
"RAB0", /* NTHW_FPGA_BUS_TYPE_RAB0, */
"RAB1", /* NTHW_FPGA_BUS_TYPE_RAB1, */
"RAB2", /* NTHW_FPGA_BUS_TYPE_RAB2, */
"NMB", /* NTHW_FPGA_BUS_TYPE_NMB, */
"NDM", /* NTHW_FPGA_BUS_TYPE_NDM, */
};
static const char *get_bus_name(int n_bus_type_id)
{
if (n_bus_type_id >= 1 &&
n_bus_type_id <= (int)ARRAY_SIZE(sa_nthw_fpga_bus_type_str))
return sa_nthw_fpga_bus_type_str[n_bus_type_id];
else
return "ERR";
}
PVS-Studio warnings:
V557 Array overrun is possible. The value of 'n_bus_type_id' index could reach 9. nthw_fpga_model.c 32
The n_bus_type_id index is checked before extracting a row from an array. There are two questions to this check:
- Why is an index starting with 1 considered valid?
- Why is the right boundary checked using the <= operator? If the index is equal to the number of elements in the array, an Off-by-one Error will occur.
I would venture to guess that the ID values in the n_bus_type_id variable start with 1. This way, the mistake is that one forgot to subtract 1 before extracting an element from the array. In this case, the correct code will look like this:
static const char *get_bus_name(int n_bus_type_id)
{
if (n_bus_type_id >= 1 &&
n_bus_type_id <= (int)ARRAY_SIZE(sa_nthw_fpga_bus_type_str))
return sa_nthw_fpga_bus_type_str[n_bus_type_id - 1];
else
return "ERR";
}
I'm not sure, though. It's strange that no one noticed that the function returns the wrong lines. Perhaps the indexes are numbered from 0 after all. Then the check should be rewritten:
static const char *get_bus_name(int n_bus_type_id)
{
if (n_bus_type_id >= 0 &&
n_bus_type_id < (int)ARRAY_SIZE(sa_nthw_fpga_bus_type_str))
return sa_nthw_fpga_bus_type_str[n_bus_type_id];
else
return "ERR";
}
Please forgive my uncertainty. It's the first time when I see this code. The code is obviously incorrect, but unfortunately, I am limited in time to study each found error in more depth. There are dozens of them, and one of me.
Bug 35-37: a couple of Off-by-one Errors
/** Number of elements in the array. */
#define RTE_DIM(a) (sizeof (a) / sizeof ((a)[0]))
int
rte_devargs_layers_parse(struct rte_devargs *devargs,
const char *devstr)
{
struct {
const char *key;
const char *str;
struct rte_kvargs *kvlist;
} layers[] = {
{ RTE_DEVARGS_KEY_BUS "=", NULL, NULL, },
{ RTE_DEVARGS_KEY_CLASS "=", NULL, NULL, },
{ RTE_DEVARGS_KEY_DRIVER "=", NULL, NULL, },
};
....
if (nblayer > RTE_DIM(layers)) {
ret = -E2BIG;
goto get_out;
}
layers[nblayer].str = s;
....
}
PVS-Studio warning:
V557 Array overrun is possible. The value of 'nblayer' index could reach 3. eal_common_devargs.c 95
Another incorrect check:
if (nblayer > RTE_DIM(layers)) {
If nblayer == RTE_DIM(layers), the array will be overrun. Correct code version:
if (nblayer >= RTE_DIM(layers)) {
ret = -E2BIG;
goto get_out;
}
Similar cases:
- V557 Array overrun is possible. The value of 'nblayer' index could reach 3. eal_common_devargs.c 114
- V557 Array overrun is possible. The value of 'nblayer' index could reach 3. eal_common_devargs.c 116
Bug 38: a brilliant error - array overrun
The last array overrun is so beautifully difficult to detect during code review that I decided to write a special note about it. Hope you'll like it too: "Most striking error I found with PVS-Studio in 2024".
Bug 39-42: meaningless choice
Here are four similar cases. Same assignments regardless of a condition.
static uint32_t bnx2x_send_unload_req(struct bnx2x_softc *sc,
int unload_mode)
{
uint32_t reset_code = 0;
/* Select the UNLOAD request mode */
if (unload_mode == UNLOAD_NORMAL) {
reset_code = DRV_MSG_CODE_UNLOAD_REQ_WOL_DIS;
} else {
reset_code = DRV_MSG_CODE_UNLOAD_REQ_WOL_DIS;
}
....
}
PVS-Studio warning:
V523 The 'then' statement is equivalent to the 'else' statement. bnx2x.c 1633
static s32 txgbe_read_phy_if(struct txgbe_hw *hw)
{
....
if (!hw->phy.phy_semaphore_mask) {
if (hw->bus.lan_id)
hw->phy.phy_semaphore_mask = TXGBE_MNGSEM_SWPHY;
else
hw->phy.phy_semaphore_mask = TXGBE_MNGSEM_SWPHY;
}
return 0;
}
PVS-Studio warning:
V523 The 'then' statement is equivalent to the 'else' statement. txgbe_phy.c 87
int nthw_hif_init(nthw_hif_t *p, nthw_fpga_t *p_fpga, int n_instance)
{
....
p->mp_reg_build_seed = NULL; /* Reg/Fld not present on HIF */
if (p->mp_reg_build_seed)
p->mp_fld_build_seed = NULL; /* Reg/Fld not present on HIF */
else
p->mp_fld_build_seed = NULL;
....
}
PVS-Studio warning:
V523 The 'then' statement is equivalent to the 'else' statement. nthw_hif.c 90
I suspect that various constants should have been used, but the author's carelessness played a dirty trick with them. If it is written exactly as intended, the code clearly lacks explanatory comments.
And the last one is the most facepalm code version.
Note. It's an anniversary bug. It has become the 16000th in the bug collection on our website.
static int
otx_ep_eth_dev_uninit(struct rte_eth_dev *eth_dev)
{
if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
eth_dev->dev_ops = NULL;
eth_dev->rx_pkt_burst = NULL;
eth_dev->tx_pkt_burst = NULL;
return 0;
}
eth_dev->dev_ops = NULL;
eth_dev->rx_pkt_burst = NULL;
eth_dev->tx_pkt_burst = NULL;
return 0;
}
PVS-Studio warning:
V523 The 'then' statement is equivalent to the subsequent code fragment. otx_ep_ethdev.c 728
Bug 43: memory buffer compares to itself
static void
parse_sync(struct ptpv2_data_slave_ordinary *ptp_data,
uint16_t rx_tstamp_idx)
{
....
if (memcmp(&ptp_hdr->source_port_id.clock_id,
&ptp_hdr->source_port_id.clock_id,
sizeof(struct clock_id)) == 0) {
....
}
PVS-Studio warning:
V549 The first argument of 'memcmp' function is equal to the second argument. ptpclient.c 371
The programmer misprinted in the variable name so now the call of the memcmp function does not make sense.
Bug 44-54: bitmask formation error (it will be a tricky one)
#define HINIC_MAX_Q_FILTERS 64 /* hinic just support 64 filter types */
/* Structure to store filters' info. */
struct hinic_filter_info {
....
uint64_t type_mask; /* Bit mask for every used filter */
....
};
static inline void
hinic_ethertype_filter_remove(struct hinic_filter_info *filter_info,
uint8_t idx)
{
if (idx >= HINIC_MAX_Q_FILTERS)
return;
filter_info->pkt_type = 0;
filter_info->type_mask &= ~(1 << idx);
filter_info->pkt_filters[idx].pkt_proto = (uint16_t)0;
filter_info->pkt_filters[idx].enable = FALSE;
filter_info->pkt_filters[idx].qid = 0;
}
At first glance, everything seems fine. There is a 64-bit mask in which various bits are reset. By the way, you can first try to figure out yourself what's wrong here.
Let's get all the needed elements next to each other.
uint64_t type_mask; // 64-bit mask
uint8_t idx; // Bit number to be reset
// There is protection from a too large value
#define HINIC_MAX_Q_FILTERS 64
if (idx >= HINIC_MAX_Q_FILTERS) return;
filter_info->type_mask &= ~(1 << idx); // Bit reset
PVS-Studio warning:
V784 The size of the bit mask is less than the size of the first operand. This will cause the loss of higher bits. hinic_pmd_flow.c 2292
The point is that the character literal 1 is of the 32-bit int type! The result of the shift will also be of int type. Accordingly, it's not possible to get a mask for the 40th bit, for example. Strictly speaking, the behavior of such an operation is undefined if idx >= 32.
But let's stick to the case where idx < 32 for now. Although now the behavior of the shift operation is defined, it is still bad - the code sometimes corrupts high bits in the 64-bit mask. Let's say that idx is equal to 1. A programmer expects the following:
- shift: 1 << 1 = 2i32;
- inversion: ~2i32 = 0xFFFFFFFFFFFFFFFDu64;
- bit reset: 0xXXXXXXXXXXXXXXXXu64 & 0xFFFFFFFFFFFFFFFDu64.
What we will actually get:
- shift: 1 << 1 = 2i32;
- inversion: ~2i32 = 0xFFFFFFFFFDi32;
- implicit extension of 0xFFFFFFFFFFFDi32 to the uint64_t type. Since the sign bit is set, we get: 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFDu64;
- bit reset: 0xXXXXXXXXXXXXXXXXXXu64 & 0xFFFFFFFFFFFFFFFDu64.
It all seems to be the same..... Just a little longer.
No, watch the hands! Or rather, note the nuance that the sign bit turned out to be one. What if it's null? Then the high bits of the mask will be reset. See what happens if we want to reset the 32nd bit:
- shift: 1 << 31 = 0x80000000i32;
- inversion ~2i32 = 0x7FFFFFFFi32;
- implicit extension of 0x7FFFFFFFFFFFi32 to the uint64_t type. We get: 0x000000007FFFFFFFu64;
- resetting a set of bits: XXXXXXXXXXXXXXXXu64 & 0x00000000FFFFFFFDu64.
All the higher 32 bits in the 64-bit mask will be reset for nothing. This bug is particularly sneaky as it rarely manifests itself.
To fix the error, you should shift a unit of the uint64_t type:
filter_info->type_mask &= ~(1ULL << idx); // OK
filter_info->type_mask &= ~(uint64_t(1) << idx); // OK
10 more places where the mask is handled incorrectly.
Bug 55-56: one counter variable for two loops
First, I'll cite a big chunk of code to make sure that it's a bug, not a clever intention.
#define MAX_TLV_BUFFER 128 /* In dwords. 512 in bytes*/
typedef enum _lldp_agent_e {
LLDP_NEAREST_BRIDGE = 0,
LLDP_NEAREST_NON_TPMR_BRIDGE,
LLDP_NEAREST_CUSTOMER_BRIDGE,
LLDP_MAX_LLDP_AGENTS
} lldp_agent_e;
enum _ecore_status_t
ecore_lldp_mib_update_event(struct ecore_hwfn *p_hwfn,
struct ecore_ptt *p_ptt)
{
....
struct ecore_dcbx_mib_meta_data data;
enum _ecore_status_t rc = ECORE_SUCCESS;
struct lldp_received_tlvs_s tlvs;
int i;
for (i = 0; i < LLDP_MAX_LLDP_AGENTS; i++) {
OSAL_MEM_ZERO(&data, sizeof(data));
data.addr = p_hwfn->mcp_info->port_addr +
offsetof(struct public_port, lldp_received_tlvs[i]);
data.lldp_tlvs = &tlvs;
data.size = sizeof(tlvs);
rc = ecore_dcbx_copy_mib(p_hwfn, p_ptt, &data,
ECORE_DCBX_LLDP_TLVS);
if (rc != ECORE_SUCCESS) {
DP_NOTICE(p_hwfn, false, "Failed to read lldp TLVs\n");
return rc;
}
if (!tlvs.length)
continue;
for (i = 0; i < MAX_TLV_BUFFER; i++)
tlvs.tlvs_buffer[i] =
OSAL_CPU_TO_BE32(tlvs.tlvs_buffer[i]);
OSAL_LLDP_RX_TLVS(p_hwfn, tlvs.tlvs_buffer, tlvs.length);
}
return rc;
....
}
PVS-Studio warning:
V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 1368, 1384. ecore_dcbx.c 1384
You don't feel like figuring out the code? Apparently, neither did the developers when checking it. I'll shorten the code now, substitute the value of constants:
for (i = 0; i < 3; i++) {
....
for (i = 0; i < 128; i++) { .... }
....
}
When one variable is used for two loops, the outer loop can become an infinite loop. Here the developers got kind of "lucky". Or maybe not - if the loop were infinite, it would be noticed.
After the end of the inner loop, the variable i is 128. So the outer loop also immediately terminates after that, having executed only one iteration.
A similar bug lives here:
- V535 The variable 'i' is being used for this loop and for the outer loop. Check lines: 445, 455. nicvf_ethdev.c 455
Bug 57-58: operation precedence
static int
check_lcore_params(void)
{
....
if ((socketid = rte_lcore_to_socket_id(lcore) != 0) &&
(numa_on == 0)) {
printf("warning: lcore %u is on socket %d with numa off\n",
lcore, socketid);
}
....
}
PVS-Studio warning:
V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. main.c 1415
The programmer wanted to assign a value to a variable and then check it:
((socketid = rte_lcore_to_socket_id(lcore)) != 0)
But the operation precedence is such that the check is performed first and then the assignment:
(socketid = (rte_lcore_to_socket_id(lcore) != 0))
The value of the socketid variable will be corrupted. It will always be 0 or 1. This incorrect value can be both used to form a message (printf) and further in the code.
Similar error:
- V593 Consider reviewing the expression of the 'A = B != C' kind. The expression is calculated as following: 'A = (B != C)'. main.c 310
Bug 59: unknowable chthonic unspeakable horror
static void
bnx2x_storm_stats_post(struct bnx2x_softc *sc)
{
int rc;
if (!sc->stats_pending) {
if (sc->stats_pending)
return;
sc->fw_stats_req->hdr.drv_stats_counter =
htole16(sc->stats_counter++);
....
}
This code is so strange that PVS-Studio generates two warnings for it:
- V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 75, 76. bnx2x_stats.c 75
- V547 Expression 'sc->stats_pending' is always false. bnx2x_stats.c 76
What did a developer want? What is it?
if (!sc->stats_pending) {
if (sc->stats_pending)
return;
Perhaps stats_pending - is something crafty? No, it's just an ordinary variable:
/* tracking a pending STAT_QUERY ramrod */
uint16_t stats_pending;
It's clear that nothing is clear. Either a typo or an underwritten code.
So, where's the chthonic indescribable horror? It was waiting for me in the bnx2x_softc structure when I looked into its abyss to see how the stats_pending variable was declared.
Cthulhu in a world of structures. Don't write like this.
struct bnx2x_softc {
void **rx_queues;
void **tx_queues;
uint32_t max_tx_queues;
uint32_t max_rx_queues;
const struct rte_pci_device *pci_dev;
uint32_t pci_val;
struct bnx2x_pci_cap *pci_caps;
#define BNX2X_INTRS_POLL_PERIOD 1
void *firmware;
uint64_t fw_len;
/* MAC address operations */
struct bnx2x_mac_ops mac_ops;
/* structures for VF mbox/response/bulletin */
struct bnx2x_vf_mbx_msg *vf2pf_mbox;
struct bnx2x_dma vf2pf_mbox_mapping;
struct vf_acquire_resp_tlv acquire_resp;
struct bnx2x_vf_bulletin *pf2vf_bulletin;
struct bnx2x_dma pf2vf_bulletin_mapping;
struct bnx2x_vf_bulletin old_bulletin;
rte_spinlock_t vf2pf_lock;
int media;
int state; /* device state */
#define BNX2X_STATE_CLOSED 0x0000
#define BNX2X_STATE_OPENING_WAITING_LOAD 0x1000
#define BNX2X_STATE_OPENING_WAITING_PORT 0x2000
#define BNX2X_STATE_OPEN 0x3000
#define BNX2X_STATE_CLOSING_WAITING_HALT 0x4000
#define BNX2X_STATE_CLOSING_WAITING_DELETE 0x5000
#define BNX2X_STATE_CLOSING_WAITING_UNLOAD 0x6000
#define BNX2X_STATE_DISABLED 0xD000
#define BNX2X_STATE_DIAG 0xE000
#define BNX2X_STATE_ERROR 0xF000
int flags;
#define BNX2X_ONE_PORT_FLAG 0x1
#define BNX2X_NO_FCOE_FLAG 0x2
#define BNX2X_NO_WOL_FLAG 0x4
#define BNX2X_NO_MCP_FLAG 0x8
#define BNX2X_NO_ISCSI_OOO_FLAG 0x10
#define BNX2X_NO_ISCSI_FLAG 0x20
#define BNX2X_MF_FUNC_DIS 0x40
#define BNX2X_TX_SWITCHING 0x80
#define BNX2X_IS_VF_FLAG 0x100
#define BNX2X_ONE_PORT(sc) (sc->flags & BNX2X_ONE_PORT_FLAG)
#define BNX2X_NOFCOE(sc) (sc->flags & BNX2X_NO_FCOE_FLAG)
#define BNX2X_NOMCP(sc) (sc->flags & BNX2X_NO_MCP_FLAG)
#define MAX_BARS 5
struct bnx2x_bar bar[MAX_BARS]; /* map BARs 0, 2, 4 */
uint16_t doorbell_size;
/* periodic timer callout */
#define PERIODIC_STOP 0
#define PERIODIC_GO 1
volatile unsigned long periodic_flags;
rte_atomic32_t scan_fp;
struct bnx2x_fastpath fp[MAX_RSS_CHAINS];
struct bnx2x_sp_objs sp_objs[MAX_RSS_CHAINS];
uint8_t unit; /* driver instance number */
int pcie_bus; /* PCIe bus number */
int pcie_device; /* PCIe device/slot number */
int pcie_func; /* PCIe function number */
uint8_t pfunc_rel; /* function relative */
uint8_t pfunc_abs; /* function absolute */
uint8_t path_id; /* function absolute */
#define SC_PATH(sc) (sc->path_id)
#define SC_PORT(sc) (sc->pfunc_rel & 1)
#define SC_FUNC(sc) (sc->pfunc_rel)
#define SC_ABS_FUNC(sc) (sc->pfunc_abs)
#define SC_VN(sc) (sc->pfunc_rel >> 1)
#define SC_L_ID(sc) (SC_VN(sc) << 2)
#define PORT_ID(sc) SC_PORT(sc)
#define PATH_ID(sc) SC_PATH(sc)
#define VNIC_ID(sc) SC_VN(sc)
#define FUNC_ID(sc) SC_FUNC(sc)
#define ABS_FUNC_ID(sc) SC_ABS_FUNC(sc)
#define SC_FW_MB_IDX_VN(sc, vn) \
(SC_PORT(sc) + (vn) * \
((CHIP_IS_E1x(sc) || (CHIP_IS_MODE_4_PORT(sc))) ? 2 : 1))
#define SC_FW_MB_IDX(sc) SC_FW_MB_IDX_VN(sc, SC_VN(sc))
int if_capen; /* enabled interface capabilities */
struct bnx2x_devinfo devinfo;
char fw_ver_str[32];
char mf_mode_str[32];
char pci_link_str[32];
struct iro *iro_array;
int dmae_ready;
#define DMAE_READY(sc) (sc->dmae_ready)
struct ecore_credit_pool_obj vlans_pool;
struct ecore_credit_pool_obj macs_pool;
struct ecore_rx_mode_obj rx_mode_obj;
struct ecore_mcast_obj mcast_obj;
struct ecore_rss_config_obj rss_conf_obj;
struct ecore_func_sp_obj func_obj;
uint16_t fw_seq;
uint16_t fw_drv_pulse_wr_seq;
uint32_t func_stx;
struct elink_params link_params;
struct elink_vars link_vars;
uint32_t link_cnt;
struct bnx2x_link_report_data last_reported_link;
char mac_addr_str[32];
uint32_t tx_ring_size;
uint32_t rx_ring_size;
int wol;
int is_leader;
int recovery_state;
#define BNX2X_RECOVERY_DONE 1
#define BNX2X_RECOVERY_INIT 2
#define BNX2X_RECOVERY_WAIT 3
#define BNX2X_RECOVERY_FAILED 4
#define BNX2X_RECOVERY_NIC_LOADING 5
uint32_t rx_mode;
#define BNX2X_RX_MODE_NONE 0
#define BNX2X_RX_MODE_NORMAL 1
#define BNX2X_RX_MODE_ALLMULTI 2
#define BNX2X_RX_MODE_ALLMULTI_PROMISC 3
#define BNX2X_RX_MODE_PROMISC 4
#define BNX2X_MAX_MULTICAST 64
struct bnx2x_port port;
struct cmng_init cmng;
/* user configs */
uint8_t num_queues;
int hc_rx_ticks;
int hc_tx_ticks;
uint32_t rx_budget;
int interrupt_mode;
#define INTR_MODE_INTX 0
#define INTR_MODE_MSI 1
#define INTR_MODE_MSIX 2
#define INTR_MODE_SINGLE_MSIX 3
int udp_rss;
uint8_t igu_dsb_id;
uint8_t igu_base_sb;
uint8_t igu_sb_cnt;
uint32_t igu_base_addr;
uint8_t base_fw_ndsb;
#define DEF_SB_IGU_ID 16
#define DEF_SB_ID HC_SP_SB_ID
/* default status block */
struct bnx2x_dma def_sb_dma;
struct host_sp_status_block *def_sb;
uint16_t def_idx;
uint16_t def_att_idx;
uint32_t attn_state;
struct attn_route attn_group[MAX_DYNAMIC_ATTN_GRPS];
/* general SP events - stats query, cfc delete, etc */
#define HC_SP_INDEX_ETH_DEF_CONS 3
/* EQ completions */
#define HC_SP_INDEX_EQ_CONS 7
/* FCoE L2 connection completions */
#define HC_SP_INDEX_ETH_FCOE_TX_CQ_CONS 6
#define HC_SP_INDEX_ETH_FCOE_RX_CQ_CONS 4
/* iSCSI L2 */
#define HC_SP_INDEX_ETH_ISCSI_CQ_CONS 5
#define HC_SP_INDEX_ETH_ISCSI_RX_CQ_CONS 1
/* event queue */
struct bnx2x_dma eq_dma;
union event_ring_elem *eq;
uint16_t eq_prod;
uint16_t eq_cons;
uint16_t *eq_cons_sb;
#define NUM_EQ_PAGES 1 /* must be a power of 2 */
#define EQ_DESC_CNT_PAGE (BNX2X_PAGE_SIZE / sizeof(union event_ring_elem))
#define EQ_DESC_MAX_PAGE (EQ_DESC_CNT_PAGE - 1)
#define NUM_EQ_DESC (EQ_DESC_CNT_PAGE * NUM_EQ_PAGES)
#define EQ_DESC_MASK (NUM_EQ_DESC - 1)
#define MAX_EQ_AVAIL (EQ_DESC_MAX_PAGE * NUM_EQ_PAGES - 2)
/* depends on EQ_DESC_CNT_PAGE being a power of 2 */
#define NEXT_EQ_IDX(x) \
((((x) & EQ_DESC_MAX_PAGE) == (EQ_DESC_MAX_PAGE - 1)) ? \
((x) + 2) : ((x) + 1))
/* depends on the above and on NUM_EQ_PAGES being a power of 2 */
#define EQ_DESC(x) ((x) & EQ_DESC_MASK)
/* slow path */
struct bnx2x_dma sp_dma;
struct bnx2x_slowpath *sp;
uint32_t sp_state;
/* slow path queue */
struct bnx2x_dma spq_dma;
struct eth_spe *spq;
#define SP_DESC_CNT (BNX2X_PAGE_SIZE / sizeof(struct eth_spe))
#define MAX_SP_DESC_CNT (SP_DESC_CNT - 1)
#define MAX_SPQ_PENDING 8
uint16_t spq_prod_idx;
struct eth_spe *spq_prod_bd;
struct eth_spe *spq_last_bd;
uint16_t *dsb_sp_prod;
volatile unsigned long eq_spq_left; /* COMMON_xxx ramrod credit */
volatile unsigned long cq_spq_left; /* ETH_xxx ramrod credit */
/* fw decompression buffer */
struct bnx2x_dma gz_buf_dma;
void *gz_buf;
uint32_t gz_outlen;
#define GUNZIP_BUF(sc) (sc->gz_buf)
#define GUNZIP_OUTLEN(sc) (sc->gz_outlen)
#define GUNZIP_PHYS(sc) (rte_iova_t)(sc->gz_buf_dma.paddr)
#define FW_BUF_SIZE 0x40000
struct raw_op *init_ops;
uint16_t *init_ops_offsets; /* init block offsets inside init_ops */
uint32_t *init_data; /* data blob, 32 bit granularity */
uint32_t init_mode_flags;
#define INIT_MODE_FLAGS(sc) (sc->init_mode_flags)
/* PRAM blobs - raw data */
const uint8_t *tsem_int_table_data;
const uint8_t *tsem_pram_data;
const uint8_t *usem_int_table_data;
const uint8_t *usem_pram_data;
const uint8_t *xsem_int_table_data;
const uint8_t *xsem_pram_data;
const uint8_t *csem_int_table_data;
const uint8_t *csem_pram_data;
#define INIT_OPS(sc) (sc->init_ops)
#define INIT_OPS_OFFSETS(sc) (sc->init_ops_offsets)
#define INIT_DATA(sc) (sc->init_data)
#define INIT_TSEM_INT_TABLE_DATA(sc) (sc->tsem_int_table_data)
#define INIT_TSEM_PRAM_DATA(sc) (sc->tsem_pram_data)
#define INIT_USEM_INT_TABLE_DATA(sc) (sc->usem_int_table_data)
#define INIT_USEM_PRAM_DATA(sc) (sc->usem_pram_data)
#define INIT_XSEM_INT_TABLE_DATA(sc) (sc->xsem_int_table_data)
#define INIT_XSEM_PRAM_DATA(sc) (sc->xsem_pram_data)
#define INIT_CSEM_INT_TABLE_DATA(sc) (sc->csem_int_table_data)
#define INIT_CSEM_PRAM_DATA(sc) (sc->csem_pram_data)
#define PHY_FW_VER_LEN 20
char fw_ver[32];
/* ILT
* For max 196 cids (64*3 + non-eth), 32KB ILT page size and 1KB
* context size we need 8 ILT entries.
*/
#define ILT_MAX_L2_LINES 8
struct hw_context context[ILT_MAX_L2_LINES];
struct ecore_ilt *ilt;
#define ILT_MAX_LINES 256
/* max supported number of RSS queues: IGU SBs minus one for CNIC */
#define BNX2X_MAX_RSS_COUNT(sc) ((sc)->igu_sb_cnt - CNIC_SUPPORT(sc))
/* max CID count: Max RSS * Max_Tx_Multi_Cos + FCoE + iSCSI */
#define BNX2X_L2_MAX_CID(sc) \
(BNX2X_MAX_RSS_COUNT(sc) * ECORE_MULTI_TX_COS + 2 * CNIC_SUPPORT(sc))
#define BNX2X_L2_CID_COUNT(sc) \
(BNX2X_NUM_ETH_QUEUES(sc) * ECORE_MULTI_TX_COS + 2 * CNIC_SUPPORT(sc))
#define L2_ILT_LINES(sc) \
(DIV_ROUND_UP(BNX2X_L2_CID_COUNT(sc), ILT_PAGE_CIDS))
int qm_cid_count;
uint8_t dropless_fc;
/* total number of FW statistics requests */
uint8_t fw_stats_num;
/*
* This is a memory buffer that will contain both statistics ramrod
* request and data.
*/
struct bnx2x_dma fw_stats_dma;
/*
* FW statistics request shortcut (points at the beginning of fw_stats
* buffer).
*/
int fw_stats_req_size;
struct bnx2x_fw_stats_req *fw_stats_req;
rte_iova_t fw_stats_req_mapping;
/*
* FW statistics data shortcut (points at the beginning of fw_stats
* buffer + fw_stats_req_size).
*/
int fw_stats_data_size;
struct bnx2x_fw_stats_data *fw_stats_data;
rte_iova_t fw_stats_data_mapping;
/* tracking a pending STAT_QUERY ramrod */
uint16_t stats_pending;
/* number of completed statistics ramrods */
uint16_t stats_comp;
uint16_t stats_counter;
uint8_t stats_init;
int stats_state;
struct bnx2x_eth_stats eth_stats;
struct host_func_stats func_stats;
struct bnx2x_eth_stats_old eth_stats_old;
struct bnx2x_net_stats_old net_stats_old;
struct bnx2x_fw_port_stats_old fw_stats_old;
struct dmae_command stats_dmae; /* used by dmae command loader */
int executer_idx;
int mtu;
/* DCB support on/off */
int dcb_state;
#define BNX2X_DCB_STATE_OFF 0
#define BNX2X_DCB_STATE_ON 1
/* DCBX engine mode */
int dcbx_enabled;
#define BNX2X_DCBX_ENABLED_OFF 0
#define BNX2X_DCBX_ENABLED_ON_NEG_OFF 1
#define BNX2X_DCBX_ENABLED_ON_NEG_ON 2
#define BNX2X_DCBX_ENABLED_INVALID -1
uint8_t cnic_support;
uint8_t cnic_enabled;
uint8_t cnic_loaded;
#define CNIC_SUPPORT(sc) 0 /* ((sc)->cnic_support) */
#define CNIC_ENABLED(sc) 0 /* ((sc)->cnic_enabled) */
#define CNIC_LOADED(sc) 0 /* ((sc)->cnic_loaded) */
/* multiple tx classes of service */
uint8_t max_cos;
#define BNX2X_MAX_PRIORITY 8
/* priority to cos mapping */
uint8_t prio_to_cos[BNX2X_MAX_PRIORITY];
int panic;
/* Array of Multicast addrs */
struct rte_ether_addr mc_addrs[VF_MAX_MULTICAST_PER_VF];
/* Multicast mac addresses number */
uint16_t mc_addrs_num;
}; /* struct bnx2x_softc */
Bug 60-64: reckless pointer handling
The analyzer often issues V595 warnings—the pointer is dereferenced before checking. DPDK is no exception. However, bugs may have different essence. Let's start with a rare variety.
int qbman_fq_query(struct qbman_swp *s, uint32_t fqid,
struct qbman_fq_query_rslt *r)
{
struct qbman_fq_query_desc *p;
p = (struct qbman_fq_query_desc *)qbman_swp_mc_start(s);
if (!p)
return -EBUSY;
p->fqid = fqid;
*r = *(struct qbman_fq_query_rslt *)qbman_swp_mc_complete(s, p,
QBMAN_FQ_QUERY);
if (!r) {
pr_err("qbman: Query FQID %d failed, no response\n",
fqid);
return -EIO;
}
....
}
PVS-Studio warning:
V595 The 'r' pointer was utilized before it was verified against nullptr. Check lines: 211, 213. qbman_debug.c 211
Let's look at these lines more closely:
*r = *(struct qbman_fq_query_rslt *)
qbman_swp_mc_complete(s, p, QBMAN_FQ_QUERY);
if (!r) {
The function returns a pointer to a structure. The value of this structure is immediately copied over a pointer r. Then comes an attempt to check if the function returned NULL, but it was written incorrectly:
- The structure itself is copied instead of a pointer. There is no point in checking the pointer r. This is not the pointer that the function returned.
- By the time the check is done, the data has already been copied. So if the function returns NULL, a segfault will occur.
To remedy the situation, one should use an intermediate pointer:
p->fqid = fqid;
qbman_fq_query_rslt *tmp =
(struct qbman_fq_query_rslt *)
qbman_swp_mc_complete(s, p, QBMAN_FQ_QUERY);
if (tmp) {
pr_err("qbman: Query FQID %d failed, no response\n",
fqid);
return -EIO;
}
*r = *tmp;
There are four more mistakes like that.
Bug 65-71: pointer dereference before a check
int
roc_bphy_cgx_set_link_mode(struct roc_bphy_cgx *roc_cgx, unsigned int lmac,
struct roc_bphy_cgx_link_mode *mode)
{
uint64_t scr1, scr0;
if (roc_model_is_cn9k() &&
(mode->use_portm_idx || mode->portm_idx || mode->mode_group_idx)) { // <=
return -ENOTSUP;
}
if (!roc_cgx)
return -EINVAL;
if (!roc_bphy_cgx_lmac_exists(roc_cgx, lmac))
return -ENODEV;
if (!mode) // <=
return -EINVAL;
....
}
PVS-Studio warning:
V595 The 'mode' pointer was utilized before it was verified against nullptr. Check lines: 373, 383. roc_bphy_cgx.c 373
First the mode pointer is dereferenced. Then suddenly it turns out that it may actually be null and needs to be checked. But it's too late.
Similar cases.
Bug 72: when pointers drop from your hands
This case is similar to the previous one, but here the pointer is checked even more ineptly. In some sense, there are even two bugs here instead of one, that's why PVS-Studio issues two warnings.
static int
eth_dev_close(struct rte_eth_dev *eth_dev)
{
struct pmd_internals *internals =
(struct pmd_internals *)eth_dev->data->dev_private;
struct drv_s *p_drv = internals->p_drv;
internals->p_drv = NULL;
rte_eth_dev_release_port(eth_dev);
/* decrease initialized ethernet devices */
p_drv->n_eth_dev_init_count--;
/*
* rte_pci_dev has no private member for p_drv
* wait until all rte_eth_dev's are closed - then close adapters via p_drv
*/
if (!p_drv->n_eth_dev_init_count && p_drv)
drv_deinit(p_drv);
return 0;
}
First, the p_drv pointer is boldly used at the beginning of the function and then checked:
p_drv->n_eth_dev_init_count--;
if (.... && p_drv)
First warning:
V595 The 'p_drv' pointer was utilized before it was verified against nullptr. Check lines: 389, 395. ntnic_ethdev.c 389
Second, the author made a mistake even in the condition where the pointer is checked:
if (!p_drv->n_eth_dev_init_count && p_drv)
Again, first dereferencing, then checking:
V713 The pointer 'p_drv' was utilized in the logical expression before it was verified against nullptr in the same logical expression. ntnic_ethdev.c 395
Bug 73: unreliable synchronization
struct __rte_cache_aligned vhost_crypto_info {
....
uint32_t nb_inflight_ops;
....
};
static void
destroy_device(int vid)
{
struct vhost_crypto_info *info = NULL;
....
info = options.infos[i];
....
do {
} while (info->nb_inflight_ops);
info->initialized[j] = 0;
....
}
PVS-Studio warning:
V712 Be advised that compiler may delete this cycle or make it infinity. Use volatile variable(s) or synchronization primitives to avoid this. main.c 353
It is a bad idea to synchronize using an ordinary variable of the uint32_t type. This code may or may not work. For example, for optimization purposes, the compiler may check a variable. And, if it is not equal to zero, the compiler will generate an infinite loop.
Bug 74-75: duplicate
static inline int
vrb_dma_desc_ld_fill(struct rte_bbdev_dec_op *op, ....)
{
....
if (op->ldpc_dec.harq_combined_input.data == 0) {
rte_bbdev_log(ERR, "HARQ input is not defined");
return -1;
}
h_p_size = fcw->hcin_size0 + fcw->hcin_size1;
if (fcw->hcin_decomp_mode == 1)
h_p_size = (h_p_size * 3 + 3) / 4;
else if (fcw->hcin_decomp_mode == 4)
h_p_size = h_p_size / 2;
if (op->ldpc_dec.harq_combined_input.data == 0) {
rte_bbdev_log(ERR, "HARQ input is not defined");
return -1;
}
....
}
PVS-Studio warning:
V547 Expression is always false. rte_vrb_pmd.c 1861
Basically, it's not a scary thing. This code snippet is just duplicated twice:
if (op->ldpc_dec.harq_combined_input.data == 0) {
rte_bbdev_log(ERR, "HARQ input is not defined");
return -1;
}
In this case, the value of the checked variable does not change between fragments. So one of the fragments can simply be deleted. Unless one of them didn't actually mean to check something else, but typoed it.
Similar:
- V547 Expression '!npa_lf' is always false. roc_nix_debug.c 770
Bug 76: with and without a check
static void
vmxnet3_dev_tx_queue_reset(void *txq)
{
vmxnet3_tx_queue_t *tq = txq;
....
if (tq != NULL) {
/* Release the cmd_ring mbufs */
vmxnet3_tx_cmd_ring_release_mbufs(&tq->cmd_ring);
}
....
size += tq->txdata_desc_size * data_ring->size;
....
}
PVS-Studio warning:
V1004 The 'tq' pointer was used unsafely after it was verified against nullptr. Check lines: 213, 227. vmxnet3_rxtx.c 227
The tq pointer is used after a preliminary check tq != NULL. So it can be null. But further in the code the author forgot about it and the pointer is used tq->txdata_desc_size without checking.
Bug 77: code that is too lazy to be checked, so it wasn't checked
#define RTE_ETH_SPEED_NUM_2_5G 2500 /**< 2.5 Gbps */
#define RTE_ETH_SPEED_NUM_5G 5000 /**< 5 Gbps */
static uint32_t
tap_dev_speed_capa(void)
{
uint32_t speed = pmd_link.link_speed;
uint32_t capa = 0;
if (speed >= RTE_ETH_SPEED_NUM_10M)
capa |= RTE_ETH_LINK_SPEED_10M;
if (speed >= RTE_ETH_SPEED_NUM_100M)
capa |= RTE_ETH_LINK_SPEED_100M;
if (speed >= RTE_ETH_SPEED_NUM_1G)
capa |= RTE_ETH_LINK_SPEED_1G;
if (speed >= RTE_ETH_SPEED_NUM_5G)
capa |= RTE_ETH_LINK_SPEED_2_5G;
if (speed >= RTE_ETH_SPEED_NUM_5G)
capa |= RTE_ETH_LINK_SPEED_5G;
if (speed >= RTE_ETH_SPEED_NUM_10G)
capa |= RTE_ETH_LINK_SPEED_10G;
if (speed >= RTE_ETH_SPEED_NUM_20G)
capa |= RTE_ETH_LINK_SPEED_20G;
if (speed >= RTE_ETH_SPEED_NUM_25G)
capa |= RTE_ETH_LINK_SPEED_25G;
if (speed >= RTE_ETH_SPEED_NUM_40G)
capa |= RTE_ETH_LINK_SPEED_40G;
if (speed >= RTE_ETH_SPEED_NUM_50G)
capa |= RTE_ETH_LINK_SPEED_50G;
if (speed >= RTE_ETH_SPEED_NUM_56G)
capa |= RTE_ETH_LINK_SPEED_56G;
if (speed >= RTE_ETH_SPEED_NUM_100G)
capa |= RTE_ETH_LINK_SPEED_100G;
return capa;
}
PVS-Studio warning:
V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 997, 999. rte_eth_tap.c 999
This kind of code is hard to check during code review. So we get a typo as a result.
if (speed >= RTE_ETH_SPEED_NUM_5G)
capa |= RTE_ETH_LINK_SPEED_2_5G;
if (speed >= RTE_ETH_SPEED_NUM_5G)
capa |= RTE_ETH_LINK_SPEED_5G;
The same constant is compared twice, but different bit flags are set. Most likely, the author intended to use the constant _RTE_ETH_SPEED_NUM_2_5G_in the first condition:
if (speed >= RTE_ETH_SPEED_NUM_2_5G)
capa |= RTE_ETH_LINK_SPEED_2_5G;
if (speed >= RTE_ETH_SPEED_NUM_5G)
capa |= RTE_ETH_LINK_SPEED_5G;
Bug 78-88: overwriting variables
Let's start with an innocuous case.
static int bnxt_rss_hash_update_op(struct rte_eth_dev *eth_dev,
struct rte_eth_rss_conf *rss_conf)
{
....
/* Cache the hash function */
bp->rss_conf.rss_hf = rss_conf->rss_hf;
/* Cache the hash function */
bp->rss_conf.rss_hf = rss_conf->rss_hf;
....
}
PVS-Studio warning:
V519 The 'bp->rss_conf.rss_hf' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2322, 2325. bnxt_ethdev.c 2325
Identical assignments, one of which can simply be deleted. Now for the fun part.
uint16_t
cnxk_eswitch_dev_tx_burst(....)
{
....
uint64_t aura_handle, data = 0;
....
for (pkt = 0; pkt < nb_tx; pkt++) {
....
data &= ~0x7ULL;
/*<15:12> = CNTM1: Count minus one of LMTSTs in the burst */
data = (0ULL << 12);
/* *<10:0> = LMT_ID:
Identifies which LMT line is used for the first LMTST
*/
data |= (uint64_t)lmt_id;
....
}
....
}
PVS-Studio warning:
V519 The 'data' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 211, 213. cnxk_eswitch_rxtx.c 213
Hmm...
data &= ~0x7ULL;
data = (0ULL << 12);
I don't know what it's supposed to mean, but right now it's something confusing.
Let's move on.
static int
enetfec_tx_queue_setup(struct rte_eth_dev *dev,
uint16_t queue_idx,
uint16_t nb_desc,
unsigned int socket_id __rte_unused,
const struct rte_eth_txconf *tx_conf)
{
....
struct bufdesc *bdp, *bd_base;
....
bdp = txq->bd.base;
bdp = txq->bd.cur;
....
}
PVS-Studio warning:
V519 The 'bdp' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 417, 418. enet_ethdev.c 418
There's a typo here, and the code should probably look like this:
bd_base = txq->bd.base;
bdp = txq->bd.cur;
In the following code, one must have forgotten else.
static void ice_fill_sw_info(struct ice_hw *hw, struct ice_fltr_info *fi)
{
if ((fi->flag & ICE_FLTR_RX) &&
(fi->fltr_act == ICE_FWD_TO_VSI ||
fi->fltr_act == ICE_FWD_TO_VSI_LIST) &&
fi->lkup_type == ICE_SW_LKUP_LAST)
fi->lan_en = true;
fi->lb_en = false;
fi->lan_en = false;
....
}
PVS-Studio warning:
V519 The 'fi->lan_en' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3930, 3932. ice_switch.c 3932
I think this part should look like this (but not too sure):
if ((fi->flag & ICE_FLTR_RX) &&
(fi->fltr_act == ICE_FWD_TO_VSI ||
fi->fltr_act == ICE_FWD_TO_VSI_LIST) &&
fi->lkup_type == ICE_SW_LKUP_LAST)
fi->lan_en = true;
else
fi->lb_en = false;
I suspect the following bug is due to a failed refactoring or branch merge.
enum _ecore_status_t ecore_mcp_fill_shmem_func_info(....)
{
....
info->mtu = (u16)shmem_info.mtu_size;
if (info->mtu == 0)
info->mtu = 1500;
info->mtu = (u16)shmem_info.mtu_size;
....
}
PVS-Studio warning:
V519 The 'info->mtu' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2607, 2609. ecore_mcp.c 2609
The last assignment is clearly unnecessary and breaks everything.
The mistakes are obvious, but typical. I'm getting bored. So what follows is just a list.
Bug 89-93: very suspicious synchronization
The analyzer issued a pack of V1020 warnings that a resource might remain blocked. The diagnostic rule is not very accurate, and nested macros make it even more complicated. I got lost in DPDK macros. So it's hard for me to say where the diagnostic rule is right and where we got false positives. I think most of those warnings are off the point, so I won't cite them. Perhaps later I will take a closer look at the preprocessed files and highlight ideas to refine the diagnostic rule.
But as for more understandable places with less deep macros, the code still seems too suspicious for me. Let's take a look at these simple cases.
Let's start with some macros. This will help us to clear up the problem. Also we'll learn where the rte_spinlock_unlock function came from.
#define spin_lock(x) rte_spinlock_lock(x)
#define spin_unlock(x) rte_spinlock_unlock(x)
#define FQLOCK(fq) \
do { \
struct qman_fq *__fq478 = (fq); \
if (fq_isset(__fq478, QMAN_FQ_FLAG_LOCKED)) \
spin_lock(&__fq478->fqlock); \
} while (0)
#define FQUNLOCK(fq) \
do { \
struct qman_fq *__fq478 = (fq); \
if (fq_isset(__fq478, QMAN_FQ_FLAG_LOCKED)) \
spin_unlock(&__fq478->fqlock); \
} while (0)
Here is the troublesome code itself:
int qman_set_vdq(struct qman_fq *fq, u16 num, uint32_t vdqcr_flags)
{
....
if (!p->vdqcr_owned) {
FQLOCK(fq);
if (fq_isset(fq, QMAN_FQ_STATE_VDQCR))
goto escape;
fq_set(fq, QMAN_FQ_STATE_VDQCR);
FQUNLOCK(fq);
p->vdqcr_owned = fq;
ret = 0;
}
escape:
if (!ret)
qm_dqrr_vdqcr_set(&p->p, vdqcr);
out:
return ret;
}
PVS-Studio warning:
V1020 The function exited without calling the 'rte_spinlock_unlock' function. Check lines: 2149, 2136. qman.c 2149
If the fq_isset function returns an error status, the program will jump to the escape label and exit the function. With that said, it is very suspicious that the FQUNLOCK macro is not used. Perhaps, correct code version should be as follows:
FQLOCK(fq);
if (fq_isset(fq, QMAN_FQ_STATE_VDQCR))
{
FQUNLOCK(fq);
goto escape;
}
fq_set(fq, QMAN_FQ_STATE_VDQCR);
FQUNLOCK(fq);
Let's look at another case:
static __rte_noinline int
l2fwd_get_free_event_port(struct l2fwd_event_resources *evt_rsrc)
{
static int index;
int port_id;
rte_spinlock_lock(&evt_rsrc->evp.lock);
if (index >= evt_rsrc->evp.nb_ports) {
printf("No free event port is available\n");
return -1;
}
port_id = evt_rsrc->evp.event_p_id[index];
index++;
rte_spinlock_unlock(&evt_rsrc->evp.lock);
return port_id;
}
PVS-Studio warning:
V1020 The function exited without calling the 'rte_spinlock_unlock' function. Check lines: 144, 141. l2fwd_event.c 144
I think the author forgot to write unlock in the error handler again. I guess it should be like this:
rte_spinlock_lock(&evt_rsrc->evp.lock);
if (index >= evt_rsrc->evp.nb_ports) {
rte_spinlock_unlock(&evt_rsrc->evp.lock);
printf("No free event port is available\n");
return -1;
}
Similar simple suspicious code fragments.
Bug 94-100: pointless check of invalid pointers
Let's first check out how the roc_cpt_to_cpt_priv function works.
struct roc_cpt {
struct plt_pci_device *pci_dev;
struct roc_cpt_lf *lf[ROC_CPT_MAX_LFS];
uint16_t nb_lf;
uint16_t nb_lf_avail;
uintptr_t lmt_base;
/**< CPT device capabilities */
union cpt_eng_caps hw_caps[CPT_MAX_ENG_TYPES];
uint8_t eng_grp[CPT_MAX_ENG_TYPES];
uint8_t cpt_revision;
void *opaque;
#define ROC_CPT_MEM_SZ (6 * 1024)
uint8_t reserved[ROC_CPT_MEM_SZ] __plt_cache_aligned;
} __plt_cache_aligned;
static inline struct cpt *
roc_cpt_to_cpt_priv(struct roc_cpt *roc_cpt)
{
return (struct cpt *)&roc_cpt->reserved[0];
}
If the roc_cpt pointer is null, the function will return an invalid address, which is physically equal to the array offset relative to the beginning of the structure. One can't use this address. The only thing we can say about it is that it'll be non-null.
In fact, we now approach the topic of undefined behavior when working with null pointers. And we can't say for sure how or what will work. However, for the sake of simplicity, we can assume the pointer is non-null to continue our narrative.
Let's see how the returned pointer is used.
int
roc_cpt_dev_fini(struct roc_cpt *roc_cpt)
{
struct cpt *cpt = roc_cpt_to_cpt_priv(roc_cpt);
if (cpt == NULL)
return -EINVAL;
....
return dev_fini(&cpt->dev, cpt->pci_dev);
}
PVS-Studio warning:
V547 Expression 'cpt == NULL' is always false. roc_cpt.c 970
The author attempts to check the pointer for NULL, after which the pointer will be used for different purposes. If roc_cpt turns out to be NULL, undefined behavior occurs. The check is no protection from anything.
I think one should refine the roc_cpt_to_cpt_priv function as follows:
static inline struct cpt *
roc_cpt_to_cpt_priv(struct roc_cpt *roc_cpt)
{
if (roc_cpt == NULL)
return NULL;
return (struct cpt *)&roc_cpt->reserved[0];
}
Six more similar cases to get to an even number of 100 errors.
Are there other errors?
Yes, there are. Still, I have to stop somewhere, and 100 looks like a nice number in the title of the article. I'm also tired.
Developers and everyone can use a free trial version of PVS-Studio to check the project and study the report more thoroughly. I'd also like to remind you that we have an option of free PVS-Studio licensing for open source projects.
Conclusion
Finding real errors in code is the best way to promote static code analysis methodology. But while this is a vivid demonstration, it is not a productive way to use the tool. Analyzers should be used regularly during the development process to detect bugs as early as possible, making their fixing quick and cheap. Introduce Static Analysis in the Process, Don't Just Search for Bugs with It.
In any case, 100 bugs are better than 1000 words about the usefulness of static code analysis. Try the PVS-Studio analyzer.
Top comments (0)