[PATCH, i386]: Fix PR 67484, asan detects heap-use-after-free with target options

[PATCH, i386]: Fix PR 67484, asan detects heap-use-after-free with target options

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

Hello!

As mentioned in the PR, ix86_valid_target_attribute_tree creates
temporary copies of current options strings and saves *pointers* to
these copies with build_target_option_node. A couple of lines below,
these temporary copies are freed, leaving dangling pointers in the
saved structure.

Use xstrndup to create permanent copy of string on the heap. This will
however create a small leak, as this copy is never deallocated.

There is no test infrastructure to check for memory errors, so there
is no testcase added.

2015-09-15  Uros Bizjak  <ubizjak@gmail.com>

    PR target/67484
    * config/i386/i386.c (ix86_valid_target_attribute_tree):
    Use xstrdup to copy option_strings to opts->x_ix86_arch_string and
    opts->x_ix86_tune_string.

Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.

I'll wait a couple of days for possible comments on the above solution.

Uros.

[-- Attachment #2: p.diff.txt --]
[-- Type: text/plain, Size: 935 bytes --]

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 227777)
+++ config/i386/i386.c	(working copy)
@@ -5080,12 +5080,14 @@ ix86_valid_target_attribute_tree (tree args,
       /* If we are using the default tune= or arch=, undo the string assigned,
 	 and use the default.  */
       if (option_strings[IX86_FUNCTION_SPECIFIC_ARCH])
-	opts->x_ix86_arch_string = option_strings[IX86_FUNCTION_SPECIFIC_ARCH];
+	opts->x_ix86_arch_string
+	  = xstrdup (option_strings[IX86_FUNCTION_SPECIFIC_ARCH]);
       else if (!orig_arch_specified)
 	opts->x_ix86_arch_string = NULL;
 
       if (option_strings[IX86_FUNCTION_SPECIFIC_TUNE])
-	opts->x_ix86_tune_string = option_strings[IX86_FUNCTION_SPECIFIC_TUNE];
+	opts->x_ix86_tune_string
+	  = xstrdup (option_strings[IX86_FUNCTION_SPECIFIC_TUNE]);
       else if (orig_tune_defaulted)
 	opts->x_ix86_tune_string = NULL;
 

Re: [PATCH, i386]: Fix PR 67484, asan detects heap-use-after-free with target options

[Richard Biener]

On Tue, Sep 15, 2015 at 8:13 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> Hello!
>
> As mentioned in the PR, ix86_valid_target_attribute_tree creates
> temporary copies of current options strings and saves *pointers* to
> these copies with build_target_option_node. A couple of lines below,
> these temporary copies are freed, leaving dangling pointers in the
> saved structure.
>
> Use xstrndup to create permanent copy of string on the heap. This will
> however create a small leak, as this copy is never deallocated.
>
> There is no test infrastructure to check for memory errors, so there
> is no testcase added.
>
> 2015-09-15  Uros Bizjak  <ubizjak@gmail.com>
>
>     PR target/67484
>     * config/i386/i386.c (ix86_valid_target_attribute_tree):
>     Use xstrdup to copy option_strings to opts->x_ix86_arch_string and
>     opts->x_ix86_tune_string.
>
> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>
> I'll wait a couple of days for possible comments on the above solution.

I thought we have a custom destructor for target_option_node.  Ah, no,
that was for target_globals.  I suppose we could add one to cl_target_option
as well.  Note that currently the strings are not GTY((skip)) so it seems
we expect ggc allocated strings there?  Which means the xstrdup in
ix86_valid_target_attribute_inner_p should be ggc_strdup?

Richard.

> Uros.

Re: [PATCH, i386]: Fix PR 67484, asan detects heap-use-after-free with target options

[Uros Bizjak]

On Wed, Sep 16, 2015 at 10:45 AM, Richard Biener
<richard.guenther@gmail.com> wrote:

>> As mentioned in the PR, ix86_valid_target_attribute_tree creates
>> temporary copies of current options strings and saves *pointers* to
>> these copies with build_target_option_node. A couple of lines below,
>> these temporary copies are freed, leaving dangling pointers in the
>> saved structure.
>>
>> Use xstrndup to create permanent copy of string on the heap. This will
>> however create a small leak, as this copy is never deallocated.
>>
>> There is no test infrastructure to check for memory errors, so there
>> is no testcase added.
>>
>> 2015-09-15  Uros Bizjak  <ubizjak@gmail.com>
>>
>>     PR target/67484
>>     * config/i386/i386.c (ix86_valid_target_attribute_tree):
>>     Use xstrdup to copy option_strings to opts->x_ix86_arch_string and
>>     opts->x_ix86_tune_string.
>>
>> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>>
>> I'll wait a couple of days for possible comments on the above solution.
>
> I thought we have a custom destructor for target_option_node.  Ah, no,
> that was for target_globals.  I suppose we could add one to cl_target_option
> as well.  Note that currently the strings are not GTY((skip)) so it seems
> we expect ggc allocated strings there?  Which means the xstrdup in
> ix86_valid_target_attribute_inner_p should be ggc_strdup?

This is a bit over my knowledge of option processing, but please note
that the only function that performs non-recursive call to
ix86_valid_target_attribute_inner_p also frees the strings, allocated
by mentioned function.

Uros.

Re: [PATCH, i386]: Fix PR 67484, asan detects heap-use-after-free with target options

[Richard Biener]

I see in gtype-desc.c:

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_ix86_arch_string);
      gt_ggc_m_S ((*x).x_ix86_recip_name);
      gt_ggc_m_S ((*x).x_ix86_tune_ctrl_string);
      gt_ggc_m_S ((*x).x_ix86_tune_memcpy_strategy);
      gt_ggc_m_S ((*x).x_ix86_tune_memset_strategy);
      gt_ggc_m_S ((*x).x_ix86_tune_string);
    }

so it certainly does not expect heap allocated strings in
ix86_arch_string and friends.

Richard.

On Wed, Sep 16, 2015 at 10:59 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Wed, Sep 16, 2015 at 10:45 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>
>>> As mentioned in the PR, ix86_valid_target_attribute_tree creates
>>> temporary copies of current options strings and saves *pointers* to
>>> these copies with build_target_option_node. A couple of lines below,
>>> these temporary copies are freed, leaving dangling pointers in the
>>> saved structure.
>>>
>>> Use xstrndup to create permanent copy of string on the heap. This will
>>> however create a small leak, as this copy is never deallocated.
>>>
>>> There is no test infrastructure to check for memory errors, so there
>>> is no testcase added.
>>>
>>> 2015-09-15  Uros Bizjak  <ubizjak@gmail.com>
>>>
>>>     PR target/67484
>>>     * config/i386/i386.c (ix86_valid_target_attribute_tree):
>>>     Use xstrdup to copy option_strings to opts->x_ix86_arch_string and
>>>     opts->x_ix86_tune_string.
>>>
>>> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>>>
>>> I'll wait a couple of days for possible comments on the above solution.
>>
>> I thought we have a custom destructor for target_option_node.  Ah, no,
>> that was for target_globals.  I suppose we could add one to cl_target_option
>> as well.  Note that currently the strings are not GTY((skip)) so it seems
>> we expect ggc allocated strings there?  Which means the xstrdup in
>> ix86_valid_target_attribute_inner_p should be ggc_strdup?
>
> This is a bit over my knowledge of option processing, but please note
> that the only function that performs non-recursive call to
> ix86_valid_target_attribute_inner_p also frees the strings, allocated
> by mentioned function.
>
> Uros.

Re: [PATCH, i386]: Fix PR 67484, asan detects heap-use-after-free with target options

[Richard Biener]

And it is initialized via

void
cl_target_option_save (struct cl_target_option *ptr, struct gcc_options *opts)
{
  if (targetm.target_option.save)
    targetm.target_option.save (ptr, opts);

  ptr->x_recip_mask = opts->x_recip_mask;
  ptr->x_ix86_isa_flags = opts->x_ix86_isa_flags;
  ptr->x_ix86_fpmath = opts->x_ix86_fpmath;
  ptr->x_target_flags = opts->x_target_flags;
}

which uses a target hook to copy from gcc_options to cl_target_options...
(what a maze), and ix86_function_specific_save also plain copies the
pointers.

Richard.

On Wed, Sep 16, 2015 at 11:08 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> I see in gtype-desc.c:
>
> 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_ix86_arch_string);
>       gt_ggc_m_S ((*x).x_ix86_recip_name);
>       gt_ggc_m_S ((*x).x_ix86_tune_ctrl_string);
>       gt_ggc_m_S ((*x).x_ix86_tune_memcpy_strategy);
>       gt_ggc_m_S ((*x).x_ix86_tune_memset_strategy);
>       gt_ggc_m_S ((*x).x_ix86_tune_string);
>     }
>
> so it certainly does not expect heap allocated strings in
> ix86_arch_string and friends.
>
> Richard.
>
> On Wed, Sep 16, 2015 at 10:59 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Wed, Sep 16, 2015 at 10:45 AM, Richard Biener
>> <richard.guenther@gmail.com> wrote:
>>
>>>> As mentioned in the PR, ix86_valid_target_attribute_tree creates
>>>> temporary copies of current options strings and saves *pointers* to
>>>> these copies with build_target_option_node. A couple of lines below,
>>>> these temporary copies are freed, leaving dangling pointers in the
>>>> saved structure.
>>>>
>>>> Use xstrndup to create permanent copy of string on the heap. This will
>>>> however create a small leak, as this copy is never deallocated.
>>>>
>>>> There is no test infrastructure to check for memory errors, so there
>>>> is no testcase added.
>>>>
>>>> 2015-09-15  Uros Bizjak  <ubizjak@gmail.com>
>>>>
>>>>     PR target/67484
>>>>     * config/i386/i386.c (ix86_valid_target_attribute_tree):
>>>>     Use xstrdup to copy option_strings to opts->x_ix86_arch_string and
>>>>     opts->x_ix86_tune_string.
>>>>
>>>> Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>>>>
>>>> I'll wait a couple of days for possible comments on the above solution.
>>>
>>> I thought we have a custom destructor for target_option_node.  Ah, no,
>>> that was for target_globals.  I suppose we could add one to cl_target_option
>>> as well.  Note that currently the strings are not GTY((skip)) so it seems
>>> we expect ggc allocated strings there?  Which means the xstrdup in
>>> ix86_valid_target_attribute_inner_p should be ggc_strdup?
>>
>> This is a bit over my knowledge of option processing, but please note
>> that the only function that performs non-recursive call to
>> ix86_valid_target_attribute_inner_p also frees the strings, allocated
>> by mentioned function.
>>
>> Uros.

[PATCH, i386]: Fix PR 67484 (version 2)

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 221 bytes --]

Hello.

I've just applied suggested change that Richi proposed.
The patch can bootstrap on x86_64-linux-gnu and survives regression tests. Moreover,
the memory leak/invalid read has gone.

Ready for trunk?
Thanks,
Martin

[-- Attachment #2: 0001-Fix-PR-target-67484.patch --]
[-- Type: text/x-patch, Size: 1495 bytes --]

From f0eca2d3efd711498fbc8b7e8980fdf32f0abfd0 Mon Sep 17 00:00:00 2001
From: marxin <mliska@suse.cz>
Date: Thu, 10 Dec 2015 14:02:13 +0100
Subject: [PATCH] Fix PR target/67484

gcc/ChangeLog:

2015-12-10  Martin Liska  <mliska@suse.cz>
	    Uros Bizjak  <ubizjak@gmail.com>

	PR target/67484
	* config/i386/i386.c (ix86_valid_target_attribute_tree):
	Use ggc_strdup to copy option_strings to opts->x_ix86_arch_string and
	opts->x_ix86_tune_string.
---
 gcc/config/i386/i386.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index d30fbff..2eacea7 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -6152,7 +6152,7 @@ ix86_valid_target_attribute_tree (tree args,
       if (option_strings[IX86_FUNCTION_SPECIFIC_ARCH])
 	{
 	  opts->x_ix86_arch_string
-	    = option_strings[IX86_FUNCTION_SPECIFIC_ARCH];
+	    = ggc_strdup (option_strings[IX86_FUNCTION_SPECIFIC_ARCH]);
 
 	  /* If arch= is set,  clear all bits in x_ix86_isa_flags,
 	     except for ISA_64BIT, ABI_64, ABI_X32, and CODE16.  */
@@ -6166,7 +6166,8 @@ ix86_valid_target_attribute_tree (tree args,
 	opts->x_ix86_arch_string = NULL;
 
       if (option_strings[IX86_FUNCTION_SPECIFIC_TUNE])
-	opts->x_ix86_tune_string = option_strings[IX86_FUNCTION_SPECIFIC_TUNE];
+	opts->x_ix86_tune_string
+	  = ggc_strdup (option_strings[IX86_FUNCTION_SPECIFIC_TUNE]);
       else if (orig_tune_defaulted)
 	opts->x_ix86_tune_string = NULL;
 
-- 
2.6.3

Re: [PATCH, i386]: Fix PR 67484 (version 2)

[Richard Biener]

On Fri, Dec 11, 2015 at 10:10 AM, Martin Liška <mliska@suse.cz> wrote:
> Hello.
>
> I've just applied suggested change that Richi proposed.
> The patch can bootstrap on x86_64-linux-gnu and survives regression tests. Moreover,
> the memory leak/invalid read has gone.
>
> Ready for trunk?

Ok.

Thanks,
Richard.

> Thanks,
> Martin