[PATCH 10/13] gdbsupport: add xstrcpy
Andrew Burgess
aburgess@redhat.com
Mon Aug 17 16:45:46 GMT 2026
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
More information about the Binutils
mailing list