Buffer Overflow Patch in GDB/TCL Interface code.

Buffer Overflow Patch in GDB/TCL Interface code.

[Fernando Nasser]

Message forwarded in behalf of Steven Johnson <sbjohnson@ozemail.com.au>
(Steven is not sure if this message made it to the list -- ISP problems.
 Please forgive us if you get it in duplicate)


There are a number of fixed size buffers defined in the C code that acts as an
interface between GDB and the TCL Insight. Unfortunately, on my system some of
those buffers were not big enough, leading to stack corruption and a resultant
segfault. I Specifically had a problem with clearing breakpoints, the problem 
was that the path to my source file was quite long, and caused a sprintf to
overflow it's pre-allocated buffer.

Instead of just adding some more to the buffer, i decided to fix as many of the
pre-allocated buffers as possible, so that this sort of problem can not occur
in future.

A Couple of fixes were tried. The one I settled on was wholescale replacement
of as many 'sprintf'/'vsprintf' calls with 'asprintf' and 'vasprintf'
respectively, as was possible without serious modification of the existing
code. There are now only 2 or 3 places in gdbtk-cmds where sprintf is used
(Mainly to do with source file loading). Everywhere else has been replaced with
asprintf/vasprintf.

Given that neither asprintf or vasprintf are in the MAN pages of linux, here is
a brief explenation.

asprintf (I call it "auto sprintf"): It automatically allocates a buffer of the
correct size to hold the sprintf'd string. The buffer when finished with must
be discarded with "free", otherwise you will get a heap leak.

vasprintf is the same as vsprintf, except it too automatically allocates its
buffer.

Eg:

Where code would have once been:

char buf[1024]; /* This is big enough for any reasonable use I reckon. */
sprintf(buf,"my %s %s %s %s %s undetermined length string",s1,s2,s3,s4,s5);

it is written as:

char *buf;
sprintf(&buf,"my %s %s %s %s %s undetermined length string",s1,s2,s3,s4,s5);

/* I have now finished with buf, so... */
free(buf);

The big difference is you no longer have to guess the size of your buffer, so
no more stack corruption can result.

It has been used elsewhere in GDB/Insight (remote.c and MI) to name a couple,
ergo, "it's portable". If it isn't really portable, then I would suggest adding
a local copy to utils.c in GDB for targets that do not support it. 

Anyway, here comes the changelog and patch:
12345678901234567890123456789012345678901234567890123456789012345678901234567890
2000-10-19  Steven Johnson  <sbjohnson@ozemail.com.au>

        * gdbtk.c,gdbtk-cmds.c,gdbtk-hooks.c,gdbtk-variable.c
                        : Replaced the vast majority of sprintf/vsprintf calls 
                          with asprintf and vasprintf respectively. Should
                          prevent any possible buffer overruns possible with
                          fixed size sprintf buffers. Specifically fixes a
                          problem with long filenames and clearing breakpoints
                          overflowing their buffers when using sprintf, causing
                          a segfault. Generically should also prevent any other
                          similar problems from occuring.

diff -C2 -r -b ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk-cmds.c
src/gdb/gdbtk/generic/gdbtk-cmds.c
*** ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk-cmds.c       Mon Aug 21 08:59:40
2000
--- src/gdb/gdbtk/generic/gdbtk-cmds.c  Thu Oct 19 11:25:10 2000
***************
*** 546,556 ****
  {
    va_list args;
!   char buf[1024];
  
    va_start (args, format);
  
!   vsprintf (buf, format, args);
  
    Tcl_ListObjAppendElement (NULL, objp, Tcl_NewStringObj (buf, -1));
  }
  
--- 546,557 ----
  {
    va_list args;
!   char *buf;
  
    va_start (args, format);
  
!   vasprintf (&buf, format, args);
  
    Tcl_ListObjAppendElement (NULL, objp, Tcl_NewStringObj (buf, -1));
+   free(buf);
  }
  
***************
*** 2009,2016 ****
       Tcl_Obj *CONST objv[];
  {
!   char buff[64];
  
!   sprintf (buff, "0x%llx", (long long) read_register (PC_REGNUM));
    Tcl_SetStringObj (result_ptr->obj_ptr, buff, -1);
    return TCL_OK;
  }
--- 2010,2018 ----
       Tcl_Obj *CONST objv[];
  {
!   char *buff;
  
!   asprintf (&buff, "0x%llx", (long long) read_register (PC_REGNUM));
    Tcl_SetStringObj (result_ptr->obj_ptr, buff, -1);
+   free(buff);
    return TCL_OK;
  }
***************
*** 2181,2187 ****
    if (tp == NULL)
      {
!       char buff[64];
!       sprintf (buff, "Tracepoint #%d does not exist", tpnum);
        Tcl_SetStringObj (result_ptr->obj_ptr, buff, -1);
        return TCL_ERROR;
      }
--- 2183,2190 ----
    if (tp == NULL)
      {
!       char *buff;
!       asprintf (&buff, "Tracepoint #%d does not exist", tpnum);
        Tcl_SetStringObj (result_ptr->obj_ptr, buff, -1);
+       free(buff);
        return TCL_ERROR;
      }
***************
*** 2598,2609 ****
  
    if (ret_val == TCL_OK) {
!     char buffer[256];
      Tcl_Obj *limits_obj[2];
  
!     sprintf (buffer, "0x%s", paddr_nz (low));
      limits_obj[0] = Tcl_NewStringObj (buffer, -1);
      
!     sprintf (buffer, "0x%s", paddr_nz (high));
      limits_obj[1] = Tcl_NewStringObj (buffer, -1);
  
      Tcl_DecrRefCount (result_ptr->obj_ptr);
--- 2601,2614 ----
  
    if (ret_val == TCL_OK) {
!     char *buffer;
      Tcl_Obj *limits_obj[2];
  
!     asprintf (&buffer, "0x%s", paddr_nz (low));
      limits_obj[0] = Tcl_NewStringObj (buffer, -1);
+     free(buffer);
       
!     asprintf (&buffer, "0x%s", paddr_nz (high));
      limits_obj[1] = Tcl_NewStringObj (buffer, -1);
+     free(buffer);
       
      Tcl_DecrRefCount (result_ptr->obj_ptr);
***************
*** 2621,2625 ****
    struct disassembly_client_data *client_data =
      (struct disassembly_client_data *) clientData;
!   char buffer[18];
    int index_len;
  
--- 2626,2630 ----
    struct disassembly_client_data *client_data =
      (struct disassembly_client_data *) clientData;
!   char *buffer;
    int index_len;
  
***************
*** 2696,2704 ****
                 will allow us avoid converting widget_line_no into a string.
*/
              
!             sprintf (buffer, "%d", client_data->widget_line_no);
              
              Tcl_SetVar2 (client_data->interp, client_data->map_arr,
                           Tcl_DStringValue (&client_data->src_to_line_prefix),
                           buffer, 0);
              
              Tcl_DStringSetLength (&client_data->src_to_line_prefix,
index_len);
--- 2701,2710 ----
                 will allow us avoid converting widget_line_no into a string.
*/
              
!             asprintf (&buffer, "%d", client_data->widget_line_no);
              
              Tcl_SetVar2 (client_data->interp, client_data->map_arr,
                           Tcl_DStringValue (&client_data->src_to_line_prefix),
                           buffer, 0);
+             free(buffer);
               
              Tcl_DStringSetLength (&client_data->src_to_line_prefix,
index_len);
***************
*** 2802,2806 ****
    if (*client_data->map_arr != '\0')
      {
!       char buffer[16];
        
        /* Run the command, then add an entry to the map array in
--- 2808,2812 ----
    if (*client_data->map_arr != '\0')
      {
!       char *buffer;
        
        /* Run the command, then add an entry to the map array in
***************
*** 2812,2816 ****
         will allow us avoid converting widget_line_no into a string. */
        
!       sprintf (buffer, "%d", client_data->widget_line_no);
        
        Tcl_SetVar2 (client_data->interp, client_data->map_arr,
--- 2818,2822 ----
         will allow us avoid converting widget_line_no into a string. */
        
!       asprintf (&buffer, "%d", client_data->widget_line_no);
        
        Tcl_SetVar2 (client_data->interp, client_data->map_arr,
***************
*** 2829,2832 ****
--- 2835,2839 ----
        Tcl_DStringSetLength (&client_data->line_to_pc_prefix,
line_to_pc_len);      
        
+       free(buffer);
      }
    
***************
*** 3685,3689 ****
    int line, ret, thread = -1;
    struct breakpoint *b;
!   char buf[64], *typestr;
    Tcl_DString cmd;
    enum bpdisp disp;
--- 3692,3696 ----
    int line, ret, thread = -1;
    struct breakpoint *b;
!   char *buf, *typestr;
    Tcl_DString cmd;
    enum bpdisp disp;
***************
*** 3745,3751 ****
  
    /* FIXME: this won't work for duplicate basenames! */
!   sprintf (buf, "%s:%d", basename (Tcl_GetStringFromObj (objv[1], NULL)),
           line);
    b->addr_string = strsave (buf);
  
    /* now send notification command back to GUI */
--- 3752,3759 ----
  
    /* FIXME: this won't work for duplicate basenames! */
!   asprintf (&buf, "%s:%d", basename (Tcl_GetStringFromObj (objv[1], NULL)),
           line);
    b->addr_string = strsave (buf);
+   free(buf);
     
    /* now send notification command back to GUI */
***************
*** 3754,3769 ****
  
    Tcl_DStringAppend (&cmd, "gdbtk_tcl_breakpoint create ", -1);
!   sprintf (buf, "%d", b->number);
    Tcl_DStringAppendElement (&cmd, buf);
!   sprintf (buf, "0x%lx", (long) sal.pc);
    Tcl_DStringAppendElement (&cmd, buf);
    Tcl_DStringAppendElement (&cmd, Tcl_GetStringFromObj (objv[2], NULL));
    Tcl_DStringAppendElement (&cmd, Tcl_GetStringFromObj (objv[1], NULL));
    Tcl_DStringAppendElement (&cmd, bpdisp[b->disposition]);
!   sprintf (buf, "%d", b->enable);
    Tcl_DStringAppendElement (&cmd, buf);
!   sprintf (buf, "%d", b->thread);
    Tcl_DStringAppendElement (&cmd, buf);
! 
  
    ret = Tcl_Eval (interp, Tcl_DStringValue (&cmd));
--- 3762,3780 ----
  
    Tcl_DStringAppend (&cmd, "gdbtk_tcl_breakpoint create ", -1);
!   asprintf (&buf, "%d", b->number);
    Tcl_DStringAppendElement (&cmd, buf);
!   free(buf);
!   asprintf (&buf, "0x%lx", (long) sal.pc);
    Tcl_DStringAppendElement (&cmd, buf);
    Tcl_DStringAppendElement (&cmd, Tcl_GetStringFromObj (objv[2], NULL));
    Tcl_DStringAppendElement (&cmd, Tcl_GetStringFromObj (objv[1], NULL));
    Tcl_DStringAppendElement (&cmd, bpdisp[b->disposition]);
!   free(buf);
!   asprintf (&buf, "%d", b->enable);
    Tcl_DStringAppendElement (&cmd, buf);
!   free(buf);
!   asprintf (&buf, "%d", b->thread);
    Tcl_DStringAppendElement (&cmd, buf);
!   free(buf);
  
    ret = Tcl_Eval (interp, Tcl_DStringValue (&cmd));
***************
*** 3797,3801 ****
    long addr;
    struct breakpoint *b;
!   char *filename, *typestr, buf[64];
    Tcl_DString cmd;
    enum bpdisp disp;
--- 3808,3812 ----
    long addr;
    struct breakpoint *b;
!   char *filename, *typestr, *buf;
    Tcl_DString cmd;
    enum bpdisp disp;
***************
*** 3849,3853 ****
    b->thread = thread;
  
!   sprintf (buf, "*(0x%lx)", addr);
    b->addr_string = strsave (buf);
  
--- 3860,3864 ----
    b->thread = thread;
  
!   asprintf (&buf, "*(0x%lx)", addr);
    b->addr_string = strsave (buf);
  
***************
*** 3857,3865 ****
  
    Tcl_DStringAppend (&cmd, "gdbtk_tcl_breakpoint create ", -1);
!   sprintf (buf, "%d", b->number);
    Tcl_DStringAppendElement (&cmd, buf);
!   sprintf (buf, "0x%lx", addr);
    Tcl_DStringAppendElement (&cmd, buf);
!   sprintf (buf, "%d", b->line_number);
    Tcl_DStringAppendElement (&cmd, buf);
  
--- 3868,3879 ----
  
    Tcl_DStringAppend (&cmd, "gdbtk_tcl_breakpoint create ", -1);
!   free(buf);
!   asprintf (&buf, "%d", b->number);
    Tcl_DStringAppendElement (&cmd, buf);
!   free(buf);
!   asprintf (&buf, "0x%lx", addr);
    Tcl_DStringAppendElement (&cmd, buf);
!   free(buf);
!   asprintf (&buf, "%d", b->line_number);
    Tcl_DStringAppendElement (&cmd, buf);
  
***************
*** 3869,3879 ****
    Tcl_DStringAppendElement (&cmd, filename);
    Tcl_DStringAppendElement (&cmd, bpdisp[b->disposition]);
!   sprintf (buf, "%d", b->enable);
    Tcl_DStringAppendElement (&cmd, buf);
!   sprintf (buf, "%d", b->thread);
    Tcl_DStringAppendElement (&cmd, buf);
  
    ret = Tcl_Eval (interp, Tcl_DStringValue (&cmd));
    Tcl_DStringFree (&cmd);
    return ret;
  }
--- 3883,3896 ----
    Tcl_DStringAppendElement (&cmd, filename);
    Tcl_DStringAppendElement (&cmd, bpdisp[b->disposition]);
!   free(buf);
!   asprintf (&buf, "%d", b->enable);
    Tcl_DStringAppendElement (&cmd, buf);
!   free(buf);
!   asprintf (&buf, "%d", b->thread);
    Tcl_DStringAppendElement (&cmd, buf);
  
    ret = Tcl_Eval (interp, Tcl_DStringValue (&cmd));
    Tcl_DStringFree (&cmd);
+   free(buf);
    return ret;
  }
***************
*** 4014,4020 ****
    if (!b || b->type != bp_breakpoint)
      {
!       char err_buf[64];
!       sprintf (err_buf, "Breakpoint #%d does not exist.", bpnum);
        Tcl_SetStringObj (result_ptr->obj_ptr, err_buf, -1);
        return TCL_ERROR;
      }
--- 4031,4038 ----
    if (!b || b->type != bp_breakpoint)
      {
!       char *err_buf;
!       asprintf (&err_buf, "Breakpoint #%d does not exist.", bpnum);
        Tcl_SetStringObj (result_ptr->obj_ptr, err_buf, -1);
+       free(err_buf);
        return TCL_ERROR;
      }
***************
*** 4309,4321 ****
       Tcl_Obj *CONST objv[];
  {
!   char frame[32];
  
    if (selected_frame == NULL)
!     strcpy (frame, "");
    else
!     sprintf (frame, "0x%s", paddr_nz (FRAME_FP (selected_frame)));
  
    Tcl_SetStringObj (result_ptr->obj_ptr, frame, -1);
  
    return TCL_OK;
  }
--- 4327,4340 ----
       Tcl_Obj *CONST objv[];
  {
!   char *frame;
  
    if (selected_frame == NULL)
!     asprintf (&frame, "%s","");
    else
!     asprintf (&frame, "0x%s", paddr_nz (FRAME_FP (selected_frame)));
  
    Tcl_SetStringObj (result_ptr->obj_ptr, frame, -1);
  
+   free(frame);
    return TCL_OK;
  }
***************
*** 4339,4349 ****
       Tcl_Obj *CONST objv[];
  {
!   char start[32];
!   char end[32];
  
    if (selected_frame == NULL)
      {
!       strcpy (start, "");
!       strcpy (end, "");
      }
    else
--- 4358,4368 ----
       Tcl_Obj *CONST objv[];
  {
!   char *start = NULL;
!   char *end   = NULL;
  
    if (selected_frame == NULL)
      {
!       asprintf (&start, "%s", "");
!       asprintf (&end, "%s", "");
      }
    else
***************
*** 4351,4356 ****
        struct block *block;
        block = get_frame_block (selected_frame);
!       sprintf (start, "0x%s", paddr_nz (BLOCK_START (block)));
!       sprintf (end, "0x%s", paddr_nz (BLOCK_END (block)));
      }
  
--- 4370,4375 ----
        struct block *block;
        block = get_frame_block (selected_frame);
!       asprintf (&start, "0x%s", paddr_nz (BLOCK_START (block)));
!       asprintf (&end, "0x%s", paddr_nz (BLOCK_END (block)));
      }
  
***************
*** 4361,4364 ****
--- 4380,4385 ----
                            Tcl_NewStringObj (end, -1));
  
+   free(start);
+   free(end);
    return TCL_OK;
  }
***************
*** 4436,4449 ****
          if (!junk && pc < BLOCK_END (block))
            {
!             char addr[32];
  
              Tcl_Obj *elt = Tcl_NewListObj (0, NULL);
!             sprintf (addr, "0x%s", paddr_nz (BLOCK_START (block)));
              Tcl_ListObjAppendElement (interp, elt,
                                        Tcl_NewStringObj (addr, -1));
!             sprintf (addr, "0x%s", paddr_nz (BLOCK_END (block)));
              Tcl_ListObjAppendElement (interp, elt,
                                        Tcl_NewStringObj (addr, -1));
              Tcl_ListObjAppendElement (interp, result_ptr->obj_ptr, elt);
            }
  
--- 4457,4472 ----
          if (!junk && pc < BLOCK_END (block))
            {
!             char *addr;
  
              Tcl_Obj *elt = Tcl_NewListObj (0, NULL);
!             asprintf (&addr, "0x%s", paddr_nz (BLOCK_START (block)));
              Tcl_ListObjAppendElement (interp, elt,
                                        Tcl_NewStringObj (addr, -1));
!             free(addr);
!             asprintf (&addr, "0x%s", paddr_nz (BLOCK_END (block)));
              Tcl_ListObjAppendElement (interp, elt,
                                        Tcl_NewStringObj (addr, -1));
              Tcl_ListObjAppendElement (interp, result_ptr->obj_ptr, elt);
+             free(addr);
            }
  
diff -C2 -r -b ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk-hooks.c
src/gdb/gdbtk/generic/gdbtk-hooks.c
*** ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk-hooks.c      Thu Jul 13 09:10:03
2000
--- src/gdb/gdbtk/generic/gdbtk-hooks.c Thu Oct 19 10:54:06 2000
***************
*** 293,300 ****
       va_list args;
  {
!   char buf[200];
  
!   vsprintf (buf, warning, args);
    gdbtk_two_elem_cmd ("gdbtk_tcl_warning", buf);
  }
  
--- 293,302 ----
       va_list args;
  {
!   char *buf;
  
!   vasprintf (&buf, warning, args);
    gdbtk_two_elem_cmd ("gdbtk_tcl_warning", buf);
+ 
+   free(buf);
  }
  
***************
*** 326,333 ****
       const char *warning;
  {
!   char buf[512];
!   sprintf (buf, "gdbtk_tcl_ignorable_warning {%s} {%s}", class, warning);
    if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
      report_error ();
  }
  
--- 328,336 ----
       const char *warning;
  {
!   char *buf;
!   asprintf (&buf, "gdbtk_tcl_ignorable_warning {%s} {%s}", class, warning);
    if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
      report_error ();
+   free(buf); 
  }
  
***************
*** 460,468 ****
  {
    va_list args;
!   char buf[200];
  
    va_start (args, format);
!   vsprintf (buf, format, args);
    gdbtk_two_elem_cmd ("gdbtk_tcl_readline_begin", buf);
  }
  
--- 466,475 ----
  {
    va_list args;
!   char *buf;
  
    va_start (args, format);
!   vasprintf (&buf, format, args);
    gdbtk_two_elem_cmd ("gdbtk_tcl_readline_begin", buf);
+   free(buf);
  }
  
***************
*** 529,533 ****
    Tcl_DString cmd;
    char *p;
!   char buffer[30];
  
    Tcl_DStringInit (&cmd);
--- 536,540 ----
    Tcl_DString cmd;
    char *p;
!   char *buffer = NULL;
  
    Tcl_DStringInit (&cmd);
***************
*** 541,545 ****
      {
        char *q = strchr (p, ' ');
!       char save;
        if (q)
        {
--- 548,552 ----
      {
        char *q = strchr (p, ' ');
!       char save = '\0';
        if (q)
        {
***************
*** 572,581 ****
      case var_uinteger:
      case var_zinteger:
!       sprintf (buffer, "%u", *(unsigned int *) cmdblk->var);
        Tcl_DStringAppendElement (&cmd, buffer);
        break;
  
      case var_integer:
!       sprintf (buffer, "%d", *(int *) cmdblk->var);
        Tcl_DStringAppendElement (&cmd, buffer);
        break;
--- 579,588 ----
      case var_uinteger:
      case var_zinteger:
!       asprintf (&buffer, "%u", *(unsigned int *) cmdblk->var);
        Tcl_DStringAppendElement (&cmd, buffer);
        break;
  
      case var_integer:
!       asprintf (&buffer, "%d", *(int *) cmdblk->var);
        Tcl_DStringAppendElement (&cmd, buffer);
        break;
***************
*** 591,594 ****
--- 598,606 ----
  
    Tcl_DStringFree (&cmd);
+    
+   if (buffer != NULL)
+     {
+        free(buffer);
+     }
  }
  
***************
*** 631,635 ****
       const char *action;
  {
!   char buf[256];
    int v;
    struct symtab_and_line sal;
--- 643,647 ----
       const char *action;
  {
!   char *buf;
    int v;
    struct symtab_and_line sal;
***************
*** 646,650 ****
      filename = "";
  
!   sprintf (buf, "gdbtk_tcl_breakpoint %s %d 0x%lx %d {%s} {%s} %d %d",
           action, b->number, (long) b->address, b->line_number, filename,
           bpdisp[b->disposition], b->enable, b->thread);
--- 658,662 ----
      filename = "";
  
!   asprintf (&buf, "gdbtk_tcl_breakpoint %s %d 0x%lx %d {%s} {%s} %d %d",
           action, b->number, (long) b->address, b->line_number, filename,
           bpdisp[b->disposition], b->enable, b->thread);
***************
*** 652,655 ****
--- 664,668 ----
    if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
      report_error ();
+   free(buf); 
  }
  
***************
*** 657,664 ****
  gdbtk_load_hash (const char *section, unsigned long num)
  {
!   char buf[128];
!   sprintf (buf, "Download::download_hash %s %ld", section, num);
    if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
      report_error ();
    return atoi (gdbtk_interp->result);
  }
--- 670,679 ----
  gdbtk_load_hash (const char *section, unsigned long num)
  {
!   char *buf;
!   asprintf (&buf, "Download::download_hash %s %ld", section, num);
    if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
      report_error ();
+   free(buf); 
+    
    return atoi (gdbtk_interp->result);
  }
***************
*** 712,720 ****
       va_list args;
  {
!   char buf[200];
    long val;
  
!   vsprintf (buf, query, args);
    gdbtk_two_elem_cmd ("gdbtk_tcl_query", buf);
  
    val = atol (gdbtk_interp->result);
--- 727,736 ----
       va_list args;
  {
!   char *buf;
    long val;
  
!   vasprintf (&buf, query, args);
    gdbtk_two_elem_cmd ("gdbtk_tcl_query", buf);
+   free(buf);
     
    val = atol (gdbtk_interp->result);
***************
*** 760,764 ****
       const char *action;
  {
!   char buf[256];
    int v;
    struct symtab_and_line sal;
--- 776,780 ----
       const char *action;
  {
!   char *buf;
    int v;
    struct symtab_and_line sal;
***************
*** 772,780 ****
    if (filename == NULL)
      filename = "N/A";
!   sprintf (buf, "gdbtk_tcl_tracepoint %s %d 0x%lx %d {%s} %d", action,
tp->number,
           (long) tp->address, sal.line, filename, tp->pass_count);
  
    if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
      report_error ();
  }
  
--- 788,797 ----
    if (filename == NULL)
      filename = "N/A";
!   asprintf (&buf, "gdbtk_tcl_tracepoint %s %d 0x%lx %d {%s} %d", action,
tp->number,
           (long) tp->address, sal.line, filename, tp->pass_count);
  
    if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
      report_error ();
+   free(buf); 
  }
  
***************
*** 877,881 ****
  gdbtk_annotate_signal ()
  {
!   char buf[128];
  
    /* Inform gui that the target has stopped. This is
--- 894,898 ----
  gdbtk_annotate_signal ()
  {
!   char *buf;
  
    /* Inform gui that the target has stopped. This is
***************
*** 885,892 ****
    Tcl_Eval (gdbtk_interp, "gdbtk_stop_idle_callback");
  
!   sprintf (buf, "gdbtk_signal %s {%s}", target_signal_to_name (stop_signal),
           target_signal_to_string (stop_signal));
    if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
      report_error ();
  }
  
--- 902,910 ----
    Tcl_Eval (gdbtk_interp, "gdbtk_stop_idle_callback");
  
!   asprintf (&buf, "gdbtk_signal %s {%s}", target_signal_to_name
(stop_signal),
           target_signal_to_string (stop_signal));
    if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
      report_error ();
+   free(buf);  
  }
  
diff -C2 -r -b ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk-variable.c
src/gdb/gdbtk/generic/gdbtk-variable.c
*** ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk-variable.c   Thu Jul 13 09:10:03
2000
--- src/gdb/gdbtk/generic/gdbtk-variable.c      Thu Oct 19 11:18:26 2000
***************
*** 566,570 ****
    gdb_variable *var;
    char *name;
!   char obj_name[31];
    int index;
    static int id = 0;
--- 566,570 ----
    gdb_variable *var;
    char *name;
!   char *obj_name;
    int index;
    static int id = 0;
***************
*** 587,596 ****
        /* generate a name for this object */
        id++;
!       sprintf (obj_name, "var%d", id);
      }
    else
      {
        /* specified name for object */
!       strncpy (obj_name, name, 30);
        objv++;
        objc--;
--- 587,596 ----
        /* generate a name for this object */
        id++;
!       asprintf (&obj_name, "var%d", id);
      }
    else
      {
        /* specified name for object */
!       asprintf (&obj_name, "%s", name);
        objv++;
        objc--;
***************
*** 648,651 ****
--- 648,653 ----
      }
  
+   free(obj_name);
+    
    return TCL_ERROR;
  }
***************
*** 2018,2025 ****
      case TYPE_CODE_ARRAY:
        {
!         char number[16];
          *obj = Tcl_NewStringObj (NULL, 0);
!         sprintf (number, "%d", var->num_children);
          Tcl_AppendStringsToObj (*obj, "[", number, "]", NULL);
        }
        break;
--- 2020,2028 ----
      case TYPE_CODE_ARRAY:
        {
!         char *number;
          *obj = Tcl_NewStringObj (NULL, 0);
!         asprintf (&number, "%d", var->num_children); 
          Tcl_AppendStringsToObj (*obj, "[", number, "]", NULL);
+       free(number); 
        }
        break;
diff -C2 -r -b ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk.c
src/gdb/gdbtk/generic/gdbtk.c
*** ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk.c    Thu Jul  6 12:04:47 2000
--- src/gdb/gdbtk/generic/gdbtk.c       Thu Oct 19 10:59:48 2000
***************
*** 197,201 ****
  {
    va_list args;
!   char buf[10000], *v[3], *merge, *priority;
  
    switch (level)
--- 197,201 ----
  {
    va_list args;
!   char *buf, *v[3], *merge, *priority;
  
    switch (level)
***************
*** 216,230 ****
    va_start (args, fmt);
  
    v[0] = "dbug";
    v[1] = priority;
    v[2] = buf;
  
-   vsprintf (buf, fmt, args);
-   va_end (args);
- 
    merge = Tcl_Merge (3, v);
    if (Tcl_Eval (gdbtk_interp, merge) != TCL_OK)
      Tcl_BackgroundError (gdbtk_interp);
    Tcl_Free (merge);
  }
  
--- 216,232 ----
    va_start (args, fmt);
  
+ 
+   vasprintf (&buf, fmt, args);
+   va_end (args);
+ 
    v[0] = "dbug";
    v[1] = priority;
    v[2] = buf;
     
    merge = Tcl_Merge (3, v);
    if (Tcl_Eval (gdbtk_interp, merge) != TCL_OK)
      Tcl_BackgroundError (gdbtk_interp);
    Tcl_Free (merge);
+   free(buf);
  }
  
***************
*** 359,363 ****
    struct cleanup *old_chain;
    int found_main;
!   char s[5];
    Tcl_Obj *auto_path_elem, *auto_path_name;
  
--- 361,365 ----
    struct cleanup *old_chain;
    int found_main;
!   char *s;
    Tcl_Obj *auto_path_elem, *auto_path_name;
  
***************
*** 389,394 ****
    /* Set up some globals used by gdb to pass info to gdbtk
       for start up options and the like */
!   sprintf (s, "%d", inhibit_gdbinit);
    Tcl_SetVar2 (gdbtk_interp, "GDBStartup", "inhibit_prefs", s,
TCL_GLOBAL_ONLY);
    /* Note: Tcl_SetVar2() treats the value as read-only (making a
       copy).  Unfortunatly it does not mark the parameter as
--- 391,398 ----
    /* Set up some globals used by gdb to pass info to gdbtk
       for start up options and the like */
!   asprintf (&s, "%d", inhibit_gdbinit);
    Tcl_SetVar2 (gdbtk_interp, "GDBStartup", "inhibit_prefs", s,
TCL_GLOBAL_ONLY);
+   free(s);
+    
    /* Note: Tcl_SetVar2() treats the value as read-only (making a
       copy).  Unfortunatly it does not mark the parameter as

Re: Buffer Overflow Patch in GDB/TCL Interface code.

[Syd Polk]

My only concern is whether this call, asprintf, is available on all platforms.

Fernando Nasser wrote:
> 
> Message forwarded in behalf of Steven Johnson <sbjohnson@ozemail.com.au>
> (Steven is not sure if this message made it to the list -- ISP problems.
>  Please forgive us if you get it in duplicate)
> 
> There are a number of fixed size buffers defined in the C code that acts as an
> interface between GDB and the TCL Insight. Unfortunately, on my system some of
> those buffers were not big enough, leading to stack corruption and a resultant
> segfault. I Specifically had a problem with clearing breakpoints, the problem
> was that the path to my source file was quite long, and caused a sprintf to
> overflow it's pre-allocated buffer.
> 
> Instead of just adding some more to the buffer, i decided to fix as many of the
> pre-allocated buffers as possible, so that this sort of problem can not occur
> in future.
> 
> A Couple of fixes were tried. The one I settled on was wholescale replacement
> of as many 'sprintf'/'vsprintf' calls with 'asprintf' and 'vasprintf'
> respectively, as was possible without serious modification of the existing
> code. There are now only 2 or 3 places in gdbtk-cmds where sprintf is used
> (Mainly to do with source file loading). Everywhere else has been replaced with
> asprintf/vasprintf.
> 
> Given that neither asprintf or vasprintf are in the MAN pages of linux, here is
> a brief explenation.
> 
> asprintf (I call it "auto sprintf"): It automatically allocates a buffer of the
> correct size to hold the sprintf'd string. The buffer when finished with must
> be discarded with "free", otherwise you will get a heap leak.
> 
> vasprintf is the same as vsprintf, except it too automatically allocates its
> buffer.
> 
> Eg:
> 
> Where code would have once been:
> 
> char buf[1024]; /* This is big enough for any reasonable use I reckon. */
> sprintf(buf,"my %s %s %s %s %s undetermined length string",s1,s2,s3,s4,s5);
> 
> it is written as:
> 
> char *buf;
> sprintf(&buf,"my %s %s %s %s %s undetermined length string",s1,s2,s3,s4,s5);
> 
> /* I have now finished with buf, so... */
> free(buf);
> 
> The big difference is you no longer have to guess the size of your buffer, so
> no more stack corruption can result.
> 
> It has been used elsewhere in GDB/Insight (remote.c and MI) to name a couple,
> ergo, "it's portable". If it isn't really portable, then I would suggest adding
> a local copy to utils.c in GDB for targets that do not support it.
> 
> Anyway, here comes the changelog and patch:
> 12345678901234567890123456789012345678901234567890123456789012345678901234567890
> 2000-10-19  Steven Johnson  <sbjohnson@ozemail.com.au>
> 
>         * gdbtk.c,gdbtk-cmds.c,gdbtk-hooks.c,gdbtk-variable.c
>                         : Replaced the vast majority of sprintf/vsprintf calls
>                           with asprintf and vasprintf respectively. Should
>                           prevent any possible buffer overruns possible with
>                           fixed size sprintf buffers. Specifically fixes a
>                           problem with long filenames and clearing breakpoints
>                           overflowing their buffers when using sprintf, causing
>                           a segfault. Generically should also prevent any other
>                           similar problems from occuring.
> 
> diff -C2 -r -b ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk-cmds.c
> src/gdb/gdbtk/generic/gdbtk-cmds.c
> *** ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk-cmds.c       Mon Aug 21 08:59:40
> 2000
> --- src/gdb/gdbtk/generic/gdbtk-cmds.c  Thu Oct 19 11:25:10 2000
> ***************
> *** 546,556 ****
>   {
>     va_list args;
> !   char buf[1024];
> 
>     va_start (args, format);
> 
> !   vsprintf (buf, format, args);
> 
>     Tcl_ListObjAppendElement (NULL, objp, Tcl_NewStringObj (buf, -1));
>   }
> 
> --- 546,557 ----
>   {
>     va_list args;
> !   char *buf;
> 
>     va_start (args, format);
> 
> !   vasprintf (&buf, format, args);
> 
>     Tcl_ListObjAppendElement (NULL, objp, Tcl_NewStringObj (buf, -1));
> +   free(buf);
>   }
> 
> ***************
> *** 2009,2016 ****
>        Tcl_Obj *CONST objv[];
>   {
> !   char buff[64];
> 
> !   sprintf (buff, "0x%llx", (long long) read_register (PC_REGNUM));
>     Tcl_SetStringObj (result_ptr->obj_ptr, buff, -1);
>     return TCL_OK;
>   }
> --- 2010,2018 ----
>        Tcl_Obj *CONST objv[];
>   {
> !   char *buff;
> 
> !   asprintf (&buff, "0x%llx", (long long) read_register (PC_REGNUM));
>     Tcl_SetStringObj (result_ptr->obj_ptr, buff, -1);
> +   free(buff);
>     return TCL_OK;
>   }
> ***************
> *** 2181,2187 ****
>     if (tp == NULL)
>       {
> !       char buff[64];
> !       sprintf (buff, "Tracepoint #%d does not exist", tpnum);
>         Tcl_SetStringObj (result_ptr->obj_ptr, buff, -1);
>         return TCL_ERROR;
>       }
> --- 2183,2190 ----
>     if (tp == NULL)
>       {
> !       char *buff;
> !       asprintf (&buff, "Tracepoint #%d does not exist", tpnum);
>         Tcl_SetStringObj (result_ptr->obj_ptr, buff, -1);
> +       free(buff);
>         return TCL_ERROR;
>       }
> ***************
> *** 2598,2609 ****
> 
>     if (ret_val == TCL_OK) {
> !     char buffer[256];
>       Tcl_Obj *limits_obj[2];
> 
> !     sprintf (buffer, "0x%s", paddr_nz (low));
>       limits_obj[0] = Tcl_NewStringObj (buffer, -1);
> 
> !     sprintf (buffer, "0x%s", paddr_nz (high));
>       limits_obj[1] = Tcl_NewStringObj (buffer, -1);
> 
>       Tcl_DecrRefCount (result_ptr->obj_ptr);
> --- 2601,2614 ----
> 
>     if (ret_val == TCL_OK) {
> !     char *buffer;
>       Tcl_Obj *limits_obj[2];
> 
> !     asprintf (&buffer, "0x%s", paddr_nz (low));
>       limits_obj[0] = Tcl_NewStringObj (buffer, -1);
> +     free(buffer);
> 
> !     asprintf (&buffer, "0x%s", paddr_nz (high));
>       limits_obj[1] = Tcl_NewStringObj (buffer, -1);
> +     free(buffer);
> 
>       Tcl_DecrRefCount (result_ptr->obj_ptr);
> ***************
> *** 2621,2625 ****
>     struct disassembly_client_data *client_data =
>       (struct disassembly_client_data *) clientData;
> !   char buffer[18];
>     int index_len;
> 
> --- 2626,2630 ----
>     struct disassembly_client_data *client_data =
>       (struct disassembly_client_data *) clientData;
> !   char *buffer;
>     int index_len;
> 
> ***************
> *** 2696,2704 ****
>                  will allow us avoid converting widget_line_no into a string.
> */
> 
> !             sprintf (buffer, "%d", client_data->widget_line_no);
> 
>               Tcl_SetVar2 (client_data->interp, client_data->map_arr,
>                            Tcl_DStringValue (&client_data->src_to_line_prefix),
>                            buffer, 0);
> 
>               Tcl_DStringSetLength (&client_data->src_to_line_prefix,
> index_len);
> --- 2701,2710 ----
>                  will allow us avoid converting widget_line_no into a string.
> */
> 
> !             asprintf (&buffer, "%d", client_data->widget_line_no);
> 
>               Tcl_SetVar2 (client_data->interp, client_data->map_arr,
>                            Tcl_DStringValue (&client_data->src_to_line_prefix),
>                            buffer, 0);
> +             free(buffer);
> 
>               Tcl_DStringSetLength (&client_data->src_to_line_prefix,
> index_len);
> ***************
> *** 2802,2806 ****
>     if (*client_data->map_arr != '\0')
>       {
> !       char buffer[16];
> 
>         /* Run the command, then add an entry to the map array in
> --- 2808,2812 ----
>     if (*client_data->map_arr != '\0')
>       {
> !       char *buffer;
> 
>         /* Run the command, then add an entry to the map array in
> ***************
> *** 2812,2816 ****
>          will allow us avoid converting widget_line_no into a string. */
> 
> !       sprintf (buffer, "%d", client_data->widget_line_no);
> 
>         Tcl_SetVar2 (client_data->interp, client_data->map_arr,
> --- 2818,2822 ----
>          will allow us avoid converting widget_line_no into a string. */
> 
> !       asprintf (&buffer, "%d", client_data->widget_line_no);
> 
>         Tcl_SetVar2 (client_data->interp, client_data->map_arr,
> ***************
> *** 2829,2832 ****
> --- 2835,2839 ----
>         Tcl_DStringSetLength (&client_data->line_to_pc_prefix,
> line_to_pc_len);
> 
> +       free(buffer);
>       }
> 
> ***************
> *** 3685,3689 ****
>     int line, ret, thread = -1;
>     struct breakpoint *b;
> !   char buf[64], *typestr;
>     Tcl_DString cmd;
>     enum bpdisp disp;
> --- 3692,3696 ----
>     int line, ret, thread = -1;
>     struct breakpoint *b;
> !   char *buf, *typestr;
>     Tcl_DString cmd;
>     enum bpdisp disp;
> ***************
> *** 3745,3751 ****
> 
>     /* FIXME: this won't work for duplicate basenames! */
> !   sprintf (buf, "%s:%d", basename (Tcl_GetStringFromObj (objv[1], NULL)),
>            line);
>     b->addr_string = strsave (buf);
> 
>     /* now send notification command back to GUI */
> --- 3752,3759 ----
> 
>     /* FIXME: this won't work for duplicate basenames! */
> !   asprintf (&buf, "%s:%d", basename (Tcl_GetStringFromObj (objv[1], NULL)),
>            line);
>     b->addr_string = strsave (buf);
> +   free(buf);
> 
>     /* now send notification command back to GUI */
> ***************
> *** 3754,3769 ****
> 
>     Tcl_DStringAppend (&cmd, "gdbtk_tcl_breakpoint create ", -1);
> !   sprintf (buf, "%d", b->number);
>     Tcl_DStringAppendElement (&cmd, buf);
> !   sprintf (buf, "0x%lx", (long) sal.pc);
>     Tcl_DStringAppendElement (&cmd, buf);
>     Tcl_DStringAppendElement (&cmd, Tcl_GetStringFromObj (objv[2], NULL));
>     Tcl_DStringAppendElement (&cmd, Tcl_GetStringFromObj (objv[1], NULL));
>     Tcl_DStringAppendElement (&cmd, bpdisp[b->disposition]);
> !   sprintf (buf, "%d", b->enable);
>     Tcl_DStringAppendElement (&cmd, buf);
> !   sprintf (buf, "%d", b->thread);
>     Tcl_DStringAppendElement (&cmd, buf);
> !
> 
>     ret = Tcl_Eval (interp, Tcl_DStringValue (&cmd));
> --- 3762,3780 ----
> 
>     Tcl_DStringAppend (&cmd, "gdbtk_tcl_breakpoint create ", -1);
> !   asprintf (&buf, "%d", b->number);
>     Tcl_DStringAppendElement (&cmd, buf);
> !   free(buf);
> !   asprintf (&buf, "0x%lx", (long) sal.pc);
>     Tcl_DStringAppendElement (&cmd, buf);
>     Tcl_DStringAppendElement (&cmd, Tcl_GetStringFromObj (objv[2], NULL));
>     Tcl_DStringAppendElement (&cmd, Tcl_GetStringFromObj (objv[1], NULL));
>     Tcl_DStringAppendElement (&cmd, bpdisp[b->disposition]);
> !   free(buf);
> !   asprintf (&buf, "%d", b->enable);
>     Tcl_DStringAppendElement (&cmd, buf);
> !   free(buf);
> !   asprintf (&buf, "%d", b->thread);
>     Tcl_DStringAppendElement (&cmd, buf);
> !   free(buf);
> 
>     ret = Tcl_Eval (interp, Tcl_DStringValue (&cmd));
> ***************
> *** 3797,3801 ****
>     long addr;
>     struct breakpoint *b;
> !   char *filename, *typestr, buf[64];
>     Tcl_DString cmd;
>     enum bpdisp disp;
> --- 3808,3812 ----
>     long addr;
>     struct breakpoint *b;
> !   char *filename, *typestr, *buf;
>     Tcl_DString cmd;
>     enum bpdisp disp;
> ***************
> *** 3849,3853 ****
>     b->thread = thread;
> 
> !   sprintf (buf, "*(0x%lx)", addr);
>     b->addr_string = strsave (buf);
> 
> --- 3860,3864 ----
>     b->thread = thread;
> 
> !   asprintf (&buf, "*(0x%lx)", addr);
>     b->addr_string = strsave (buf);
> 
> ***************
> *** 3857,3865 ****
> 
>     Tcl_DStringAppend (&cmd, "gdbtk_tcl_breakpoint create ", -1);
> !   sprintf (buf, "%d", b->number);
>     Tcl_DStringAppendElement (&cmd, buf);
> !   sprintf (buf, "0x%lx", addr);
>     Tcl_DStringAppendElement (&cmd, buf);
> !   sprintf (buf, "%d", b->line_number);
>     Tcl_DStringAppendElement (&cmd, buf);
> 
> --- 3868,3879 ----
> 
>     Tcl_DStringAppend (&cmd, "gdbtk_tcl_breakpoint create ", -1);
> !   free(buf);
> !   asprintf (&buf, "%d", b->number);
>     Tcl_DStringAppendElement (&cmd, buf);
> !   free(buf);
> !   asprintf (&buf, "0x%lx", addr);
>     Tcl_DStringAppendElement (&cmd, buf);
> !   free(buf);
> !   asprintf (&buf, "%d", b->line_number);
>     Tcl_DStringAppendElement (&cmd, buf);
> 
> ***************
> *** 3869,3879 ****
>     Tcl_DStringAppendElement (&cmd, filename);
>     Tcl_DStringAppendElement (&cmd, bpdisp[b->disposition]);
> !   sprintf (buf, "%d", b->enable);
>     Tcl_DStringAppendElement (&cmd, buf);
> !   sprintf (buf, "%d", b->thread);
>     Tcl_DStringAppendElement (&cmd, buf);
> 
>     ret = Tcl_Eval (interp, Tcl_DStringValue (&cmd));
>     Tcl_DStringFree (&cmd);
>     return ret;
>   }
> --- 3883,3896 ----
>     Tcl_DStringAppendElement (&cmd, filename);
>     Tcl_DStringAppendElement (&cmd, bpdisp[b->disposition]);
> !   free(buf);
> !   asprintf (&buf, "%d", b->enable);
>     Tcl_DStringAppendElement (&cmd, buf);
> !   free(buf);
> !   asprintf (&buf, "%d", b->thread);
>     Tcl_DStringAppendElement (&cmd, buf);
> 
>     ret = Tcl_Eval (interp, Tcl_DStringValue (&cmd));
>     Tcl_DStringFree (&cmd);
> +   free(buf);
>     return ret;
>   }
> ***************
> *** 4014,4020 ****
>     if (!b || b->type != bp_breakpoint)
>       {
> !       char err_buf[64];
> !       sprintf (err_buf, "Breakpoint #%d does not exist.", bpnum);
>         Tcl_SetStringObj (result_ptr->obj_ptr, err_buf, -1);
>         return TCL_ERROR;
>       }
> --- 4031,4038 ----
>     if (!b || b->type != bp_breakpoint)
>       {
> !       char *err_buf;
> !       asprintf (&err_buf, "Breakpoint #%d does not exist.", bpnum);
>         Tcl_SetStringObj (result_ptr->obj_ptr, err_buf, -1);
> +       free(err_buf);
>         return TCL_ERROR;
>       }
> ***************
> *** 4309,4321 ****
>        Tcl_Obj *CONST objv[];
>   {
> !   char frame[32];
> 
>     if (selected_frame == NULL)
> !     strcpy (frame, "");
>     else
> !     sprintf (frame, "0x%s", paddr_nz (FRAME_FP (selected_frame)));
> 
>     Tcl_SetStringObj (result_ptr->obj_ptr, frame, -1);
> 
>     return TCL_OK;
>   }
> --- 4327,4340 ----
>        Tcl_Obj *CONST objv[];
>   {
> !   char *frame;
> 
>     if (selected_frame == NULL)
> !     asprintf (&frame, "%s","");
>     else
> !     asprintf (&frame, "0x%s", paddr_nz (FRAME_FP (selected_frame)));
> 
>     Tcl_SetStringObj (result_ptr->obj_ptr, frame, -1);
> 
> +   free(frame);
>     return TCL_OK;
>   }
> ***************
> *** 4339,4349 ****
>        Tcl_Obj *CONST objv[];
>   {
> !   char start[32];
> !   char end[32];
> 
>     if (selected_frame == NULL)
>       {
> !       strcpy (start, "");
> !       strcpy (end, "");
>       }
>     else
> --- 4358,4368 ----
>        Tcl_Obj *CONST objv[];
>   {
> !   char *start = NULL;
> !   char *end   = NULL;
> 
>     if (selected_frame == NULL)
>       {
> !       asprintf (&start, "%s", "");
> !       asprintf (&end, "%s", "");
>       }
>     else
> ***************
> *** 4351,4356 ****
>         struct block *block;
>         block = get_frame_block (selected_frame);
> !       sprintf (start, "0x%s", paddr_nz (BLOCK_START (block)));
> !       sprintf (end, "0x%s", paddr_nz (BLOCK_END (block)));
>       }
> 
> --- 4370,4375 ----
>         struct block *block;
>         block = get_frame_block (selected_frame);
> !       asprintf (&start, "0x%s", paddr_nz (BLOCK_START (block)));
> !       asprintf (&end, "0x%s", paddr_nz (BLOCK_END (block)));
>       }
> 
> ***************
> *** 4361,4364 ****
> --- 4380,4385 ----
>                             Tcl_NewStringObj (end, -1));
> 
> +   free(start);
> +   free(end);
>     return TCL_OK;
>   }
> ***************
> *** 4436,4449 ****
>           if (!junk && pc < BLOCK_END (block))
>             {
> !             char addr[32];
> 
>               Tcl_Obj *elt = Tcl_NewListObj (0, NULL);
> !             sprintf (addr, "0x%s", paddr_nz (BLOCK_START (block)));
>               Tcl_ListObjAppendElement (interp, elt,
>                                         Tcl_NewStringObj (addr, -1));
> !             sprintf (addr, "0x%s", paddr_nz (BLOCK_END (block)));
>               Tcl_ListObjAppendElement (interp, elt,
>                                         Tcl_NewStringObj (addr, -1));
>               Tcl_ListObjAppendElement (interp, result_ptr->obj_ptr, elt);
>             }
> 
> --- 4457,4472 ----
>           if (!junk && pc < BLOCK_END (block))
>             {
> !             char *addr;
> 
>               Tcl_Obj *elt = Tcl_NewListObj (0, NULL);
> !             asprintf (&addr, "0x%s", paddr_nz (BLOCK_START (block)));
>               Tcl_ListObjAppendElement (interp, elt,
>                                         Tcl_NewStringObj (addr, -1));
> !             free(addr);
> !             asprintf (&addr, "0x%s", paddr_nz (BLOCK_END (block)));
>               Tcl_ListObjAppendElement (interp, elt,
>                                         Tcl_NewStringObj (addr, -1));
>               Tcl_ListObjAppendElement (interp, result_ptr->obj_ptr, elt);
> +             free(addr);
>             }
> 
> diff -C2 -r -b ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk-hooks.c
> src/gdb/gdbtk/generic/gdbtk-hooks.c
> *** ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk-hooks.c      Thu Jul 13 09:10:03
> 2000
> --- src/gdb/gdbtk/generic/gdbtk-hooks.c Thu Oct 19 10:54:06 2000
> ***************
> *** 293,300 ****
>        va_list args;
>   {
> !   char buf[200];
> 
> !   vsprintf (buf, warning, args);
>     gdbtk_two_elem_cmd ("gdbtk_tcl_warning", buf);
>   }
> 
> --- 293,302 ----
>        va_list args;
>   {
> !   char *buf;
> 
> !   vasprintf (&buf, warning, args);
>     gdbtk_two_elem_cmd ("gdbtk_tcl_warning", buf);
> +
> +   free(buf);
>   }
> 
> ***************
> *** 326,333 ****
>        const char *warning;
>   {
> !   char buf[512];
> !   sprintf (buf, "gdbtk_tcl_ignorable_warning {%s} {%s}", class, warning);
>     if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
>       report_error ();
>   }
> 
> --- 328,336 ----
>        const char *warning;
>   {
> !   char *buf;
> !   asprintf (&buf, "gdbtk_tcl_ignorable_warning {%s} {%s}", class, warning);
>     if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
>       report_error ();
> +   free(buf);
>   }
> 
> ***************
> *** 460,468 ****
>   {
>     va_list args;
> !   char buf[200];
> 
>     va_start (args, format);
> !   vsprintf (buf, format, args);
>     gdbtk_two_elem_cmd ("gdbtk_tcl_readline_begin", buf);
>   }
> 
> --- 466,475 ----
>   {
>     va_list args;
> !   char *buf;
> 
>     va_start (args, format);
> !   vasprintf (&buf, format, args);
>     gdbtk_two_elem_cmd ("gdbtk_tcl_readline_begin", buf);
> +   free(buf);
>   }
> 
> ***************
> *** 529,533 ****
>     Tcl_DString cmd;
>     char *p;
> !   char buffer[30];
> 
>     Tcl_DStringInit (&cmd);
> --- 536,540 ----
>     Tcl_DString cmd;
>     char *p;
> !   char *buffer = NULL;
> 
>     Tcl_DStringInit (&cmd);
> ***************
> *** 541,545 ****
>       {
>         char *q = strchr (p, ' ');
> !       char save;
>         if (q)
>         {
> --- 548,552 ----
>       {
>         char *q = strchr (p, ' ');
> !       char save = '\0';
>         if (q)
>         {
> ***************
> *** 572,581 ****
>       case var_uinteger:
>       case var_zinteger:
> !       sprintf (buffer, "%u", *(unsigned int *) cmdblk->var);
>         Tcl_DStringAppendElement (&cmd, buffer);
>         break;
> 
>       case var_integer:
> !       sprintf (buffer, "%d", *(int *) cmdblk->var);
>         Tcl_DStringAppendElement (&cmd, buffer);
>         break;
> --- 579,588 ----
>       case var_uinteger:
>       case var_zinteger:
> !       asprintf (&buffer, "%u", *(unsigned int *) cmdblk->var);
>         Tcl_DStringAppendElement (&cmd, buffer);
>         break;
> 
>       case var_integer:
> !       asprintf (&buffer, "%d", *(int *) cmdblk->var);
>         Tcl_DStringAppendElement (&cmd, buffer);
>         break;
> ***************
> *** 591,594 ****
> --- 598,606 ----
> 
>     Tcl_DStringFree (&cmd);
> +
> +   if (buffer != NULL)
> +     {
> +        free(buffer);
> +     }
>   }
> 
> ***************
> *** 631,635 ****
>        const char *action;
>   {
> !   char buf[256];
>     int v;
>     struct symtab_and_line sal;
> --- 643,647 ----
>        const char *action;
>   {
> !   char *buf;
>     int v;
>     struct symtab_and_line sal;
> ***************
> *** 646,650 ****
>       filename = "";
> 
> !   sprintf (buf, "gdbtk_tcl_breakpoint %s %d 0x%lx %d {%s} {%s} %d %d",
>            action, b->number, (long) b->address, b->line_number, filename,
>            bpdisp[b->disposition], b->enable, b->thread);
> --- 658,662 ----
>       filename = "";
> 
> !   asprintf (&buf, "gdbtk_tcl_breakpoint %s %d 0x%lx %d {%s} {%s} %d %d",
>            action, b->number, (long) b->address, b->line_number, filename,
>            bpdisp[b->disposition], b->enable, b->thread);
> ***************
> *** 652,655 ****
> --- 664,668 ----
>     if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
>       report_error ();
> +   free(buf);
>   }
> 
> ***************
> *** 657,664 ****
>   gdbtk_load_hash (const char *section, unsigned long num)
>   {
> !   char buf[128];
> !   sprintf (buf, "Download::download_hash %s %ld", section, num);
>     if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
>       report_error ();
>     return atoi (gdbtk_interp->result);
>   }
> --- 670,679 ----
>   gdbtk_load_hash (const char *section, unsigned long num)
>   {
> !   char *buf;
> !   asprintf (&buf, "Download::download_hash %s %ld", section, num);
>     if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
>       report_error ();
> +   free(buf);
> +
>     return atoi (gdbtk_interp->result);
>   }
> ***************
> *** 712,720 ****
>        va_list args;
>   {
> !   char buf[200];
>     long val;
> 
> !   vsprintf (buf, query, args);
>     gdbtk_two_elem_cmd ("gdbtk_tcl_query", buf);
> 
>     val = atol (gdbtk_interp->result);
> --- 727,736 ----
>        va_list args;
>   {
> !   char *buf;
>     long val;
> 
> !   vasprintf (&buf, query, args);
>     gdbtk_two_elem_cmd ("gdbtk_tcl_query", buf);
> +   free(buf);
> 
>     val = atol (gdbtk_interp->result);
> ***************
> *** 760,764 ****
>        const char *action;
>   {
> !   char buf[256];
>     int v;
>     struct symtab_and_line sal;
> --- 776,780 ----
>        const char *action;
>   {
> !   char *buf;
>     int v;
>     struct symtab_and_line sal;
> ***************
> *** 772,780 ****
>     if (filename == NULL)
>       filename = "N/A";
> !   sprintf (buf, "gdbtk_tcl_tracepoint %s %d 0x%lx %d {%s} %d", action,
> tp->number,
>            (long) tp->address, sal.line, filename, tp->pass_count);
> 
>     if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
>       report_error ();
>   }
> 
> --- 788,797 ----
>     if (filename == NULL)
>       filename = "N/A";
> !   asprintf (&buf, "gdbtk_tcl_tracepoint %s %d 0x%lx %d {%s} %d", action,
> tp->number,
>            (long) tp->address, sal.line, filename, tp->pass_count);
> 
>     if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
>       report_error ();
> +   free(buf);
>   }
> 
> ***************
> *** 877,881 ****
>   gdbtk_annotate_signal ()
>   {
> !   char buf[128];
> 
>     /* Inform gui that the target has stopped. This is
> --- 894,898 ----
>   gdbtk_annotate_signal ()
>   {
> !   char *buf;
> 
>     /* Inform gui that the target has stopped. This is
> ***************
> *** 885,892 ****
>     Tcl_Eval (gdbtk_interp, "gdbtk_stop_idle_callback");
> 
> !   sprintf (buf, "gdbtk_signal %s {%s}", target_signal_to_name (stop_signal),
>            target_signal_to_string (stop_signal));
>     if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
>       report_error ();
>   }
> 
> --- 902,910 ----
>     Tcl_Eval (gdbtk_interp, "gdbtk_stop_idle_callback");
> 
> !   asprintf (&buf, "gdbtk_signal %s {%s}", target_signal_to_name
> (stop_signal),
>            target_signal_to_string (stop_signal));
>     if (Tcl_Eval (gdbtk_interp, buf) != TCL_OK)
>       report_error ();
> +   free(buf);
>   }
> 
> diff -C2 -r -b ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk-variable.c
> src/gdb/gdbtk/generic/gdbtk-variable.c
> *** ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk-variable.c   Thu Jul 13 09:10:03
> 2000
> --- src/gdb/gdbtk/generic/gdbtk-variable.c      Thu Oct 19 11:18:26 2000
> ***************
> *** 566,570 ****
>     gdb_variable *var;
>     char *name;
> !   char obj_name[31];
>     int index;
>     static int id = 0;
> --- 566,570 ----
>     gdb_variable *var;
>     char *name;
> !   char *obj_name;
>     int index;
>     static int id = 0;
> ***************
> *** 587,596 ****
>         /* generate a name for this object */
>         id++;
> !       sprintf (obj_name, "var%d", id);
>       }
>     else
>       {
>         /* specified name for object */
> !       strncpy (obj_name, name, 30);
>         objv++;
>         objc--;
> --- 587,596 ----
>         /* generate a name for this object */
>         id++;
> !       asprintf (&obj_name, "var%d", id);
>       }
>     else
>       {
>         /* specified name for object */
> !       asprintf (&obj_name, "%s", name);
>         objv++;
>         objc--;
> ***************
> *** 648,651 ****
> --- 648,653 ----
>       }
> 
> +   free(obj_name);
> +
>     return TCL_ERROR;
>   }
> ***************
> *** 2018,2025 ****
>       case TYPE_CODE_ARRAY:
>         {
> !         char number[16];
>           *obj = Tcl_NewStringObj (NULL, 0);
> !         sprintf (number, "%d", var->num_children);
>           Tcl_AppendStringsToObj (*obj, "[", number, "]", NULL);
>         }
>         break;
> --- 2020,2028 ----
>       case TYPE_CODE_ARRAY:
>         {
> !         char *number;
>           *obj = Tcl_NewStringObj (NULL, 0);
> !         asprintf (&number, "%d", var->num_children);
>           Tcl_AppendStringsToObj (*obj, "[", number, "]", NULL);
> +       free(number);
>         }
>         break;
> diff -C2 -r -b ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk.c
> src/gdb/gdbtk/generic/gdbtk.c
> *** ../gdb_cvs/src/gdb/gdbtk/generic/gdbtk.c    Thu Jul  6 12:04:47 2000
> --- src/gdb/gdbtk/generic/gdbtk.c       Thu Oct 19 10:59:48 2000
> ***************
> *** 197,201 ****
>   {
>     va_list args;
> !   char buf[10000], *v[3], *merge, *priority;
> 
>     switch (level)
> --- 197,201 ----
>   {
>     va_list args;
> !   char *buf, *v[3], *merge, *priority;
> 
>     switch (level)
> ***************
> *** 216,230 ****
>     va_start (args, fmt);
> 
>     v[0] = "dbug";
>     v[1] = priority;
>     v[2] = buf;
> 
> -   vsprintf (buf, fmt, args);
> -   va_end (args);
> -
>     merge = Tcl_Merge (3, v);
>     if (Tcl_Eval (gdbtk_interp, merge) != TCL_OK)
>       Tcl_BackgroundError (gdbtk_interp);
>     Tcl_Free (merge);
>   }
> 
> --- 216,232 ----
>     va_start (args, fmt);
> 
> +
> +   vasprintf (&buf, fmt, args);
> +   va_end (args);
> +
>     v[0] = "dbug";
>     v[1] = priority;
>     v[2] = buf;
> 
>     merge = Tcl_Merge (3, v);
>     if (Tcl_Eval (gdbtk_interp, merge) != TCL_OK)
>       Tcl_BackgroundError (gdbtk_interp);
>     Tcl_Free (merge);
> +   free(buf);
>   }
> 
> ***************
> *** 359,363 ****
>     struct cleanup *old_chain;
>     int found_main;
> !   char s[5];
>     Tcl_Obj *auto_path_elem, *auto_path_name;
> 
> --- 361,365 ----
>     struct cleanup *old_chain;
>     int found_main;
> !   char *s;
>     Tcl_Obj *auto_path_elem, *auto_path_name;
> 
> ***************
> *** 389,394 ****
>     /* Set up some globals used by gdb to pass info to gdbtk
>        for start up options and the like */
> !   sprintf (s, "%d", inhibit_gdbinit);
>     Tcl_SetVar2 (gdbtk_interp, "GDBStartup", "inhibit_prefs", s,
> TCL_GLOBAL_ONLY);
>     /* Note: Tcl_SetVar2() treats the value as read-only (making a
>        copy).  Unfortunatly it does not mark the parameter as
> --- 391,398 ----
>     /* Set up some globals used by gdb to pass info to gdbtk
>        for start up options and the like */
> !   asprintf (&s, "%d", inhibit_gdbinit);
>     Tcl_SetVar2 (gdbtk_interp, "GDBStartup", "inhibit_prefs", s,
> TCL_GLOBAL_ONLY);
> +   free(s);
> +
>     /* Note: Tcl_SetVar2() treats the value as read-only (making a
>        copy).  Unfortunatly it does not mark the parameter as

Re: Buffer Overflow Patch in GDB/TCL Interface code.

[Fernando Nasser]

Syd Polk wrote:
> 
> My only concern is whether this call, asprintf, is available on all platforms.
> 

As gdb already uses it, it must be available at least where gdb is available.
This should suffice for us.


-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@cygnus.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9

Re: Buffer Overflow Patch in GDB/TCL Interface code.

[Syd Polk]

At 04:27 PM 10/19/00 +0000, Fernando Nasser wrote:
>Syd Polk wrote:
> >
> > My only concern is whether this call, asprintf, is available on all 
> platforms.
> >
>
>As gdb already uses it, it must be available at least where gdb is available.
>This should suffice for us.

Then I approve this for check-in.

Syd Polk		spolk@redhat.com
Engineering Manager	+1 415 777 9810 x 241
Red Hat, Inc.

Re: Buffer Overflow Patch in GDB/TCL Interface code.

[Mike A. Harris]

On Thu, 19 Oct 2000, Syd Polk wrote:

>Date: Thu, 19 Oct 2000 09:20:54 -0700
>From: Syd Polk <spolk@redhat.com>
>To: Fernando Nasser <fnasser@cygnus.com>
>Cc: insight@sources.redhat.com
>Content-Type: text/plain; charset=us-ascii
>Subject: Re: Buffer Overflow Patch in GDB/TCL Interface code.
>
>My only concern is whether this call, asprintf, is available on all platforms.
>

IMHO, calls to any functions such as this, are just oddball
memleak situations waiting to happen.  I've never heard of
asprintf before myself..  Shouldn't it also be asnprintf() to
avoid a potential overflow?  Sorry if not, as I haven't looked at
the actual usage in the code.  Just some thoughts to share..

Take care!
TTYL


----------------------------------------------------------------------
      Mike A. Harris  -  Linux advocate  -  Open source advocate
              Computer Consultant - Capslock Consulting
                 Copyright 2000 all rights reserved
----------------------------------------------------------------------

If you're interested in computer security, and want to stay on top of the
latest security exploits, and other information, visit:

http://www.securityfocus.com

Liberty, asprintf(), malloc()

[Fernando Nasser]

Just to clarify things about asprintf() and vasprintf().

These are part of the GNU libiberty (-liberty).  This library is sanctioned and blessed
by the Gods of Free Software, it is highly portable and few GNU software doesn't rely on
it.  We can use liberty calls at will.

W.r.t. malloc() uses here a few rules:

1) free where you call malloc whenever possible

2) functions can return malloc'ed things, but they cannot malloc things that are not
   explicitly returned to the caller (there are exceptions, see 3).
   In this case it is like the caller called malloc: it should free what was returned.

3) gdb provides a mechanism so things can be safely malloc'ed during a command execution.
   You don't have to free() where you malloc() but you must (absolutely must) create a
   "cleanup" for it.  You can them rest assured that it will be destroyed at the
   appropriate time (it borrows the ideas for OOP destructos).

These practices have been working fine for ages.  The memory leaks happen when people
fail to follow the coding conventions above.

One of the reasons we have this post-and-approval (or just post for the maintainers) is
that people who find some time and/or curiosity to look at the patches can find that
some rule has been broken, something condition was not handled, typos etc.
Everyone makes mistake, sometimes we do thins pressed by time etc. so it is good that
we have other people to help us catch things before they manifest themselves as bugs.

P.S.: Anyway, memory leaks are a risk of programming in C (or other languages that
require explicitly deallocation, deletions etc.)  Java does not have this problem,
but them it has the garbage collector...


-- 
Fernando Nasser
Red Hat Canada Ltd.                     E-Mail:  fnasser@cygnus.com
2323 Yonge Street, Suite #300
Toronto, Ontario   M4P 2C9