about the usage of sprintf in gdb, specifically in gdb/remote.c

[Wu Zhou <woodzltc at cn dot ibm dot com>]

Mark, Eli and others who might be interested in this topic,

To continue the discussion on the usage of sprintf in gdb, I am now
trying to get out some pattern of it in the source code, specifically 
in gdb/remote.c (It happens that I am reading its source recently). 
I summarize my findings below.  Would you please take a look and 
give comments. Any errors, please feel free to correct me.  

Thanks a lot.  

1. Won't introduce overflow. And one can easily see this from the 
context.  To name an example in function remote_thread_alive:
 
  int tid = PIDGET (ptid);
  char buf[16];

  if (tid < 0)
    sprintf (buf, "T-%08x", -tid);
  else
    sprintf (buf, "T%08x", tid); 

In this context, there is no possibility that buf get overflowed. 

IMHO, there is no need to switch to safer alternatives with this
usage pattern. 

2. Won't introduce overflow, but it is somewhat harder to see this from
the context.  To name an example in function remote_threads_extra_info:

  char *bufp = alloca (rs->remote_packet_size);
  ...
  if (use_threadextra_query)
    {
      sprintf (bufp, "qThreadExtraInfo,%x", PIDGET (tp->ptid));
  ...

To determine whether there is any possibility to overflow bufp in this
context, we need to determine: first the size of bufp, then the size of
PIDGET (tp->ptid).  To determine the sizeof of bufp, we need to determine
the range of rs->remote_packet_size.  Althought it turns out that this is
much larger than what could be put into it, and this won't introduce any
overflow. But I still consider this kind of usage to be hard-to-determine
one.  My idea on this is to define a macro:

  #define REMOTE_PACKET_SIZE (rs->remote_packet_size)

then use xsnprintf to replace the original sprintf call:

      xsnprintf (bufp, REMOTE_PACKTE_SIZE, "qThreadExtraInfo,%x", PIDGET (tp->ptid));

There are about dozen such usages of sprintf in gdb/remote.c.
 
3. Might introduce overflow. 

Investigating the source code of remote.c, I find there are two usages
of sprintf, which might introduce overflow:

3.1 sprintf on display_buf, which is also in remote_threads_extra_info 

display_buf is a char array with the size of 100:

  static char display_buf[100]; /* arbitrary...  */

But it is coded to hold three members of struct gdb_ext_thread_info
(shortname, display and more_display):

  if (remote_get_threadinfo (&id, set, &threadinfo))
    if (threadinfo.active)
      {
        if (*threadinfo.shortname)
          n += sprintf(&display_buf[0], " Name: %s,", threadinfo.shortname);
        if (*threadinfo.display)
          n += sprintf(&display_buf[n], " State: %s,", threadinfo.display);
        if (*threadinfo.more_display)
          n += sprintf(&display_buf[n], " Priority: %s",
                       threadinfo.more_display);

The size of these three members will be 544 if added together (Refer
to the following declaration):

struct gdb_ext_thread_info
  {
    threadref threadid;         /* External form of thread reference.  */
    int active;                 /* Has state interesting to GDB?
                                   regs, stack.  */
    char display[256];          /* Brief state display, name,
                                   blocked/suspended.  */
    char shortname[32];         /* To be used to name threads.  */
    char more_display[256];     /* Long info, statistics, queue depth,
                                   whatever.  */
  };

In my opinion, this is easy to incur an overflow.  But I don't have
chance to design a scenario to verify this.  What is your thought
on this?    

3.2  sprintf on msg, which is in function remote_check_symbols

Please have a look at the following code section:

  msg   = alloca (rs->remote_packet_size);
  reply = alloca (rs->remote_packet_size);

  /* Invite target to request symbol lookups.  */

  putpkt ("qSymbol::");
  getpkt (reply, (rs->remote_packet_size), 0);
  packet_ok (reply, &remote_protocol_qSymbol);

  while (strncmp (reply, "qSymbol:", 8) == 0)
    {
      tmp = &reply[8];
      end = hex2bin (tmp, msg, strlen (tmp) / 2);
      msg[end] = '\0';
      sym = lookup_minimal_symbol (msg, NULL, NULL);
      if (sym == NULL)
        sprintf (msg, "qSymbol::%s", &reply[8]);
      else
        sprintf (msg, "qSymbol:%s:%s",
                 paddr_nz (SYMBOL_VALUE_ADDRESS (sym)),
                 &reply[8]);

through alloca, msg and reply get a memory region with the size 
of rs->remote_packet_size.  If the size of reply goes beyond 
(rs->remote_packet_size - 1), the first sprintf call will be 
overflowed.  To overflow the second sprintf call, the size of 
reply could be smaller.  

I am not sure how long reply will get to (In my experiecne, 
some c++ symbols could get to very long).  I am also not sure
how long rs->remote_packet_size will get to.  So it depends on
the difference between these two values whether this could 
incur overflow.  

Maybe this won't incur any overflow at last.  But I think the
usage of sprintf here is highly suspectable.  Any different 
thought on this?  

Thanks for your attention on this long mail.   

Cheers
- Wu Zhou