[10/13] gdbsupport: add xstrcpy

  Add xstrcpy, a "safe" alternative to strcpy.  It works like strcpy, but
accepts the size of the destination buffer, and asserts that the string
fits in it.

Return the number of characters copied, so that it's possible to easily
chain calls like this:

  p += xstrcpy (p, end - p, ",C");

Context: I want to replace some code that uses strcpy and strcat to
build strings with something that has bound checks.  We already have
xsnprintf, but sometimes we want to add fixed strings, so xsnprintf is a
bit overkill.  xstrcpy is essentially xsnprintf but without the
formatting.

I looked around and changed a bunch of calls to xsnprintf that can be
replaced one for one with xstrcpy.

Change-Id: Icff6978b7431581732184d045c6ec047e2c78bcb
---
 gdb/fbsd-nat.c                         |  2 +-
 gdb/nat/netbsd-nat.c                   |  2 +-
 gdb/remote.c                           | 67 ++++++++++++--------------
 gdb/unittests/common-utils-selftests.c | 29 +++++++++++
 gdbsupport/agent.cc                    |  2 +-
 gdbsupport/common-utils.cc             | 13 +++++
 gdbsupport/common-utils.h              |  9 ++++
 gdbsupport/ptid.cc                     |  4 +-
 8 files changed, 88 insertions(+), 40 deletions(-)
  

Simon Marchi <simon.marchi@efficios.com> writes:

> Add xstrcpy, a "safe" alternative to strcpy.  It works like strcpy, but
> accepts the size of the destination buffer, and asserts that the string
> fits in it.
>
> Return the number of characters copied, so that it's possible to easily
> chain calls like this:
>
>   p += xstrcpy (p, end - p, ",C");

Don't functions that take a buffer size usually include an 'n' in the
name.  That seems to be true for libc, but also throughout GDB.  Would
it not be a good idea to adopt that here too.

This isn't exactly strncpy, but it seems similar.  While strncpy can
result in a non-null terminated output string, this "xstrncpy" asserts
that the source fits into the output buffer without being truncated.
But otherwise, it's the same function I think?

Thanks,
Andrew


>
> Context: I want to replace some code that uses strcpy and strcat to
> build strings with something that has bound checks.  We already have
> xsnprintf, but sometimes we want to add fixed strings, so xsnprintf is a
> bit overkill.  xstrcpy is essentially xsnprintf but without the
> formatting.
>
> I looked around and changed a bunch of calls to xsnprintf that can be
> replaced one for one with xstrcpy.
>
> Change-Id: Icff6978b7431581732184d045c6ec047e2c78bcb
> ---
>  gdb/fbsd-nat.c                         |  2 +-
>  gdb/nat/netbsd-nat.c                   |  2 +-
>  gdb/remote.c                           | 67 ++++++++++++--------------
>  gdb/unittests/common-utils-selftests.c | 29 +++++++++++
>  gdbsupport/agent.cc                    |  2 +-
>  gdbsupport/common-utils.cc             | 13 +++++
>  gdbsupport/common-utils.h              |  9 ++++
>  gdbsupport/ptid.cc                     |  4 +-
>  8 files changed, 88 insertions(+), 40 deletions(-)
>
> diff --git a/gdb/fbsd-nat.c b/gdb/fbsd-nat.c
> index cf59ae21efc0..2c914c0e75be 100644
> --- a/gdb/fbsd-nat.c
> +++ b/gdb/fbsd-nat.c
> @@ -897,7 +897,7 @@ fbsd_nat_target::thread_name (struct thread_info *thr)
>      return nullptr;
>    if (streq (kp.ki_comm, pl.pl_tdname))
>      return NULL;
> -  xsnprintf (buf, sizeof buf, "%s", pl.pl_tdname);
> +  xstrcpy (buf, sizeof buf, pl.pl_tdname);
>    return buf;
>  }
>  #endif
> diff --git a/gdb/nat/netbsd-nat.c b/gdb/nat/netbsd-nat.c
> index 2fe2889d7129..d88dd05cbf57 100644
> --- a/gdb/nat/netbsd-nat.c
> +++ b/gdb/nat/netbsd-nat.c
> @@ -135,7 +135,7 @@ thread_name (ptid_t ptid)
>        {
>  	if (kl->l_lid == lwp)
>  	  {
> -	    xsnprintf (buf, sizeof buf, "%s", kl->l_name);
> +	    xstrcpy (buf, sizeof buf, kl->l_name);
>  	    return true;
>  	  }
>  	return false;
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 194c4cbd9bb0..3d38a9c7c8c9 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -3045,7 +3045,7 @@ remote_target::remote_query_attached (int pid)
>    if (m_features.remote_multi_process_p ())
>      xsnprintf (rs->buf.data (), size, "qAttached:%x", pid);
>    else
> -    xsnprintf (rs->buf.data (), size, "qAttached");
> +    xstrcpy (rs->buf.data (), size, "qAttached");
>  
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
> @@ -3509,11 +3509,11 @@ remote_target::set_thread (ptid_t ptid, int gen)
>    *buf++ = 'H';
>    *buf++ = gen ? 'g' : 'c';
>    if (ptid == magic_null_ptid)
> -    xsnprintf (buf, endbuf - buf, "0");
> +    xstrcpy (buf, endbuf - buf, "0");
>    else if (ptid == any_thread_ptid)
> -    xsnprintf (buf, endbuf - buf, "0");
> +    xstrcpy (buf, endbuf - buf, "0");
>    else if (ptid == minus_one_ptid)
> -    xsnprintf (buf, endbuf - buf, "-1");
> +    xstrcpy (buf, endbuf - buf, "-1");
>    else
>      write_ptid (buf, endbuf, ptid);
>    putpkt (rs->buf);
> @@ -4633,8 +4633,7 @@ remote_target::extra_thread_info (thread_info *tp)
>        char *b = rs->buf.data ();
>        char *endb = b + get_remote_packet_size ();
>  
> -      xsnprintf (b, endb - b, "qThreadExtraInfo,");
> -      b += strlen (b);
> +      b += xstrcpy (b, endb - b, "qThreadExtraInfo,");
>        write_ptid (b, endb, tp->ptid);
>  
>        putpkt (rs->buf);
> @@ -4682,8 +4681,7 @@ remote_target::static_tracepoint_marker_at (CORE_ADDR addr,
>    struct remote_state *rs = get_remote_state ();
>    char *p = rs->buf.data ();
>  
> -  xsnprintf (p, get_remote_packet_size (), "qTSTMat:");
> -  p += strlen (p);
> +  p += xstrcpy (p, get_remote_packet_size (), "qTSTMat:");
>    p += hexnumstr (p, addr);
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
> @@ -7215,14 +7213,14 @@ remote_target::append_resumption (char *p, char *endp,
>  				   addr_size));
>  	}
>        else
> -	p += xsnprintf (p, endp - p, ";s");
> +	p += xstrcpy (p, endp - p, ";s");
>      }
>    else if (step)
> -    p += xsnprintf (p, endp - p, ";s");
> +    p += xstrcpy (p, endp - p, ";s");
>    else if (siggnal != GDB_SIGNAL_0)
>      p += xsnprintf (p, endp - p, ";C%02x", siggnal);
>    else
> -    p += xsnprintf (p, endp - p, ";c");
> +    p += xstrcpy (p, endp - p, ";c");
>  
>    if (m_features.remote_multi_process_p () && ptid.is_pid ())
>      {
> @@ -7231,12 +7229,12 @@ remote_target::append_resumption (char *p, char *endp,
>        /* All (-1) threads of process.  */
>        nptid = ptid_t (ptid.pid (), -1);
>  
> -      p += xsnprintf (p, endp - p, ":");
> +      p += xstrcpy (p, endp - p, ":");
>        p = write_ptid (p, endp, nptid);
>      }
>    else if (ptid != minus_one_ptid)
>      {
> -      p += xsnprintf (p, endp - p, ":");
> +      p += xstrcpy (p, endp - p, ":");
>        p = write_ptid (p, endp, ptid);
>      }
>  
> @@ -7357,7 +7355,7 @@ remote_target::remote_resume_with_vcont (ptid_t scope_ptid, int step,
>       about overflowing BUF.  Should there be a generic
>       "multi-part-packet" packet?  */
>  
> -  p += xsnprintf (p, endp - p, "vCont");
> +  p += xstrcpy (p, endp - p, "vCont");
>  
>    if (scope_ptid == magic_null_ptid)
>      {
> @@ -7535,7 +7533,7 @@ vcont_builder::restart ()
>  
>    m_p = rs->buf.data ();
>    m_endp = m_p + m_remote->get_remote_packet_size ();
> -  m_p += xsnprintf (m_p, m_endp - m_p, "vCont");
> +  m_p += xstrcpy (m_p, m_endp - m_p, "vCont");
>    m_first_action = m_p;
>  }
>  
> @@ -7889,12 +7887,12 @@ remote_target::remote_stop_ns (ptid_t ptid)
>  
>    if (ptid == minus_one_ptid
>        || (!m_features.remote_multi_process_p () && ptid.is_pid ()))
> -    p += xsnprintf (p, endp - p, "vCont;t");
> +    p += xstrcpy (p, endp - p, "vCont;t");
>    else
>      {
>        ptid_t nptid;
>  
> -      p += xsnprintf (p, endp - p, "vCont;t:");
> +      p += xstrcpy (p, endp - p, "vCont;t:");
>  
>        if (ptid.is_pid ())
>  	  /* All (-1) threads of process.  */
> @@ -7955,7 +7953,7 @@ remote_target::remote_interrupt_ns ()
>    char *p = rs->buf.data ();
>    char *endp = p + get_remote_packet_size ();
>  
> -  xsnprintf (p, endp - p, "vCtrlC");
> +  xstrcpy (p, endp - p, "vCtrlC");
>  
>    /* In non-stop, we get an immediate OK reply.  The stop reply will
>       come in asynchronously by notification.  */
> @@ -9313,7 +9311,7 @@ remote_target::send_g_packet ()
>    struct remote_state *rs = get_remote_state ();
>    int buf_len;
>  
> -  xsnprintf (rs->buf.data (), get_remote_packet_size (), "g");
> +  xstrcpy (rs->buf.data (), get_remote_packet_size (), "g");
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>    packet_result result = packet_check_result (rs->buf);
> @@ -11342,8 +11340,8 @@ remote_target::extended_remote_set_inferior_cwd ()
>  	{
>  	  /* An empty inferior_cwd means that the user wants us to
>  	     reset the remote server's inferior's cwd.  */
> -	  xsnprintf (rs->buf.data (), get_remote_packet_size (),
> -		     "QSetWorkingDir:");
> +	  xstrcpy (rs->buf.data (), get_remote_packet_size (),
> +		   "QSetWorkingDir:");
>  	}
>  
>        putpkt (rs->buf);
> @@ -11529,8 +11527,7 @@ remote_add_target_side_condition (struct gdbarch *gdbarch,
>      return 0;
>  
>    buf += strlen (buf);
> -  xsnprintf (buf, buf_end - buf, "%s", ";");
> -  buf++;
> +  buf += xstrcpy (buf, buf_end - buf, ";");
>  
>    /* Send conditions to the target.  */
>    for (agent_expr *aexpr : bp_tgt->conditions)
> @@ -14990,7 +14987,7 @@ remote_target::get_min_fast_tracepoint_insn_len ()
>    /* Make sure the remote is pointing at the right process.  */
>    set_general_process ();
>  
> -  xsnprintf (rs->buf.data (), get_remote_packet_size (), "qTMinFTPILen");
> +  xstrcpy (rs->buf.data (), get_remote_packet_size (), "qTMinFTPILen");
>    putpkt (rs->buf);
>    reply = remote_get_noisy_reply ();
>    if (*reply == '\0')
> @@ -15015,7 +15012,7 @@ remote_target::set_trace_buffer_size (LONGEST val)
>        char *endbuf = buf + get_remote_packet_size ();
>  
>        gdb_assert (val >= 0 || val == -1);
> -      buf += xsnprintf (buf, endbuf - buf, "QTBuffer:size:");
> +      buf += xstrcpy (buf, endbuf - buf, "QTBuffer:size:");
>        /* Send -1 as literal "-1" to avoid host size dependency.  */
>        if (val < 0)
>  	{
> @@ -15049,24 +15046,24 @@ remote_target::set_trace_notes (const char *user, const char *notes,
>    char *endbuf = buf + get_remote_packet_size ();
>    int nbytes;
>  
> -  buf += xsnprintf (buf, endbuf - buf, "QTNotes:");
> +  buf += xstrcpy (buf, endbuf - buf, "QTNotes:");
>    if (user)
>      {
> -      buf += xsnprintf (buf, endbuf - buf, "user:");
> +      buf += xstrcpy (buf, endbuf - buf, "user:");
>        nbytes = bin2hex ((gdb_byte *) user, buf, strlen (user));
>        buf += 2 * nbytes;
>        *buf++ = ';';
>      }
>    if (notes)
>      {
> -      buf += xsnprintf (buf, endbuf - buf, "notes:");
> +      buf += xstrcpy (buf, endbuf - buf, "notes:");
>        nbytes = bin2hex ((gdb_byte *) notes, buf, strlen (notes));
>        buf += 2 * nbytes;
>        *buf++ = ';';
>      }
>    if (stop_notes)
>      {
> -      buf += xsnprintf (buf, endbuf - buf, "tstop:");
> +      buf += xstrcpy (buf, endbuf - buf, "tstop:");
>        nbytes = bin2hex ((gdb_byte *) stop_notes, buf, strlen (stop_notes));
>        buf += 2 * nbytes;
>        *buf++ = ';';
> @@ -15661,8 +15658,8 @@ remote_target::enable_btrace (thread_info *tp,
>    ptid_t ptid = tp->ptid;
>    set_general_thread (ptid);
>  
> -  buf += xsnprintf (buf, endbuf - buf, "%s",
> -		    packets_descriptions[which_packet].name);
> +  buf += xstrcpy (buf, endbuf - buf,
> +		  packets_descriptions[which_packet].name);
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>  
> @@ -15702,8 +15699,8 @@ remote_target::disable_btrace (struct btrace_target_info *tinfo)
>  
>    set_general_thread (tinfo->ptid);
>  
> -  buf += xsnprintf (buf, endbuf - buf, "%s",
> -		    packets_descriptions[PACKET_Qbtrace_off].name);
> +  buf += xstrcpy (buf, endbuf - buf,
> +		  packets_descriptions[PACKET_Qbtrace_off].name);
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
>  
> @@ -16074,8 +16071,8 @@ remote_target::commit_requested_thread_options ()
>        char *obuf_endp = obuf + max_options_size;
>  
>        *obuf_p++ = ';';
> -      obuf_p += xsnprintf (obuf_p, obuf_endp - obuf_p, "%s",
> -			   phex_nz (options));
> +      obuf_p += xstrcpy (obuf_p, obuf_endp - obuf_p,
> +			 phex_nz (options));
>        if (tp.ptid != magic_null_ptid)
>  	{
>  	  *obuf_p++ = ':';
> diff --git a/gdb/unittests/common-utils-selftests.c b/gdb/unittests/common-utils-selftests.c
> index eb9c83616f08..4940e412a41a 100644
> --- a/gdb/unittests/common-utils-selftests.c
> +++ b/gdb/unittests/common-utils-selftests.c
> @@ -125,6 +125,34 @@ string_vappendf_tests ()
>    test_appendf_func (string_vappendf_wrapper);
>  }
>  
> +static void
> +xstrcpy_tests ()
> +{
> +  char buf[8];
> +  char *p;
> +  char *end = buf + sizeof (buf);
> +
> +  memset (buf, 'x', sizeof (buf));
> +  p = buf;
> +  p += xstrcpy (p, end - p, "ab");
> +  SELF_CHECK (p == buf + 2);
> +  p += xstrcpy (p, end - p, "cd");
> +  SELF_CHECK (p == buf + 4);
> +  SELF_CHECK (strcmp (buf, "abcd") == 0);
> +
> +  /* A string of exactly SIZE - 1 characters fits.  */
> +  memset (buf, 'x', sizeof (buf));
> +  p = buf;
> +  SELF_CHECK (xstrcpy (p, end - p, "1234567") == 7);
> +  SELF_CHECK (strcmp (buf, "1234567") == 0);
> +
> +  /* An empty string is fine, even in a buffer of size 1.  */
> +  memset (buf, 'x', sizeof (buf));
> +  p = buf;
> +  SELF_CHECK (xstrcpy (p, 1, "") == 0);
> +  SELF_CHECK (strcmp (p, "") == 0);
> +}
> +
>  } /* namespace selftests */
>  
>  INIT_GDB_FILE (common_utils_selftests)
> @@ -134,4 +162,5 @@ INIT_GDB_FILE (common_utils_selftests)
>    selftests::register_test ("string_appendf", selftests::string_appendf_tests);
>    selftests::register_test ("string_vappendf",
>  			    selftests::string_vappendf_tests);
> +  selftests::register_test ("xstrcpy", selftests::xstrcpy_tests);
>  }
> diff --git a/gdbsupport/agent.cc b/gdbsupport/agent.cc
> index 44b6fcdcf5c7..2054815bfc8b 100644
> --- a/gdbsupport/agent.cc
> +++ b/gdbsupport/agent.cc
> @@ -154,7 +154,7 @@ gdb_connect_sync_socket (int pid)
>  
>    addr.sun_family = AF_UNIX;
>  
> -  res = xsnprintf (addr.sun_path, UNIX_PATH_MAX, "%s", path);
> +  res = xstrcpy (addr.sun_path, UNIX_PATH_MAX, path);
>    if (res >= UNIX_PATH_MAX)
>      {
>        warning (_("string overflow allocating socket name"));
> diff --git a/gdbsupport/common-utils.cc b/gdbsupport/common-utils.cc
> index f31699be13a1..4aeaaf99f787 100644
> --- a/gdbsupport/common-utils.cc
> +++ b/gdbsupport/common-utils.cc
> @@ -86,6 +86,19 @@ xsnprintf (char *str, size_t size, const char *format, ...)
>    return ret;
>  }
>  
> +/* See common-utils.h.  */
> +
> +int
> +xstrcpy (char *str, size_t size, const char *src)
> +{
> +  size_t len = strlen (src);
> +
> +  gdb_assert (len < size);
> +  memcpy (str, src, len + 1);
> +
> +  return len;
> +}
> +
>  /* See documentation in common-utils.h.  */
>  
>  std::string
> diff --git a/gdbsupport/common-utils.h b/gdbsupport/common-utils.h
> index de83a715ac45..0c4dcb8efe86 100644
> --- a/gdbsupport/common-utils.h
> +++ b/gdbsupport/common-utils.h
> @@ -51,6 +51,15 @@ gdb::unique_xmalloc_ptr<char> xstrvprintf (const char *format, va_list ap)
>  int xsnprintf (char *str, size_t size, const char *format, ...)
>       ATTRIBUTE_PRINTF (3, 4);
>  
> +/* Like strcpy, but takes the size of the destination buffer STR as SIZE,
> +   and throws an error if SRC does not fit in it.
> +
> +   Return the number of characters copied, excluding the terminating null
> +   character.
> +
> +   This is equivalent to xsnprintf when no formatting is needed.  */
> +int xstrcpy (char *str, size_t size, const char *src);
> +
>  /* Returns a std::string built from a printf-style format string.  */
>  std::string string_printf (const char* fmt, ...)
>    ATTRIBUTE_PRINTF (1, 2);
> diff --git a/gdbsupport/ptid.cc b/gdbsupport/ptid.cc
> index 933e441f9b87..d5a5fce4c123 100644
> --- a/gdbsupport/ptid.cc
> +++ b/gdbsupport/ptid.cc
> @@ -38,12 +38,12 @@ ptid_t::to_rsp_string (bool multi) const
>    if (multi)
>      {
>        if (m_pid == -1)
> -	buf += xsnprintf (buf, endbuf - buf, "p-1.");
> +	buf += xstrcpy (buf, endbuf - buf, "p-1.");
>        else
>  	buf += xsnprintf (buf, endbuf - buf, "p%x.", (unsigned) m_pid);
>      }
>    if (m_lwp == -1)
> -    xsnprintf (buf, endbuf - buf, "-1");
> +    xstrcpy (buf, endbuf - buf, "-1");
>    else
>      xsnprintf (buf, endbuf - buf, "%lx", (unsigned long) m_lwp);
>  
> -- 
> 2.55.0
  

On 8/17/26 12:45 PM, Andrew Burgess wrote:
> Simon Marchi <simon.marchi@efficios.com> writes:
> 
>> Add xstrcpy, a "safe" alternative to strcpy.  It works like strcpy, but
>> accepts the size of the destination buffer, and asserts that the string
>> fits in it.
>>
>> Return the number of characters copied, so that it's possible to easily
>> chain calls like this:
>>
>>   p += xstrcpy (p, end - p, ",C");
> 
> Don't functions that take a buffer size usually include an 'n' in the
> name.  That seems to be true for libc, but also throughout GDB.  Would
> it not be a good idea to adopt that here too.
> 
> This isn't exactly strncpy, but it seems similar.  While strncpy can
> result in a non-null terminated output string, this "xstrncpy" asserts
> that the source fits into the output buffer without being truncated.
> But otherwise, it's the same function I think?

I hesitated about naming it strncpy.  I decided against it because the
new function doesn't behave exactly like strcnpy on one specific point:
if the source is smaller than destination, strncpy fills the remainder
of the destination buffer with zeroes.  Not sure if that matters in
practice, but it could be the source of a subtle bug if one blindly
switches strncpy for xstrncpy.

Both strcpy and strncpy return a pointer to the beginning of the
destination buffer, which is different than my xstrcpy, which returns
the number of bytes written.  The latter seems more useful to me, as
it makes it possible to easily chain the calls.

It seems like xstrcpy is more like a wrapper for strlcpy, so we could
always call it xstrlcpy.  The only thing is that the argument order is
not the same:

  int    xstrcpy (char *dst, size_t size, const char *src);
  size_t strlcpy (char *dst, const char *src, size_t size);

So if we named it xstrlcpy, I would want to match xstrlcpy's argument
order, otherwise it's just confusing.  Personally, I don't strlcpy's
argument order as much, because `size` describes `dst`, so I like having
it right next to it.  But I could live with it.

I just noticed that strcpy_s exists in C11, and it is in the same order
as xstrcpy:

  errno_t strcpy_s (char* restrict dest, rsize_t destsz, const char* restrict src);

It looks like we could use that, but we also want to return the number
of bytes written, which this does not provide.

I also considered accepting `dst` as a `gdb::array_view<char>`, which
would side-step the argument order problem, but the caller's are not
really ready for that, so their would look look awkward.

In any case, I still believe that the xstrcpy makes sense, because it is
really like "strcpy, but safe".

Given all this, what would be your choice?

Simon