Re: [PATCH 10/13] gdbsupport: add xstrcpy

Simon Marchi <[email protected]> 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