[RFA] remote.c: Avoid multiple serial_close calls on baud rate error

[RFA] remote.c: Avoid multiple serial_close calls on baud rate error

[Kevin Buettner]

One of my colleagues recently noticed the following:

    (gdb) set remotebaud 0x100000
    (gdb) target remote /dev/ttyS0
    warning: Invalid baud rate 1048576.  Maximum value is 460800.
    /dev/ttyS0: Invalid argument.
    (gdb) set remotebaud 230400
    (gdb) target remote /dev/ttyS0
    Segmentation fault

The reason for this SEGV is that remote.c was closing ``remote_desc''
twice.  On the second attempted close, it was accessing some data
structures through some already freed (and probably even reallocated)
memory.

The comment that I've added explains how the double close is avoided.

FWIW, I considered calling remote_close(), but decided against it
since remote_desc can not be passed explicitly to this function. 
Also, if the implementation of remote_close() were to change in some
way, it may end up doing more (or less) than what's desired for
handling the baud rate error.  Conversely, a hypothetical change in
remote_close() may require that the error handling code be changed in
a similar fashion, so the preferred path to fixing this problem isn't
quite so clear cut.  Therefore, I'm willing to revise this patch to
call remote_close() instead if that's deemed preferable.

With regard to the testcase above, it'd be nice if this could be added
to the testsuite, but I can't think of a portable way of doing so.

Okay?

	* remote.c (remote_open_1, remote_cisco_open): Avoid closing
	remote_desc more than once.

Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.122
diff -u -p -r1.122 remote.c
--- remote.c	10 Nov 2003 21:20:44 -0000	1.122
+++ remote.c	5 Dec 2003 19:58:17 -0000
@@ -2299,7 +2299,12 @@ remote_open_1 (char *name, int from_tty,
     {
       if (serial_setbaudrate (remote_desc, baud_rate))
 	{
+	  /* The requested speed could not be set.  Error out to
+	     top level after closing remote_desc.  Take care to
+	     set remote_desc to NULL to avoid closing remote_desc
+	     more than once.  */
 	  serial_close (remote_desc);
+	  remote_desc = NULL;
 	  perror_with_name (name);
 	}
     }
@@ -5552,7 +5557,12 @@ remote_cisco_open (char *name, int from_
   baud_rate = (baud_rate > 0) ? baud_rate : 9600;
   if (serial_setbaudrate (remote_desc, baud_rate))
     {
+      /* The requested speed could not be set.  Error out to
+	 top level after closing remote_desc.  Take care to
+	 set remote_desc to NULL to avoid closing remote_desc
+	 more than once.  */
       serial_close (remote_desc);
+      remote_desc = NULL;
       perror_with_name (name);
     }
 

Re: [RFA] remote.c: Avoid multiple serial_close calls on baud rate error

[Daniel Jacobowitz]

On Fri, Dec 05, 2003 at 01:36:32PM -0700, Kevin Buettner wrote:
> One of my colleagues recently noticed the following:
> 
>     (gdb) set remotebaud 0x100000
>     (gdb) target remote /dev/ttyS0
>     warning: Invalid baud rate 1048576.  Maximum value is 460800.
>     /dev/ttyS0: Invalid argument.
>     (gdb) set remotebaud 230400
>     (gdb) target remote /dev/ttyS0
>     Segmentation fault

Coincidentally, one of my colleagues also recently noticed this :)
Thanks!

No comments on the patch itself, looks plausible.

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [RFA] remote.c: Avoid multiple serial_close calls on baud rate error

[Andrew Cagney]

> One of my colleagues recently noticed the following:
> 
>     (gdb) set remotebaud 0x100000
>     (gdb) target remote /dev/ttyS0
>     warning: Invalid baud rate 1048576.  Maximum value is 460800.
>     /dev/ttyS0: Invalid argument.
>     (gdb) set remotebaud 230400
>     (gdb) target remote /dev/ttyS0
>     Segmentation fault
> 
> The reason for this SEGV is that remote.c was closing ``remote_desc''
> twice.  On the second attempted close, it was accessing some data
> structures through some already freed (and probably even reallocated)
> memory.
> 
> The comment that I've added explains how the double close is avoided.
> 
> FWIW, I considered calling remote_close(), but decided against it
> since remote_desc can not be passed explicitly to this function. 
> Also, if the implementation of remote_close() were to change in some
> way, it may end up doing more (or less) than what's desired for
> handling the baud rate error.  Conversely, a hypothetical change in
> remote_close() may require that the error handling code be changed in
> a similar fashion, so the preferred path to fixing this problem isn't
> quite so clear cut.  Therefore, I'm willing to revise this patch to
> call remote_close() instead if that's deemed preferable.
> 
> With regard to the testcase above, it'd be nice if this could be added
> to the testsuite, but I can't think of a portable way of doing so.

Calling target_close() here wouldn't be right.  The target isn't yet 
open, the push call only occures further down.  This begs the question: 
why was open called twice?  I suspect unpush_target should only call 
target_close on open/pushed targets.

Anyway, this change is fine.  It makes the relevant code more robust.

Andrew

Re: [RFA] remote.c: Avoid multiple serial_close calls on baud rate error

[Kevin Buettner]

On Dec 6,  8:57pm, Andrew Cagney wrote:

> Anyway, this change is fine.  It makes the relevant code more robust.

Committed.

Thanks,

Kevin