I've been looking for a challenge to put the PVS-Studio analyzer through its paces, and that's why I chose GCC, an open-source compiler collection. It's not the first time we check this project. However, as the programming languages supported by the collection evolve, so does the GCC code. So, we will use the PVS-Studio analyzer to check GCC for bugs.
The GCC compiler collection, dating back to 1987, has been a major contributor to many open-source projects. Initially, GCC included only a compiler for the C programming language, C++, Objective-C, Java, Fortran, Ada, Go, GAS, and D were supported later. Currently, the latest major update to GCC is 13. This article discusses the 13.1 release. GCC has attracted many developers and even more users over the course of its long development history. As the result, the project is carefully tested, and looking for errors in such well-debugged and polished code is an exciting challenge.
The compiler code follows special coding conventions. The code teems with macros, which makes reading it a real nightmare for an unprepared user. It also makes the job of the PVS-Studio analyzer more complicated, but not impossible. Here are 10 code snippets that caught my eye when I was looking through the warnings. Unfortunately, it's difficult to examine the report in more detail because of the macros I mentioned. They require a preliminary configuration of the analyzer, which goes beyond the scope of just writing an article. If you'd like to read about the previous check of the GCC project, follow the link.
Fragment N1. Uninitialized structure
static ctf_id_t
gen_ctf_function_type (ctf_container_ref ctfc,
dw_die_ref function,
bool from_global_func)
{
....
ctf_funcinfo_t func_info; // <=
....
{
....
if (....)
{
do
{
....
if (....)
....
else if (....)
{
func_info.ctc_flags |= CTF_FUNC_VARARG; // <=
....
}
}
}
....
}
....
}
The PVS-Studio analyzer warning: V614 Uninitialized variable 'func_info.ctc_flags' used. gcc/dwarf2ctf.cc 676
The func_info object is not initialized when it is declared. Further down the code, in one of the branches, the CTF_FUNC_VARARG flag is set in the ctc_flags data member, but it has not been initialized.
Fragment N2. Potential null pointer
static struct gcov_info *
read_gcda_file (const char *filename)
{
....
curr_gcov_info = obj_info =
(struct gcov_info *) xcalloc (sizeof (struct gcov_info) +
sizeof (struct gcov_ctr_info) * GCOV_COUNTERS, 1);
obj_info->version = version;
obj_info->filename = filename;
....
}
The PVS-Studio analyzer warning: V522 There might be dereferencing of a potential null pointer 'obj_info'. Check lines: 290, 287. libgcov-util.c 290. libgcov-util.c 287
The PVS-Studio analyzer has detected that the validity of the obj_info pointer is not checked after allocation. The xmalloc, xcalloc, and xrealloc functions always return a valid pointer. If one of them is unable to allocate memory, it displays an error message and terminates the program. When I saw this warning, I considered it as false alarm and decided to make a minimal reproducible example for future debugging. However, when I opened a preprocessed file, I found a call to calloc in the line where xcalloc should have been. This is what the xcalloc macro actually expands to:
// libgcc/libgcov.h
....
/* work around the poisoned malloc/calloc in system.h. */
#ifndef xmalloc
#define xmalloc malloc
#endif
#ifndef xcalloc
#define xcalloc calloc
#endif
I anticipate future comments, so here is the link to my colleague's article. There, he gave the reasons why we need to check the result of these functions.
Fragment N3. Forgotten JSON
static void
generate_results (const char *file_name)
{
....
json::object *root = new json::object ();
root->set ("format_version", new json::string ("1"));
root->set ("gcc_version", new json::string (version_string));
if (....)
root->set ("current_working_directory", new json::string (bbg_cwd));
root->set ("data_file", new json::string (file_name));
json::array *json_files = new json::array ();
root->set ("files", json_files);
....
if (....)
{
if (....)
{
root->dump (stdout);
printf ("\n");
}
else
{
pretty_printer pp;
root->print (&pp);
....
}
}
}
The PVS-Studio analyzer warning: V773 The function was exited without releasing the 'root' pointer. A memory leak is possible. gcov.cc 1525
Developers forgot to delete root, the json object, that was created. This caused a memory leak. Most likely, this JSON output is a one-time event, and the error will have no real impact. I chose this case to demonstrate PVS-Studio's capabilities in memory leak detection.
Fragment N4. Unsafe function that may cause a buffer overflow
char m_flag_chars[256];
....
void
flag_chars_t::add_char (char ch)
{
int i = strlen (m_flag_chars);
m_flag_chars[i++] = ch;
m_flag_chars[i] = 0;
}
The PVS-Studio analyzer warning: V557 Array overrun is possible. The value of 'i' index could reach 256. c-format.cc 1994
There may be an array overrun when writing a terminal null if the length of the m_flag_chars string was equal to 255 before calling the add_char function.
Fragment N5. Redundant calculations
....
/* Return True when the format flag CHR has been used. */
bool get_flag (char chr) const
{
unsigned char c = chr & 0xff;
return (flags[c / (CHAR_BIT * sizeof *flags)]
& (1U << (c % (CHAR_BIT * sizeof *flags))));
}
....
static fmtresult
format_floating (const directive &dir, const HOST_WIDE_INT prec[2])
{
....
unsigned flagmin = (1 /* for the first digit */
+ (dir.get_flag ('+') | dir.get_flag (' ')));
....
}
The PVS-Studio analyzer warning: V792 The 'get_flag' function located to the right of the operator '|' will be called regardless of the value of the left operand. Perhaps, it is better to use '||'. gimple-ssa-sprintf.cc 1227
The get_flag function returns an object of the bool type. The bitwise "OR" operator combines the results of two function calls. There is no danger in the code, but the bitwise operator will evaluate both operands. By using the logical "OR", you can eliminate the unnecessary evaluation if the first call returns true:
unsigned flagmin = (1 /* for the first digit */
+ (dir.get_flag ('+') || dir.get_flag (' ')));
Fragment N6. Memory leak
struct gcov_info *
gcov_read_profile_dir (const char* dir_name,
int recompute_summary ATTRIBUTE_UNUSED)
{
char *pwd;
int ret;
read_profile_dir_init ();
if (access (dir_name, R_OK) != 0)
{
fnotice (stderr, "cannot access directory %s\n", dir_name);
return NULL;
}
pwd = getcwd (NULL, 0); // <=
gcc_assert (pwd);
ret = chdir (dir_name);
if (ret != 0)
{
fnotice (stderr, "%s is not a directory\n", dir_name);
return NULL; // <=
}
#ifdef HAVE_FTW_H
ftw (".", ftw_read_file, 50);
#endif
chdir (pwd);
free (pwd); // <=
return gcov_info_head;;
}
The PVS-Studio analyzer warning: V773 The function was exited without releasing the 'pwd' pointer. A memory leak is possible. libgcov-util.c 450, libgcov-util.c 434
Let's look at the getcwd function call. If a null pointer is passed to it as the first argument, the function dynamically allocates memory for a buffer of the necessary length and writes the current working directory to it. The release of memory is at the developer's discretion. As you can see, the developer released memory at the very end of the function. However, they forgot to do so in one of the code branches with an early return. This results in a potential, though insignificant, memory leak. Here you can read more about how PVS-Studio can detect memory leaks.
Fixed code:
....
ret = chdir (dir_name);
if (ret != 0)
{
fnotice (stderr, "%s is not a directory\n", dir_name);
free(pwd);
return NULL;
}
....
Fragment N7. Rewriting a parameter
static conversion *
build_aggr_conv (tree type,
tree ctor,
int flags, // <=
tsubst_flags_t complain)
{
unsigned HOST_WIDE_INT i = 0;
conversion *c;
tree field = next_aggregate_field (TYPE_FIELDS (type));
tree empty_ctor = NULL_TREE;
hash_set<tree, true> pset;
flags = LOOKUP_IMPLICIT|LOOKUP_NO_NARROWING; // <=
....
}
The PVS-Studio analyzer warning: V763 Parameter 'flags' is always rewritten in function body before being used. call.cc 994, call.cc 981
The flags function parameter, received by value, is always overwritten. Perhaps developers planned to set flags in this variable additionally, or another variable should have been used.
Fragment N8. Unexecutable loop
void
gt_pch_p_13ctf_container (ATTRIBUTE_UNUSED void *this_obj,
void *x_p,
ATTRIBUTE_UNUSED gt_pointer_operator op,
ATTRIBUTE_UNUSED void *cookie)
{
struct ctf_container * x ATTRIBUTE_UNUSED = (struct ctf_container *)x_p;
{
....
size_t l0 = (size_t)(0); // <=
....
if ((*x).ctfc_vars_list != NULL)
{
size_t i0;
for (i0 = 0; i0 != (size_t)(l0) // <=
&& ((void *)(*x).ctfc_vars_list == this_obj); i0++)
{
if ((void *)((*x).ctfc_vars_list) == this_obj)
op (&((*x).ctfc_vars_list[i0]), NULL, cookie);
}
....
}
}
....
}
The PVS-Studio analyzer warning: V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. gtype-desc.cc 11865
The l0 variable, on which the loop state depends, stays at zero. This means that the loop will never be executed.
The PVS-Studio analyzer has issued more of the similar warnings. Here are only some of them:
V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. gtype-desc.cc 11874
V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. gtype-desc.cc 11883
V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. gtype-desc.cc 11892
Fragment N9. Suspicious loop
static bool
combine_reaching_defs (ext_cand *cand,
const_rtx set_pat,
ext_state *state)
{
....
while ( REG_P (SET_SRC (*dest_sub_rtx))
&& (REGNO (SET_SRC (*dest_sub_rtx)) == REGNO (SET_DEST (set))))
{
....
if (....)
break;
if (....)
break;
....
break;
}
}
The PVS-Studio analyzer warning: V612 An unconditional 'break' within a loop. ree.cc 985
The PVS-Studio analyzer has detected an unconditional break within the loop at the first iteration. If we look further, we can also find many other break's that are conditional. Perhaps this is a way to avoid writing the _goto _statement. It's odd, though, since the statement is used explicitly throughout the project.
Fragment N10. Strange conditional
void
const_fn_result_svalue::dump_to_pp(pretty_printer *pp,
bool simple) const
{
if (simple)
{
pp_printf (pp, "CONST_FN_RESULT(%qD, {", m_fndecl);
for (unsigned i = 0; i < m_num_inputs; i++)
{
if (i > 0)
pp_string (pp, ", ");
dump_input (pp, i, m_input_arr[i], simple);
}
pp_string (pp, "})");
}
else
{
pp_printf (pp, "CONST_FN_RESULT(%qD, {", m_fndecl);
for (unsigned i = 0; i < m_num_inputs; i++)
{
if (i > 0)
pp_string (pp, ", ");
dump_input (pp, i, m_input_arr[i], simple);
}
pp_string (pp, "})");
}
}
The PVS-Studio analyzer warning: V523 The 'then' statement is equivalent to the 'else' statement. svalue.cc 2009, svalue.cc 1998
Complete logic duplication in conditional branches is detected. I decided to look for the cause of the duplication and used the git log command (--follow -p ./gcc/analyzer/svalue.cc) in the git root directory of the GCC project to see the difference between commits. However, I found nothing there. It turns out that the file had this error all along.
Conclusion
When examining the code that uses macros so actively, one gets tired very quickly. But the satisfaction of working on a project as serious as GCC makes up for the fatigue. PVS-Studio has proven that it can detect hardly noticeable errors in well-debugged codebases, and even a mountain of macros did not become an obstacle for it.
At the end of this article, I'd want to wish everyone as few errors in code and as few issues in releases as possible :). Also, taking the opportunity, I invite you to try out the trial version of the analyzer. And to make sure you don't miss anything of interest, you're welcome to subscribeto our digest.
Top comments (0)