[Bug gdb/23342] sanity check stale struct frame_info *

[Bug gdb/23342] sanity check stale struct frame_info *

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23342

--- Comment #1 from Tom Tromey <tromey at sourceware dot org> ---
I have some patches for this.

The main issue is that the destructor has to be a little non-trivial.
So for best results we'll probably need to convert a lot of parameters
to const references; but this is laborious.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/23342] sanity check stale struct frame_info *

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23342

--- Comment #2 from Tom Tromey <tromey at sourceware dot org> ---
With a doubly-linked list as the implementation maybe this isn't so awful.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/23342] sanity check stale struct frame_info *

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23342

--- Comment #3 from Tom Tromey <tromey at sourceware dot org> ---
I wonder, though, if we could go a bit further.
Rather than simply invalidating the frame_info_ptr,
what if we made it self-reinflating?
The idea would be, on invalidation, store a frame ID in the
wrapper class.
Then, operator-> could try again to find the frame_info.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/23342] sanity check stale struct frame_info *

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=23342

Simon Marchi <simark at simark dot ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |simark at simark dot ca

--- Comment #4 from Simon Marchi <simark at simark dot ca> ---
Just wondering, what mechanism do you intend to use to know whether a
frame_info pointer is still valid?  We have a frame cache generation counter
nowadays, so maybe that could be used for a quick check.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/23342] sanity check stale struct frame_info *

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23342

--- Comment #5 from Tom Tromey <tromey at sourceware dot org> ---
(In reply to Simon Marchi from comment #4)
> Just wondering, what mechanism do you intend to use to know whether a
> frame_info pointer is still valid?  We have a frame cache generation counter
> nowadays, so maybe that could be used for a quick check.

The current patch has a smart-pointer class, "frame_info_ptr".
These register themselves on a doubly-linked list at construction.
Then reinit_frame_cache walks this list and nulls out the values.
So, it's just impossible to reuse one now.

I didn't think of the generation counter, but that would work too,
at the cost of a check at each dereference.
Maybe that's better, though, I'm not sure.
It's very easy to change.


The more I consider the "automatic reinflating" idea, the more I like
it.  It would turn an assertion failure / crash into "what the code
should be doing anyway".

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/23342] sanity check stale struct frame_info *

[jan.kratochvil at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23342

Jan Kratochvil <jan.kratochvil at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jan.kratochvil at redhat dot com

--- Comment #6 from Jan Kratochvil <jan.kratochvil at redhat dot com> ---
(In reply to Tom Tromey from comment #5)
> The more I consider the "automatic reinflating" idea, the more I like
> it.  It would turn an assertion failure / crash into "what the code
> should be doing anyway".

But then one will not find out the code is not handling invalidated frame
pointers in a reasonable (non-crashing) way. Maybe it could be configurable by
some --enable-maintainer-mode.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/23342] sanity check stale struct frame_info *

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23342

--- Comment #7 from Tom Tromey <tromey at sourceware dot org> ---
> I didn't think of the generation counter, but that would work too,
> at the cost of a check at each dereference.
> Maybe that's better, though, I'm not sure.
> It's very easy to change.

I'm going to do this, because the re-inflating idea will mean
a check at dereference time anyway.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/23342] sanity check stale struct frame_info *

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23342

--- Comment #8 from Tom Tromey <tromey at sourceware dot org> ---
(In reply to Jan Kratochvil from comment #6)
> (In reply to Tom Tromey from comment #5)
> > The more I consider the "automatic reinflating" idea, the more I like
> > it.  It would turn an assertion failure / crash into "what the code
> > should be doing anyway".
> 
> But then one will not find out the code is not handling invalidated frame
> pointers in a reasonable (non-crashing) way. Maybe it could be configurable
> by some --enable-maintainer-mode.

True, but my thinking here was that it doesn't matter.

The normal bug is that some code holds onto a frame_info* that is
the invalidated.  Then, it uses the pointer, resulting in a UAF.

The patch I have now turns the UAF into a crash, by turning the
dangling pointer into a NULL pointer.  Good so far.

The next step is that we would normally fix this kind of bug by
computing the frame_id and storing it, then using the frame_id to
look up the frame again.

The proposed "reinflation" approach is to automate this step.
It would just invisibly do what you were supposed to do anyway.

Is there ever a time when we wouldn't want to do this?
Or when it would cause some other bug?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/23342] sanity check stale struct frame_info *

[jan.kratochvil at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=23342

--- Comment #9 from Jan Kratochvil <jan.kratochvil at redhat dot com> ---
(In reply to Tom Tromey from comment #8)
> The proposed "reinflation" approach is to automate this step.
> It would just invisibly do what you were supposed to do anyway.

I believe in some rare cases this reinflation will fail. Such as when one makes
an inferior call which is normally OK but occasionally it may corrupt stack.

Or it could throw an exception in such case but I do not know if GDB codebase
is already exception-safe.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/23342] sanity check stale struct frame_info *

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=23342

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
   Target Milestone|---                         |13.1
         Resolution|---                         |FIXED

--- Comment #10 from Tom Tromey <tromey at sourceware dot org> ---
This went in, in a form that seems basically acceptable,
so I'm closing this bug.

-- 
You are receiving this mail because:
You are on the CC list for the bug.