Listen, my tastes are very...singular. The beauty of this error is one does not simply find it. I don't believe that developers can spot it in code reviews. Unless if they know beforehand that there's one and search for it on purpose.
I found this error in the DPDK project. There are other errors, but we'll talk about them later. They pale in comparison to this diamond error. Just don't expect anything wild. The error is simple as hell. But it can become a nasty piece of work to spot this tough cookie during code reviews. Well you should give it a try, go on!
First, glance at the list of named constants:
enum dbg_status
enum dbg_status {
DBG_STATUS_OK,
DBG_STATUS_APP_VERSION_NOT_SET,
DBG_STATUS_UNSUPPORTED_APP_VERSION,
DBG_STATUS_DBG_BLOCK_NOT_RESET,
DBG_STATUS_INVALID_ARGS,
DBG_STATUS_OUTPUT_ALREADY_SET,
DBG_STATUS_INVALID_PCI_BUF_SIZE,
DBG_STATUS_PCI_BUF_ALLOC_FAILED,
DBG_STATUS_PCI_BUF_NOT_ALLOCATED,
DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS,
DBG_STATUS_NO_MATCHING_FRAMING_MODE,
DBG_STATUS_VFC_READ_ERROR,
DBG_STATUS_STORM_ALREADY_ENABLED,
DBG_STATUS_STORM_NOT_ENABLED,
DBG_STATUS_BLOCK_ALREADY_ENABLED,
DBG_STATUS_BLOCK_NOT_ENABLED,
DBG_STATUS_NO_INPUT_ENABLED,
DBG_STATUS_NO_FILTER_TRIGGER_256B,
DBG_STATUS_FILTER_ALREADY_ENABLED,
DBG_STATUS_TRIGGER_ALREADY_ENABLED,
DBG_STATUS_TRIGGER_NOT_ENABLED,
DBG_STATUS_CANT_ADD_CONSTRAINT,
DBG_STATUS_TOO_MANY_TRIGGER_STATES,
DBG_STATUS_TOO_MANY_CONSTRAINTS,
DBG_STATUS_RECORDING_NOT_STARTED,
DBG_STATUS_DATA_DIDNT_TRIGGER,
DBG_STATUS_NO_DATA_RECORDED,
DBG_STATUS_DUMP_BUF_TOO_SMALL,
DBG_STATUS_DUMP_NOT_CHUNK_ALIGNED,
DBG_STATUS_UNKNOWN_CHIP,
DBG_STATUS_VIRT_MEM_ALLOC_FAILED,
DBG_STATUS_BLOCK_IN_RESET,
DBG_STATUS_INVALID_TRACE_SIGNATURE,
DBG_STATUS_INVALID_NVRAM_BUNDLE,
DBG_STATUS_NVRAM_GET_IMAGE_FAILED,
DBG_STATUS_NON_ALIGNED_NVRAM_IMAGE,
DBG_STATUS_NVRAM_READ_FAILED,
DBG_STATUS_IDLE_CHK_PARSE_FAILED,
DBG_STATUS_MCP_TRACE_BAD_DATA,
DBG_STATUS_MCP_TRACE_NO_META,
DBG_STATUS_MCP_COULD_NOT_HALT,
DBG_STATUS_MCP_COULD_NOT_RESUME,
DBG_STATUS_RESERVED0,
DBG_STATUS_SEMI_FIFO_NOT_EMPTY,
DBG_STATUS_IGU_FIFO_BAD_DATA,
DBG_STATUS_MCP_COULD_NOT_MASK_PRTY,
DBG_STATUS_FW_ASSERTS_PARSE_FAILED,
DBG_STATUS_REG_FIFO_BAD_DATA,
DBG_STATUS_PROTECTION_OVERRIDE_BAD_DATA,
DBG_STATUS_DBG_ARRAY_NOT_SET,
DBG_STATUS_RESERVED1,
DBG_STATUS_NON_MATCHING_LINES,
DBG_STATUS_INSUFFICIENT_HW_IDS,
DBG_STATUS_DBG_BUS_IN_USE,
DBG_STATUS_INVALID_STORM_DBG_MODE,
DBG_STATUS_OTHER_ENGINE_BB_ONLY,
DBG_STATUS_FILTER_SINGLE_HW_ID,
DBG_STATUS_TRIGGER_SINGLE_HW_ID,
DBG_STATUS_MISSING_TRIGGER_STATE_STORM,
MAX_DBG_STATUS
};
Now examine the string array:
static const char * const s_status_str[] = ....
static const char * const s_status_str[] = {
/* DBG_STATUS_OK */
"Operation completed successfully",
/* DBG_STATUS_APP_VERSION_NOT_SET */
"Debug application version wasn't set",
/* DBG_STATUS_UNSUPPORTED_APP_VERSION */
"Unsupported debug application version",
/* DBG_STATUS_DBG_BLOCK_NOT_RESET */
"The debug block wasn't reset since the last recording",
/* DBG_STATUS_INVALID_ARGS */
"Invalid arguments",
/* DBG_STATUS_OUTPUT_ALREADY_SET */
"The debug output was already set",
/* DBG_STATUS_INVALID_PCI_BUF_SIZE */
"Invalid PCI buffer size",
/* DBG_STATUS_PCI_BUF_ALLOC_FAILED */
"PCI buffer allocation failed",
/* DBG_STATUS_PCI_BUF_NOT_ALLOCATED */
"A PCI buffer wasn't allocated",
/* DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS */
"The filter/trigger constraint dword offsets are not "
"enabled for recording",
/* DBG_STATUS_VFC_READ_ERROR */
"Error reading from VFC",
/* DBG_STATUS_STORM_ALREADY_ENABLED */
"The Storm was already enabled",
/* DBG_STATUS_STORM_NOT_ENABLED */
"The specified Storm wasn't enabled",
/* DBG_STATUS_BLOCK_ALREADY_ENABLED */
"The block was already enabled",
/* DBG_STATUS_BLOCK_NOT_ENABLED */
"The specified block wasn't enabled",
/* DBG_STATUS_NO_INPUT_ENABLED */
"No input was enabled for recording",
/* DBG_STATUS_NO_FILTER_TRIGGER_256B */
"Filters and triggers are not allowed in E4 256-bit mode",
/* DBG_STATUS_FILTER_ALREADY_ENABLED */
"The filter was already enabled",
/* DBG_STATUS_TRIGGER_ALREADY_ENABLED */
"The trigger was already enabled",
/* DBG_STATUS_TRIGGER_NOT_ENABLED */
"The trigger wasn't enabled",
/* DBG_STATUS_CANT_ADD_CONSTRAINT */
"A constraint can be added only after a filter was "
"enabled or a trigger state was added",
/* DBG_STATUS_TOO_MANY_TRIGGER_STATES */
"Cannot add more than 3 trigger states",
/* DBG_STATUS_TOO_MANY_CONSTRAINTS */
"Cannot add more than 4 constraints per filter or trigger state",
/* DBG_STATUS_RECORDING_NOT_STARTED */
"The recording wasn't started",
/* DBG_STATUS_DATA_DID_NOT_TRIGGER */
"A trigger was configured, but it didn't trigger",
/* DBG_STATUS_NO_DATA_RECORDED */
"No data was recorded",
/* DBG_STATUS_DUMP_BUF_TOO_SMALL */
"Dump buffer is too small",
/* DBG_STATUS_DUMP_NOT_CHUNK_ALIGNED */
"Dumped data is not aligned to chunks",
/* DBG_STATUS_UNKNOWN_CHIP */
"Unknown chip",
/* DBG_STATUS_VIRT_MEM_ALLOC_FAILED */
"Failed allocating virtual memory",
/* DBG_STATUS_BLOCK_IN_RESET */
"The input block is in reset",
/* DBG_STATUS_INVALID_TRACE_SIGNATURE */
"Invalid MCP trace signature found in NVRAM",
/* DBG_STATUS_INVALID_NVRAM_BUNDLE */
"Invalid bundle ID found in NVRAM",
/* DBG_STATUS_NVRAM_GET_IMAGE_FAILED */
"Failed getting NVRAM image",
/* DBG_STATUS_NON_ALIGNED_NVRAM_IMAGE */
"NVRAM image is not dword-aligned",
/* DBG_STATUS_NVRAM_READ_FAILED */
"Failed reading from NVRAM",
/* DBG_STATUS_IDLE_CHK_PARSE_FAILED */
"Idle check parsing failed",
/* DBG_STATUS_MCP_TRACE_BAD_DATA */
"MCP Trace data is corrupt",
/* DBG_STATUS_MCP_TRACE_NO_META */
"Dump doesn't contain meta data - it must be provided in image file",
/* DBG_STATUS_MCP_COULD_NOT_HALT */
"Failed to halt MCP",
/* DBG_STATUS_MCP_COULD_NOT_RESUME */
"Failed to resume MCP after halt",
/* DBG_STATUS_RESERVED0 */
"",
/* DBG_STATUS_SEMI_FIFO_NOT_EMPTY */
"Failed to empty SEMI sync FIFO",
/* DBG_STATUS_IGU_FIFO_BAD_DATA */
"IGU FIFO data is corrupt",
/* DBG_STATUS_MCP_COULD_NOT_MASK_PRTY */
"MCP failed to mask parities",
/* DBG_STATUS_FW_ASSERTS_PARSE_FAILED */
"FW Asserts parsing failed",
/* DBG_STATUS_REG_FIFO_BAD_DATA */
"GRC FIFO data is corrupt",
/* DBG_STATUS_PROTECTION_OVERRIDE_BAD_DATA */
"Protection Override data is corrupt",
/* DBG_STATUS_DBG_ARRAY_NOT_SET */
"Debug arrays were not set "
"(when using binary files, dbg_set_bin_ptr must be called)",
/* DBG_STATUS_RESERVED1 */
"",
/* DBG_STATUS_NON_MATCHING_LINES */
"Non-matching debug lines - in E4, all lines must be of "
"the same type (either 128b or 256b)",
/* DBG_STATUS_INSUFFICIENT_HW_IDS */
"Insufficient HW IDs. Try to record less Storms/blocks",
/* DBG_STATUS_DBG_BUS_IN_USE */
"The debug bus is in use",
/* DBG_STATUS_INVALID_STORM_DBG_MODE */
"The storm debug mode is not supported in the current chip",
/* DBG_STATUS_OTHER_ENGINE_BB_ONLY */
"Other engine is supported only in BB",
/* DBG_STATUS_FILTER_SINGLE_HW_ID */
"The configured filter mode requires a single Storm/block input",
/* DBG_STATUS_TRIGGER_SINGLE_HW_ID */
"The configured filter mode requires that all the constraints of a "
"single trigger state will be defined on a single Storm/block input",
/* DBG_STATUS_MISSING_TRIGGER_STATE_STORM */
"When triggering on Storm data, the Storm to trigger on must be specified"
};
And now look at the code that turns the named constant into the string:
const char *qed_dbg_get_status_str(enum dbg_status status)
{
return (status < MAX_DBG_STATUS) ?
s_status_str[status] : "Invalid debug status";
}
Where's the error?
You found it?
There's a hint that you could theoretically latch on to. In the array, a single empty string separates all rows except for one place. But let's start with the PVS-Studio analyzer warning:
V557 Array overrun is possible. The value of 'status' index could reach 58. qede_debug.c 7149
It points to the code fragment where the index array returns the string pointer:
return (status < MAX_DBG_STATUS) ?
s_status_str[status] : "Invalid debug status";
Initially, I diagnosed it incorrectly and unpretentiously. I inadvertently thought I found an off-by-one error here. It looked like a typical incorrect check, which I encountered many times in various projects.
Here is the essence of a classic antipattern. There is an enum whose last component determines the number of elements.
enum Efoo {
A,
B,
C,
COUNT
};
Constants in enum are assigned values starting from 0. So, the auxiliary constant COUNT will be equal to 3, which corresponds to the number of the main named constants.
There's an array where each named constant corresponds to something:
char Cfoo[] = { 'A', 'B', 'C' };
It's a common bug when developers check that the index doesn't overrun the array bounds:
char Convert(unsigned id)
{
return (id > COUNT) ? 0 : Cfoo[id];
}
The check doesn't operate correctly: if id is equal to COUNT, the array will be out of bounds. PVS-Studio gives a similar warning about these kinds of errors.
The correct option would be one of the following:
return (id >= COUNT) ? 0 : Cfoo[id]; // OK
return (id < COUNT) ? Cfoo[id] : 0; // OK
Nothing special here. Let's move on. We can write out a warning about array overrun in the article but don't analyze the bug in the DPDK check article.
Then at the last minute—STOP!
THE CHECK IS ACTUALLY CORRECT!
return (status < MAX_DBG_STATUS) ?
s_status_str[status] : "Invalid debug status";
Now it's less clear and more intriguing! Why did the analyzer issue a warning? False positive? I don't think so. There's no way it could get tangled up.
Rolling up my sleeves, I start looking over the code. Here it is! A missing line for the DBG_STATUS_NO_MATCHING_FRAMING_MODE constant.
In the enum:
DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS,
DBG_STATUS_NO_MATCHING_FRAMING_MODE,
DBG_STATUS_VFC_READ_ERROR,
In the array:
/* DBG_STATUS_INVALID_FILTER_TRIGGER_DWORDS */
"The filter/trigger constraint dword offsets are not "
"enabled for recording",
/* DBG_STATUS_VFC_READ_ERROR */
"Error reading from VFC",
Look at a separator with two empty lines. I've already talked about this artifact before. Something went wrong. Maybe the array was filled incorrectly, or the row was accidentally deleted, or a branch merge failed, or there's another reason.
However, once we've already found the bug, two empty lines look suspicious. But developers will hardly pay attention to them in code reviews. Even if they do, they'll just remove one of them for beauty's sake. No one's going to say, "We might have missed something. Let's check it" :)
As a result of the error:
- possible array overrun;
- the function returns incorrect strings for all constants starting with DBG_STATUS_NO_MATCHING_FRAMING_MODE.
In my opinion, it's beautiful. Thanks for your time. Don't forget to regularly check the code using PVS-Studio!
P.S.
One of my colleagues suggested to add the idea that developers can avoid the error by using compile-time checks. Code examples with static_assert for C and C++. Computing array sizes in C++ works more handily than in C, but that's another story. It's probably excessive to add such checks everywhere. But for such large arrays, why not?
Top comments (0)