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

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

[Wu Zhou]

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

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

[Eli Zaretskii]

> Date: Wed, 25 May 2005 07:04:11 -0700 (PDT)
> From: Wu Zhou <woodzltc@cn.ibm.com>
> cc: mark.kettenis@xs4all.nl, eliz@gnu.org
> 
> 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. 

That might be so today, but if some day someone changes either the
size of buf[] or the format string, things could become messy.

I'd say, if we leave sprintf here, let's at least use sizeof(buf) in
the call to sprintf instead of a literal 8 in the format string.

As for the other 2 examples, I'd use safer functions there.  It's
unreasonable to request that Joe Random Hacker who happens to read the
code should perform the amount of analysis you demonstrated to
convince him/herself that the code is safe.  Most programmers won't go
to such lengths.

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

[Wu Zhou]

On Wed, 25 May 2005, Eli Zaretskii wrote:

> That might be so today, but if some day someone changes either the
> size of buf[] or the format string, things could become messy.
> 
> I'd say, if we leave sprintf here, let's at least use sizeof(buf) in
> the call to sprintf instead of a literal 8 in the format string.

Yes, you are right.  With this point in mind, I prefer to use xsnprint in 
this place too. 

> As for the other 2 examples, I'd use safer functions there.  It's
> unreasonable to request that Joe Random Hacker who happens to read the
> code should perform the amount of analysis you demonstrated to
> convince him/herself that the code is safe.  Most programmers won't go
> to such lengths.

Yes, I bet that too.  My intention in doing so is only to see whether 
there are any really overflow.  It is purely out of curiosity.  :-)

BTW, what is your point on my analysis in section 3.1.  I believe
it might incur an overflow.  But I need to design a scenario to verify 
that.  

Cheers
- Wu Zhou

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

[Eli Zaretskii]

> Date: Thu, 26 May 2005 02:37:08 -0700 (PDT)
> From: Wu Zhou <woodzltc@cn.ibm.com>
> cc: gdb@sources.redhat.com, gdb-patches@sources.redhat.com,
>         mark.kettenis@xs4all.nl
> 
> BTW, what is your point on my analysis in section 3.1.  I believe
> it might incur an overflow.

I think so too.

> But I need to design a scenario to verify that.

That's my point exactly: you don't need to go to such lengths.  If it
isn't 100% obvious that no overflow is possible, we should remove
sprintf in favor of safer functions.

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

[Wu Zhou]

On Thu, 26 May 2005, Eli Zaretskii wrote:

> > But I need to design a scenario to verify that.

It seems that most target don't implement "to_extra_thread_info" (ok, at 
least in i386 and ppc), gdbserver will return NULL to "qThreadExtraInfo".
So I can't verify this yet.  

> That's my point exactly: you don't need to go to such lengths.  If it
> isn't 100% obvious that no overflow is possible, we should remove
> sprintf in favor of safer functions.

Maybe you are right.  I will post a patch in a while. 

Cheers
- Wu Zhou