[PATCH 10/13] gdbsupport: add xstrcpy

[Simon Marchi <simon.marchi@efficios.com>]

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(-)

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