On 2/12/21 9:56 AM, Martin Sebor wrote:
On 2/12/21 2:09 AM, Richard Biener via Gcc-patches wrote:
On Thu, Feb 11, 2021 at 6:41 PM Martin Liška <mli...@suse.cz> wrote:
Hello.
This fixes 2 memory leaks I noticed.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Ready to be installed?
OK.
Thanks,
Martin
gcc/ChangeLog:
* opts-common.c (decode_cmdline_option): Release werror_arg.
* opts.c (gen_producer_string): Release output of
gen_command_line_string.
---
gcc/opts-common.c | 1 +
gcc/opts.c | 7 +++++--
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/gcc/opts-common.c b/gcc/opts-common.c
index 6cb5602896d..5e10edeb4cf 100644
--- a/gcc/opts-common.c
+++ b/gcc/opts-common.c
@@ -766,6 +766,7 @@ decode_cmdline_option (const char *const *argv,
unsigned int lang_mask,
werror_arg[0] = 'W';
size_t warning_index = find_opt (werror_arg, lang_mask);
+ free (werror_arg);
Sorry to butt in here, but since we're having a discussion on this
same subject in another review of a fix for a memory leak, I thought
I'd pipe up: I would suggest a better (more in line with C++ and more
future-proof) solution is to replace the call to xstrdup (and the need
to subsequently call free) with std::string.
if (warning_index != OPT_SPECIAL_unknown)
{
const struct cl_option *warning_option
diff --git a/gcc/opts.c b/gcc/opts.c
index fc5f61e13cc..24bb64198c8 100644
--- a/gcc/opts.c
+++ b/gcc/opts.c
@@ -3401,8 +3401,11 @@ char *
gen_producer_string (const char *language_string,
cl_decoded_option *options,
unsigned int options_count)
{
- return concat (language_string, " ", version_string, " ",
- gen_command_line_string (options, options_count),
NULL);
+ char *cmdline = gen_command_line_string (options, options_count);
+ char *combined = concat (language_string, " ", version_string, " ",
+ cmdline, NULL);
+ free (cmdline);
+ return combined;
}
Here, changing gen_command_line_string() to return std::string instead
of a raw pointer would similarly avoid having to remember to free
the pointer (and forgetting). The function has two other callers,
both in gcc/toplev.c, and both also leak the memory for the same
reason as here.
It occurs to me that until the APIs are improved (to use RAII), or
where changing them is not feasible, an alternative is to let our
own analyzer detect the leaks by annotating gen_command_line_string
and other functions like it with the new attribute malloc. With
that, this leak is diagnosed like so:
/src/gcc/master/gcc/opts.c: In function ‘char*
gen_command_line_string(cl_decoded_option*, unsigned int)’:
/src/gcc/master/gcc/opts.c:3395:10: warning: leak of ‘cmdline’ [CWE-401]
[-Wanalyzer-malloc-leak]
3395 | return options_string;
| ^~~~~~~~~~~~~~
‘char* gen_producer_string(const char*, cl_decoded_option*, unsigned
int)’: events 1-3
|
| 3401 | gen_producer_string (const char *language_string,
cl_decoded_option *options,
| | ^~~~~~~~~~~~~~~~~~~
| | |
| | (1) entry to ‘gen_producer_string’
|......
| 3404 | char *cmdline = gen_command_line_string (options,
options_count);
...
The attribute malloc annotation would look like this:
__attribute__ ((__malloc__, __malloc__ (free)))
extern char *gen_command_line_string (cl_decoded_option *options,
unsigned int options_count);
It would be worthwhile to do a review of GCC's own APIs and add
the attribute wherever appropriate, not just to detect leaks but
other memory management bugs (e.g., calling free on memory
allocated by operator new). Something to consider for GCC 12.
Martin
