Re: [PATCH] btrace, frame: fix crash in get_frame_type

[Pedro Alves <palves@redhat.com>]

On 01/26/2016 05:08 PM, Markus Metzger wrote:
> In skip_artificial_frames we repeatedly call get_prev_frame_always until we get
> a non-inline and non-tailcall frame assuming that there must be such a frame
> eventually.
> 
> For record targets, however, we may have a frame chain that consists only of
> inline or tailcall frames.
> 
> This leads to a crash in get_frame_type when we dereference a NULL frame
> pointer.
> 
> The comment on skip_artificial_frames says
> 
> /* Given FRAME, return the enclosing frame as found in real frames read-in from
>    inferior memory.  Skip any previous frames which were made up by GDB.
>    Return the original frame if no immediate previous frames exist.  */
> 
> That last part, "return the original frame if no immediate previous frames
> exist", is missing.  I added that.

Not sure about this.  Why does it make sense to return the original frame?
It sounds arbitrary -- could just as well be the outermost?  What does the
caller in question do with it, and why is it correct?

> I found two other places where get_frame_type is called in a similar setup to
> skip tailcall frames:
> 
>   - in infcmd.c to implement the "finish" command
>   - in frame.c's pop_frame which is used by the "return" command
> 
> In both cases I added a NULL pointer check for the frame and throw an error in
> case we don't find a non-tailcall frame.

>    /* Ignore TAILCALL_FRAME type frames, they were executed already before
>       entering THISFRAME.  */
> -  while (get_frame_type (prev_frame) == TAILCALL_FRAME)
> +  while (prev_frame != NULL && get_frame_type (prev_frame) == TAILCALL_FRAME)
>      prev_frame = get_prev_frame (prev_frame);
>
> +  /* We cannot pop tailcall frames.  */
> +  if (prev_frame == NULL)
> +    error (_("Cannot pop tailcall frame(s)."));
> +

How about factoring that out to a skip_tailcall_frames
function, similar to skip_artificial_frames, and then do:

  prev_frame = skip_tailcall_frames (prev_frame);
  if (prev_frame == NULL)
    error (_("Cannot pop tailcall frame(s)."));

here and similarly in the other case.

And I wonder whether we should be using get_prev_frame_always
for this too, like skip_artificial_frames uses.

> 
> In infcmd I further moved the tailcall-frame-chasing loop to the
> forward-stepping case since we don't need a frame for reverse execution and we
> don't want to fail because of that.  Reverse-finish does make sense for a
> tailcall frame.


Thanks,
Pedro Alves