Re: [PATCH 3/3] gdb, gdbserver: make target_waitstatus safe

[John Baldwin <jhb@FreeBSD.org>]

On 10/12/21 9:17 AM, Simon Marchi via Gdb-patches wrote:
> From: Simon Marchi <simon.marchi@polymtl.ca>
> 
> I stumbled on a bug caused by the fact that a code path read
> target_waitstatus::value::sig (expecting it to contain a gdb_signal
> value) while target_waitstatus::kind was TARGET_WAITKIND_FORKED.  This
> meant that the active union field was in fact
> target_waitstatus::value::related_pid, and contained a ptid.  The read
> signal value was therefore garbage, and that caused GDB to crash soon
> after.  Or, since that GDB was built with ubsan, this nice error
> message:
> 
>      /home/simark/src/binutils-gdb/gdb/linux-nat.c:1271:12: runtime error: load of value 2686365, which is not a valid value for type 'gdb_signal'
> 
> Despite being a large-ish change, I think it would be nice to make
> target_waitstatus safe against that kind of bug.  As already done
> elsewhere (e.g. dynamic_prop), validate that the type of value read from
> the union matches what is supposed to be the active field.
> 
>   - Make the kind and value of target_waitstatus private.
>   - Make the kind initialized to TARGET_WAITKIND_IGNORE on
>     target_waitstatus construction.  This is what most users appear to do
>     explicitly.
>   - Add setters, one for each kind.  Each setter takes as a parameter the
>     data associated to that kind, if any.  This makes it impossible to
>     forget to attach the associated data.
>   - Add getters, one for each associated data type.  Each getter
>     validates that the data type fetched by the user matches the wait
>     status kind.
>   - Change "integer" to "exit_status", "related_pid" to "child_ptid",
>     just because that's more precise terminology.
>   - Fix all users.
> 
> That last point is semi-mechanical.  There are a lot of obvious changes,
> but some less obvious ones.  For example, it's not possible to set the
> kind at some point and the associated data later, as some users did.
> But in any case, the intent of the code should not change in this patch.
> 
> This was tested on x86-64 Linux (unix, native-gdbserver and
> native-extended-gdbserver boards).  It was built-tested on x86-64
> FreeBSD, NetBSD, MinGW and macOS.  The rest of the changes to native
> files was done as a best effort.  If I forgot any place to update in
> these files, it should be easy to fix (unless the change happens to
> reveal an actual bug).

I think this is a good change.  I only read over fbsd-nat.c and I think
I like the idea.  One thing that does stick out to me is the assymetry
in that we no longer use TARGET_WAITKIND_* for setting the status, only
when comparing the result of kind().  I wonder if instead you might
consider adding type specific queries (e.g. is_ignored(), is_forked())
and removing TARGET_WAITKIND_* as a public interface entirely?

-- 
John Baldwin