We have long wanted to write an article with the idea that unit tests are cool. But we shouldn't forget that they can also contain errors. Recently we have checked the DPDK project, whose tests demonstrate this aspect very well. So let's see how typical errors in unit tests look like, and how static code analysis detects them.
and is a set of open source libraries that accelerate packet processing by allowing networking hardware to communicate directly with applications, bypassing the Linux kernel.
Its unit tests turned out to have enough errors to write an article. Actually, it is the article you are reading right now. Check out examples of bugs in unit tests and how skillfully you can search for them using PVS-Studio.
Note. I'll cover other errors from the DPDK project that do not relate to tests in a separate article.
Bugs in unit tests
Not every function contains test in its name. So why did I decide that all the code below refers to tests? Simple answer: files with this code locate in the test* folders.
Bug N1: Two loops for one variable
#define MAX_PKT_BURST (512)
static int
test_activebackup_rx_burst(void)
{
....
int i, j, burst_size = 17;
....
for (i = 0; i < test_params->bonding_member_count; i++) {
/* Generate test bursts of packets to transmit */
TEST_ASSERT_EQUAL(generate_test_burst(
&gen_pkt_burst[0], burst_size, 0, 1, 0, 0, 0),
burst_size, "burst generation failed");
....
/* free mbufs */
for (i = 0; i < MAX_PKT_BURST; i++) {
if (rx_pkt_burst[i] != NULL) {
rte_pktmbuf_free(rx_pkt_burst[i]);
rx_pkt_burst[i] = NULL;
}
}
/* reset bonding device stats */
rte_eth_stats_reset(test_params->bonding_port_id);
}
....
}
PVS-Studio warning:
The first argument of 'memcmp' function is equal to the second argument. test_link_bonding.c 795
The memcmp function compares the same memory buffer to itself. Naturally, the function will always return 0 (two memory blocks contain identical data). An obvious typo due to carelessness or haste.
This unit test checks nothing.
Apparently, one of the arguments must be the expected_mac_addr pointer. I concluded this because I found this memcmp call nearby:
TEST_ASSERT_SUCCESS(memcmp(expected_mac_addr, &read_mac_addr,
sizeof(read_mac_addr)),
"bonding port mac address not set to that of primary port\n");
Bug N3: Misplaced parenthesis
First, let's look at how the rte_ipv6_get_next_ext function is declared.
/**
* Parse next IPv6 header extension
* ....
* @return
* next protocol number if proto is an IPv6 extension, -EINVAL otherwise
*/
static inline int
rte_ipv6_get_next_ext(const uint8_t *p, int proto, size_t *ext_len);
Note that the function returns a negative value (-EINVAL, namely -22) in case of an error.
Now let's see how this function is used in tests.
test_vector_payload_populate(....)
{
....
int proto;
....
proto = hdr->proto;
p += sizeof(struct rte_ipv6_hdr);
while (proto != IPPROTO_FRAGMENT &&
(proto = rte_ipv6_get_next_ext(p, proto, &ext_len) >= 0)) // <=
p += ext_len;
/* Found fragment header, update the frag offset */
if (proto == IPPROTO_FRAGMENT) {
....
}
PVS-Studio warning:
The 'dev_conf->nb_event_port_enqueue_depth' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1166, 1168. test_event_crypto_adapter.c 1168
The analyzer noticed this repeated code:
dev_conf->nb_event_port_enqueue_depth = info->max_event_port_enqueue_depth;
dev_conf->nb_event_port_enqueue_depth = info->max_event_port_enqueue_depth;
Let's take a look at the structure rte_event_dev_config. I commented which data members are assigned some values.
struct rte_event_dev_config {
uint32_t dequeue_timeout_ns; // There is an assignment
int32_t nb_events_limit; // There is an assignment
uint8_t nb_event_queues; // There is an assignment
uint8_t nb_event_ports; // There is an assignment
uint32_t nb_event_queue_flows; // There is an assignment
uint32_t nb_event_port_dequeue_depth; // There is an assignment
uint32_t nb_event_port_enqueue_depth; // There is an assignment twice
uint32_t event_dev_cfg; //
uint8_t nb_single_link_event_port_queues; //
};
At the beginning, the entire structure is filled with zeros (see call of the memset function). Then all data members except the last two are initialized with new values. The nb_event_port_enqueue_depth data member is assigned a value twice. It's all very suspicious.
Perhaps the second assignment line is just unnecessary. Or maybe one forgot to initialize another data member. Or one even forgot to initialize two data members. I'm not sure how exactly it should look like, since I am not familiar with the project code, but there's something fishy here.
Also, this code was abundantly reproduced by copying.
- V519 The 'dev_conf->nb_event_port_enqueue_depth' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 414, 416. test_event_dma_adapter.c 416
- V519 The 'dev_conf->nb_event_port_enqueue_depth' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 83, 85. test_event_timer_adapter.c 85
- V519 The 'dev_conf->nb_event_port_enqueue_depth' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 112, 114. test_eventdev.c 114
- V519 The 'dev_conf->nb_event_port_enqueue_depth' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2114, 2115. test_pdcp.c 2115
- V519 The 'dev_conf->nb_event_port_enqueue_depth' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 123, 125. cnxk_eventdev_selftest.c 125
- V519 The 'dev_conf->nb_event_port_enqueue_depth' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 99, 101. dpaa2_eventdev_selftest.c 101
- V519 The 'dev_conf->nb_event_port_enqueue_depth' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 134, 136. ssovf_evdev_selftest.c 136
- V519 The 'dev_conf->nb_event_port_enqueue_depth' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 418, 420. octeontx_ethdev.c 420
Naughty There are identical sub-expressions '(flags->content_type == TLS_RECORD_TEST_CONTENT_TYPE_HANDSHAKE)' to the left and to the right of the '||' operator. test_cryptodev.c 12173
It's all simple here. We compare a variable with the same constant twice. The unit test does not check a certain case. One of the constants must obviously be different.
Bug N6: 6
It reminds me of my own article: '' There are identical sub-expressions 'rte_lcore_is_enabled(3)' to the left and to the right of the '&&' operator. test_eal_flags.c 678
The programmer started with calling the function rte_lcore_is_enabled, successively specifying the constants: 0, 1, 2, 3.
And then our programmer went wild: 3, 5, 4, 7. This might have happened because of the rush or distraction.
This bug is hard to spot on a code review. It just looks like some different numbers. Seems like it's all correct. Reading of such code is boring. As a result, the unit test will not check the function call for value 6.
It's good to have the PVS-Studio analyzer, which is not lazy to look closely at each line and constant :)
Bug N7: Redundant or incorrect condition
int
port_meter_policy_add(portid_t port_id, uint32_t policy_id,
const struct rte_flow_action *actions)
{
....
for (i = 0; i < RTE_COLORS; i++) {
for (act_n = 0, start = act;
act->type != RTE_FLOW_ACTION_TYPE_END; act++)
act_n++;
if (act_n && act->type == RTE_FLOW_ACTION_TYPE_END)
policy.actions[i] = start;
else
policy.actions[i] = NULL;
act++;
}
....
}
PVS-Studio warning:
here for a monthly trial. Have fun hunting for bugs in the code.
Bug N8: Checking the index after use
static int
comp_names_to_index(struct context *ctx, const struct token *token,
unsigned int ent, char *buf, unsigned int size,
const char *const names[], size_t names_size)
{
RTE_SET_USED(ctx);
RTE_SET_USED(token);
if (!buf)
return names_size;
if (names[ent] && ent < names_size)
return rte_strscpy(buf, names[ent], size);
return -1;
}
PVS-Studio warning:
Expression '* addrs == '\0'' is always false. main.c 234
The loop ends only when a non-zero character is found.
while (*addrs == '\0')
addrs++;
if (*addrs == '\0') {
Therefore, a follow-up check does not make sense. The pointer will always point not to function.
What's the point of trying to skip all null characters in the loop?
while (*addrs == '\0')
addrs++;
If the string is empty (with '\0' written at the beginning), the array will be overridden.
If the string is non-empty, the loop will stop immediately without performing any iterations.
Maybe the author of code wanted to write something like this?
while (*addrs != '\0')
addrs++;
No, it doesn't make sense either.
I suspect that the loop is redundant here, and this fragment should be written like this:
input = strndup(value, strlen(value) + 1);
if (input == NULL)
return -1;
addrs = input;
if (*addrs == '\0') {
fprintf(stderr, "No input DMA addresses\n");
ret = -1;
goto out;
}
Bug N10: 262144 times fewer checks than intended
#define MAX_NUM 1 << 20
static int
test_align(void)
{
....
for (p = 1; p <= MAX_NUM / 2; p++) { // <=
for (i = 1; i <= MAX_NUM / 2; i++) { // <=
val = RTE_ALIGN_MUL_CEIL(i, p);
if (val % p != 0 || val < i)
FAIL_ALIGN("RTE_ALIGN_MUL_CEIL", i, p);
val = RTE_ALIGN_MUL_FLOOR(i, p);
if (val % p != 0 || val > i)
FAIL_ALIGN("RTE_ALIGN_MUL_FLOOR", i, p);
val = RTE_ALIGN_MUL_NEAR(i, p);
if (val % p != 0 || ((val != RTE_ALIGN_MUL_CEIL(i, p))
& (val != RTE_ALIGN_MUL_FLOOR(i, p))))
FAIL_ALIGN("RTE_ALIGN_MUL_NEAR", i, p);
}
}
....
}
PVS-Studio warnings:
for various reasons. One of them is that they severely disrupt the perception of the program. Here is a fitting example: individually, the code looks fine, but together it becomes a mess.
A block of code from unit tests seems to be OK.
The MAX_NUM macro also looks fine at first glance:
CODE#define MAX_NUM 1 << 20
However, when substituting into the loop condition, we get the following expression:
CODE1 << 20 / 2
The priority of the division operation is higher than that of the shift operation. This results in a shift of 10 bits rather than 20: 1 << 10.
Let's count how many options are iterated in loops due to an error:
(1 << (20 / 2)) * (1 << (20 / 2)) = 1024 * 1024 = 1048576
This is how many options should be iterated according to the programmer's idea:
((1 << 20) / 2) * ((1 << 20) / 2) = 524288 * 524288 = 274877906944
Total, the unit test performs 274877906944/1048576 = 262144 times fewer checks.
When reviewing the code, you may notice such an error but there is no guarantee. You need to clearly look at how the macro is written, make substitutions in your head, and remember about the priority of operations. A static analyzer is a decent helper in detecting such errors.
To fix the code, one should add parentheses to the macro:
CODE#define MAX_NUM (1 << 20)
If we write in C++, it is better to use constexpr. And you are welcome to read the article " and see if there is something interesting in the unit tests of your projects. If you find something worthwhile, let me know in comments.
↗ Original-Artikel auf dev.to lesenVollständiger Original-BerichtAusführliche Details, Code-Beispiele & Hersteller-Stellungnahme auf dev.to.
SOCIAL SHARE CARD GENERATOR