On 2/26/25 1:46 AM, 翁愷邑 wrote:
The build_target_option_node() function may return a cached node when
fndecl having the same effective global_options. Therefore, freeing
memory used in target nodes can lead to a use-after-free issue, as a
target node may be shared by multiple fndecl.
This issue occurs in gcc.target/riscv/target-attr-16.c, where all
functions have the same march, but the last function tries to free its
old x_riscv_arch_string (which is shared) when processing the second
target attribute.However, the behavior of this issue depends on how the
OS handles malloc. It's very likely that xstrdup returns the old address
just freed, coincidentally hiding the issue. We can verify the issue by
forcing xstrdup to return a new address, e.g.,
- if (opts->x_riscv_arch_string != default_opts->x_riscv_arch_string)
- free (CONST_CAST (void *, (const void *) opts->x_riscv_arch_string));
+ // Force it to use a new address, NFCI
+ const char *tmp = opts->x_riscv_arch_string;
opts->x_riscv_arch_string = xstrdup (local_arch_str);
+ if (tmp != default_opts->x_riscv_arch_string)
+ free (CONST_CAST (void *, (const void *) tmp));
This patch replaces xstrdup with ggc_strdup and let gc to take care of
unused strings.
gcc/ChangeLog:
* config/riscv/riscv-target-attr.cc
(riscv_target_attr_parser::update_settings):
Do not manually free any arch string.
Sorry this has taken so long to dive into.
No argument about the fact this code as-is is broken and can lead to a
use-after free issue, even if it's latent right now.
As far as the fix is concerned, if you want to move something into
garbage collected memory, then it needs to be reachable from the
registered garbage collector roots, otherwise it's no different than a
malloc that you never free.
Interestingly enough that node is already marked as a GC node, so it was
incorrect to be calling free on it to begin with!
void
gt_ggc_mx_cl_target_option (void *x_p)
{
struct cl_target_option * const x = (struct cl_target_option *)x_p;
if (ggc_test_and_set_mark (x))
{
gt_ggc_m_S ((*x).x_riscv_arch_string);
gt_ggc_m_S ((*x).x_riscv_cpu_string);
gt_ggc_m_S ((*x).x_riscv_tune_string);
}
}
Strangely enough this didn't get picked up by patchwork as far as I can
tell, so it never ran through the pre-commit tester. So I ran it
through my own tester without seeing any regressions.
I'll push it to the trunk momentarily.
jeff
