This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] fix pr62009 use after free in redirect_edge_var_map_dup


On Tue, Aug 5, 2014 at 1:17 PM, Trevor Saunders <tsaunders@mozilla.com> wrote:
> On Tue, Aug 05, 2014 at 10:45:38AM +0200, Richard Biener wrote:
>> On Mon, Aug 4, 2014 at 3:04 PM,  <tsaunders@mozilla.com> wrote:
>> > From: Trevor Saunders <tsaunders@mozilla.com>
>> >
>> > Hi,
>> >
>> > It used to be that edge_var_maps held pointers to embedded vectors, but
>> > now it holds vectors.  This means that now instead of copying the
>> > address of the embedded vector from the table we keep a pointer into the
>> > table.  However that's incorrect because we may expand the table when
>> > inserting new into the map in which case our pointer into the map points
>> > at freed memory.
>> >
>> > gcc/
>> >
>> >         * tree-ssa.c (redirect_edge_var_map_dup): copy the value in the
>> >           map for old before inserting new.
>> >
>> > testing ongoing on x86_64-unknown-linux-gnu, ok?
>>
>> Umm... can you explore what it takes to instead store some kind of
>> indexes?
>
> into what?

No idea - I didn't look into the affected code not do I remember it
off-head.

>> That is, how is the above not an issue for other 'head's that may be
>> live somewhere?
>
> what do you mean by other head's? Presumably nobody did something like
>
> void **p = pointer_map_contains (t, x);
> void **q = pointer_map_insert (t, y);
> *q = *p;
>
> because that would be broken, and that's the same problem we have here.
> So assuming I didn't break anything else nobody else uses a pointer into
> the table that they have after inserting elements.

Ah, now I understand.  I thought somebody might hold onto 'p' to
some other random element.  At least if that was valid "before".

If not the patch is ok.

Thanks,
Richard.

>> Or can you restore what "used to be"?
>
> I thought I had to bail out early if the map didn't contain old, but
> when I actually look at the ddiff it looks like we delt with this before
> by just inserting new and then getting old so yeah I can just make
> things work more like they used to.  Though maybe at some point we want
> to try and have less elements in the table with empty vectors.
>
> Trev
>
>>
>> Thanks,
>> Richard.
>>
>> > Trev
>> >
>> > ---
>> >  gcc/tree-ssa.c | 6 +++++-
>> >  1 file changed, 5 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/gcc/tree-ssa.c b/gcc/tree-ssa.c
>> > index 920cbea..b949d48 100644
>> > --- a/gcc/tree-ssa.c
>> > +++ b/gcc/tree-ssa.c
>> > @@ -109,7 +109,11 @@ redirect_edge_var_map_dup (edge newe, edge olde)
>> >    if (!head)
>> >      return;
>> >
>> > -  edge_var_maps->get_or_insert (newe).safe_splice (*head);
>> > +  /* Save what head points at because if we need to insert new into the map we
>> > +     may end up expanding the table in which case head will no longer point at
>> > +     valid memory.  */
>> > +  vec<edge_var_map> h = *head;
>> > +  edge_var_maps->get_or_insert (newe).safe_splice (h);
>> >  }
>> >
>> >
>> > --
>> > 2.0.1
>> >


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]