Re: [PATCH 12/13] gdb/remote: remove uses of sprintf

[Andrew Burgess <aburgess@redhat.com>]

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

> When building on macOS, I get some:
>
>     /Users/smarchi/src/binutils-gdb/gdb/remote.c:11556:3: error: 'sprintf' is deprecated: This function is provided for compatibility reasons only.  Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Werror,-Wdeprecated-declarations]
>      11556 |   sprintf (buf, ";cmds:%x,", bp_tgt->persist);
>            |   ^
>
> Both are in remote_add_target_side_commands, which unlike the similar
> remote_add_target_side_condition, does not receive the end of the
> packet, and does not bound its writes.  Give it a BUF_END parameter,
> and use xsnprintf.
>
> The edits to remote_add_target_side_condition are to keep the two
> functions in sync.
>
> Ideally, the pack_hex_byte calls and the `*buf = '\0'` assignments
> should also have some bound checks, but that is outside the scope of
> this patch.
>
> Change-Id: Ib2e9849d89ebcc9e6275297138a3deb8bf03a7c3
> ---
>  gdb/remote.c | 36 ++++++++++++++++++++++--------------
>  1 file changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/gdb/remote.c b/gdb/remote.c
> index 3d38a9c7c8c9..fe898013a394 100644
> --- a/gdb/remote.c
> +++ b/gdb/remote.c
> @@ -11513,10 +11513,11 @@ Remote replied unexpectedly while setting startup-with-shell: %s"),
>  }
>  \f
>  
> -/* Given a location's target info BP_TGT and the packet buffer BUF,  output
> -   the list of conditions (in agent expression bytecode format), if any, the
> -   target needs to evaluate.  The output is placed into the packet buffer
> -   started from BUF and ended at BUF_END.  */
> +/* Given a location's target info BP_TGT and the packet buffer BUF,  output the
> +   list of conditions (in agent expression bytecode format), if any, the
> +   target needs to evaluate.
> +
> +   The output is appended to the existing content of BUF.  */
>  
>  static int
>  remote_add_target_side_condition (struct gdbarch *gdbarch,
> @@ -11532,35 +11533,42 @@ remote_add_target_side_condition (struct gdbarch *gdbarch,
>    /* Send conditions to the target.  */
>    for (agent_expr *aexpr : bp_tgt->conditions)
>      {
> -      xsnprintf (buf, buf_end - buf, "X%x,", (int) aexpr->buf.size ());
> -      buf += strlen (buf);
> +      buf += xsnprintf (buf, buf_end - buf, "X%x,", (int) aexpr->buf.size ());
> +
>        for (int i = 0; i < aexpr->buf.size (); ++i)
>  	buf = pack_hex_byte (buf, aexpr->buf[i]);
> +

I wonder if we should add an assert here either inside, or just after,
the loop, to check that we've not blown past BUF_END?  Unless I'm
misunderstanding this, these PACK_HEX_BYTE calls could overrun the
buffer, right?


>        *buf = '\0';
>      }
>    return 0;
>  }
>  
> +/* Given a location's target info BP_TGT and the packet buffer BUF, output the
> +   list of commands (in agent expression bytecode format), if any, the target
> +   needs to run when the breakpoint is hit.
> +
> +   The output is appended to the existing content of BUF.  */
> +
>  static void
>  remote_add_target_side_commands (struct gdbarch *gdbarch,
> -				 struct bp_target_info *bp_tgt, char *buf)
> +				 struct bp_target_info *bp_tgt, char *buf,
> +				 char *buf_end)
>  {
>    if (bp_tgt->tcommands.empty ())
>      return;
>  
>    buf += strlen (buf);
> -
> -  sprintf (buf, ";cmds:%x,", bp_tgt->persist);
> -  buf += strlen (buf);
> +  buf += xsnprintf (buf, buf_end - buf, ";cmds:%x,", bp_tgt->persist);
>  
>    /* Concatenate all the agent expressions that are commands into the
>       cmds parameter.  */
>    for (agent_expr *aexpr : bp_tgt->tcommands)
>      {
> -      sprintf (buf, "X%x,", (int) aexpr->buf.size ());
> -      buf += strlen (buf);
> +      buf += xsnprintf (buf, buf_end - buf, "X%x,", (int) aexpr->buf.size ());
> +
>        for (int i = 0; i < aexpr->buf.size (); ++i)
>  	buf = pack_hex_byte (buf, aexpr->buf[i]);
> +

As above for buffer overrun maybe?

Thanks,
Andrew

>        *buf = '\0';
>      }
>  }
> @@ -11604,7 +11612,7 @@ remote_target::insert_breakpoint (struct gdbarch *gdbarch,
>  	remote_add_target_side_condition (gdbarch, bp_tgt, p, endbuf);
>  
>        if (can_run_breakpoint_commands ())
> -	remote_add_target_side_commands (gdbarch, bp_tgt, p);
> +	remote_add_target_side_commands (gdbarch, bp_tgt, p, endbuf);
>  
>        putpkt (rs->buf);
>        getpkt (&rs->buf);
> @@ -11912,7 +11920,7 @@ remote_target::insert_hw_breakpoint (struct gdbarch *gdbarch,
>      remote_add_target_side_condition (gdbarch, bp_tgt, p, endbuf);
>  
>    if (can_run_breakpoint_commands ())
> -    remote_add_target_side_commands (gdbarch, bp_tgt, p);
> +    remote_add_target_side_commands (gdbarch, bp_tgt, p, endbuf);
>  
>    putpkt (rs->buf);
>    getpkt (&rs->buf);
> -- 
> 2.55.0