Re: [PATCH] Fix value chain use-after-free

[Alexandra Petlanova Hajkova <ahajkova@redhat.com>]

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

On Sat, Feb 11, 2023 at 2:08 AM Tom Tromey <tom@tromey.com> wrote:

> Hannes filed a bug showing a crash, where a pretty-printer written in
> Python could cause a use-after-free.  He sent a patch, but I thought a
> different approach was needed.
>
> In a much earlier patch (see bug #12533), we changed the Python code
> to release new values from the value chain when constructing a
> gdb.Value.  The rationale for this is that if you write a command that
> does a lot of computations in a loop, all the values will be kept live
> by the value chain, resulting in gdb using a large amount of memory.
>
> However, suppose a value is passed to Python from some code in gdb
> that needs to use the value after the call into Python.  In this
> scenario, value_to_value_object will still release the value -- and
> because gdb code doesn't generally keep strong references to values (a
> consequence of the ancient decision to use the value chain to avoid
> memory management), this will result in a use-after-free.
>
> This scenario can happen, as it turns out, when a value is passed to
> Python for pretty-printing.  Now, normally this route boxes the value
> via value_to_value_object_no_release, avoiding the problematic release
> from the value chain.  However, if you then call Value.cast, the
> underlying value API might return the same value, when is then
> released from the chain.
>
> This patch fixes the problem by changing how value boxing is done.
> value_to_value_object no longer removes a value from the chain.
> Instead, every spot in gdb that might construct new values uses a
> scoped_value_mark to ensure that the requirements of bug #12533 are
> met.  And, because incoming values aren't ever released from the chain
> (the Value.cast one comes earlier on the chain than the
> scoped_value_mark), the bug can no longer occur.  (Note that many
> spots in the Python layer already take this approach, so not many
> places needed to be touched.)
>
> In the future I think we should replace the use of raw "value *" with
> value_ref_ptr pretty much everywhere.  This will ensure lifetime
> safety throughout gdb.
>
> The test case in this patch comes from Hannes' original patch.  I only
> made a trivial ("require") change to it.  However, while this fails
> for him, I can't make it fail on this machine; nevertheless, he tried
> my patch and reported the bug as being fixed.
>
> 2.39.1
>
>
I think it's a good solution and I can confirm gdb.python/py-pp-cast.exp
passes for me. But I wasn't able to apply the patch cleanly, I think it
needs to be rebased.