[PATCH] cygwin_rexec() returns pointer to deallocated memory

[PATCH] cygwin_rexec() returns pointer to deallocated memory

[David Stacey]

[-- Attachment #1: Type: text/plain, Size: 557 bytes --]

In function cygwin_rexec(), a pointer to local buffer 'ahostbuf' is 
returned through 'ahost'. However, the buffer will have been deallocated 
at the end of the function, and so the contents of 'ahost' will be 
undefined. A trivial patch (attached) fixes the problem by making 
'ahostbuf' static.

This patch fixes Coverity bug ID #60028.

Change Log:
2014-05-24  David Stacey  <drstacey@tiscali.co.uk>

         * libc/rexec.cc (cygwin_rexec):
         Corrected returning a pointer to a buffer that will have gone 
out of
         scope.

Cheers,

Dave.


[-- Attachment #2: cygwin_rexec.patch --]
[-- Type: text/plain, Size: 362 bytes --]

--- cygwin-orig/libc/rexec.cc	2013-04-23 10:44:35.000000000 +0100
+++ cygwin/libc/rexec.cc	2014-05-24 22:37:39.764370000 +0100
@@ -317,7 +317,7 @@
 	u_short port = 0;
 	int s, timo = 1, s3;
 	char c;
-	char ahostbuf[INTERNET_MAX_HOST_NAME_LENGTH + 1];
+	static char ahostbuf[INTERNET_MAX_HOST_NAME_LENGTH + 1];
 
 	myfault efault;
 	if (efault.faulted (EFAULT))

Re: [PATCH] cygwin_rexec() returns pointer to deallocated memory

[Peter Rosin]

On 2014-05-25 00:00, David Stacey wrote:
> In function cygwin_rexec(), a pointer to local buffer 'ahostbuf' is returned through 'ahost'. However, the buffer will have been deallocated at the end of the function, and so the contents of 'ahost' will be undefined. A trivial patch (attached) fixes the problem by making 'ahostbuf' static.
> 
> This patch fixes Coverity bug ID #60028.
> 
> Change Log:
> 2014-05-24  David Stacey  <drstacey@tiscali.co.uk>
> 
>         * libc/rexec.cc (cygwin_rexec):
>         Corrected returning a pointer to a buffer that will have gone out of
>         scope.

I'm comparing with [1] and the same comment is applicable here (reading "it"
as "static").

Cheers,
Peter

[1] https://cygwin.com/viewvc/src/winsup/cygwin/libc/rcmd.cc?revision=1.8&view=markup#l134

Re: [PATCH] cygwin_rexec() returns pointer to deallocated memory

[David Stacey]

On 26/05/14 08:04, Peter Rosin wrote:
> On 2014-05-25 00:00, David Stacey wrote:
>> In function cygwin_rexec(), a pointer to local buffer 'ahostbuf' is returned through 'ahost'. However, the buffer will have been deallocated at the end of the function, and so the contents of 'ahost' will be undefined. A trivial patch (attached) fixes the problem by making 'ahostbuf' static.
>>   
>> This patch fixes Coverity bug ID #60028.
>>   
>> Change Log:
>> 2014-05-24  David Stacey<drstacey@tiscali.co.uk>
>>   
>>          * libc/rexec.cc (cygwin_rexec):
>>          Corrected returning a pointer to a buffer that will have gone out of
>>          scope.
> I'm comparing with [1] and the same comment is applicable here (reading "it"
> as "static").
>
> [1]https://cygwin.com/viewvc/src/winsup/cygwin/libc/rcmd.cc?revision=1.8&view=markup#l134

The two functions behave in a similar fashion. In both cases, an out 
parameter called 'ahost' is assigned to a buffer that is local to the 
function. The case of cygwin_rcmd_af() is correct in that the buffer is 
created statically (and so the buffer will not be destroyed at the end 
of the function). This means that the contents of the buffer will be 
available to the calling function.

However, in the case of cygwin_rexec(), the buffer is not static and is 
allocated on the stack. Hence after the function, if the stack were to 
be used (e.g. for local variables or function parameters) the contents 
of the buffer could easily become corrupted.

So yes, I would argue that 'static' is appropriate in both cases.

Cheers,

Dave.

Re: [PATCH] cygwin_rexec() returns pointer to deallocated memory

[Peter Rosin]

On 2014-05-26 12:09, David Stacey wrote:
> On 26/05/14 08:04, Peter Rosin wrote:
>> On 2014-05-25 00:00, David Stacey wrote:
>>> In function cygwin_rexec(), a pointer to local buffer 'ahostbuf' is returned through 'ahost'. However, the buffer will have been deallocated at the end of the function, and so the contents of 'ahost' will be undefined. A trivial patch (attached) fixes the problem by making 'ahostbuf' static.
>>>   This patch fixes Coverity bug ID #60028.
>>>   Change Log:
>>> 2014-05-24  David Stacey<drstacey@tiscali.co.uk>
>>>            * libc/rexec.cc (cygwin_rexec):
>>>          Corrected returning a pointer to a buffer that will have gone out of
>>>          scope.
>> I'm comparing with [1] and the same comment is applicable here (reading "it"
>> as "static").
>>
>> [1]https://cygwin.com/viewvc/src/winsup/cygwin/libc/rcmd.cc?revision=1.8&view=markup#l134
> 
> The two functions behave in a similar fashion. In both cases, an out parameter called 'ahost' is assigned to a buffer that is local to the function. The case of cygwin_rcmd_af() is correct in that the buffer is created statically (and so the buffer will not be destroyed at the end of the function). This means that the contents of the buffer will be available to the calling function.
> 
> However, in the case of cygwin_rexec(), the buffer is not static and is allocated on the stack. Hence after the function, if the stack were to be used (e.g. for local variables or function parameters) the contents of the buffer could easily become corrupted.
> 
> So yes, I would argue that 'static' is appropriate in both cases.

I believe the comment refers to if "static" is the right answer to the
problem. Is there a need to handle concurrent calls?

Cheers,
Peter

Re: [PATCH] cygwin_rexec() returns pointer to deallocated memory

[David Stacey]

On 26/05/14 14:36, Peter Rosin wrote:
> I believe the comment refers to if "static" is the right answer to the
> problem. Is there a need to handle concurrent calls?

I can't really comment on that. As the code stands, neither of the two 
functions that we are discussing are reentrant. As long as the author 
and the user(s) of the routines are both aware of that then it isn't a 
problem.

I was just trying to fix a coding error that was picked up by Coverity 
Scan; it wasn't my intention to question the design.

Cheers,

Dave.

Re: [PATCH] cygwin_rexec() returns pointer to deallocated memory

[Christopher Faylor]

On Mon, May 26, 2014 at 04:27:10PM +0100, David Stacey wrote:
>On 26/05/14 14:36, Peter Rosin wrote:
>> I believe the comment refers to if "static" is the right answer to the
>> problem. Is there a need to handle concurrent calls?
>
>I can't really comment on that. As the code stands, neither of the two 
>functions that we are discussing are reentrant. As long as the author 
>and the user(s) of the routines are both aware of that then it isn't a 
>problem.
>
>I was just trying to fix a coding error that was picked up by Coverity 
>Scan; it wasn't my intention to question the design.

But that is the usual problem with Coverity.  Making the simple, obvious
fix to correct a Coverity warning isn't necessarily the right way to
deal with the issue.

In this case, the linux man page says:

  ATTRIBUTES
     Multithreading (see pthreads(7))
	 The rexec() and rexec_af() functions are not thread-safe.

so static is appropriate.

cgf

Re: [PATCH] cygwin_rexec() returns pointer to deallocated memory

[Peter Rosin]

On 2014-05-26 18:35, Christopher Faylor wrote:
> On Mon, May 26, 2014 at 04:27:10PM +0100, David Stacey wrote:
>> On 26/05/14 14:36, Peter Rosin wrote:
>>> I believe the comment refers to if "static" is the right answer to the
>>> problem. Is there a need to handle concurrent calls?
>>
>> I can't really comment on that. As the code stands, neither of the two 
>> functions that we are discussing are reentrant. As long as the author 
>> and the user(s) of the routines are both aware of that then it isn't a 
>> problem.
>>
>> I was just trying to fix a coding error that was picked up by Coverity 
>> Scan; it wasn't my intention to question the design.
> 
> But that is the usual problem with Coverity.  Making the simple, obvious
> fix to correct a Coverity warning isn't necessarily the right way to
> deal with the issue.
> 
> In this case, the linux man page says:
> 
>   ATTRIBUTES
>      Multithreading (see pthreads(7))
> 	 The rexec() and rexec_af() functions are not thread-safe.
> 
> so static is appropriate.

"Not thread-safe" doesn't automatically mean that a following call may mess
with what was returned from a prior call (by the same thread). But since
it (IMHO) is a poor interface with no description of how to free any
possibly allocated memory, I agree that static is the only viable option.

Cheers,
Peter

Re: [PATCH] cygwin_rexec() returns pointer to deallocated memory

[Christopher Faylor]

On Mon, May 26, 2014 at 10:39:03PM +0200, Peter Rosin wrote:
>On 2014-05-26 18:35, Christopher Faylor wrote:
>> On Mon, May 26, 2014 at 04:27:10PM +0100, David Stacey wrote:
>>> On 26/05/14 14:36, Peter Rosin wrote:
>>>> I believe the comment refers to if "static" is the right answer to the
>>>> problem. Is there a need to handle concurrent calls?
>>>
>>> I can't really comment on that. As the code stands, neither of the two 
>>> functions that we are discussing are reentrant. As long as the author 
>>> and the user(s) of the routines are both aware of that then it isn't a 
>>> problem.
>>>
>>> I was just trying to fix a coding error that was picked up by Coverity 
>>> Scan; it wasn't my intention to question the design.
>> 
>> But that is the usual problem with Coverity.  Making the simple, obvious
>> fix to correct a Coverity warning isn't necessarily the right way to
>> deal with the issue.
>> 
>> In this case, the linux man page says:
>> 
>>   ATTRIBUTES
>>      Multithreading (see pthreads(7))
>> 	 The rexec() and rexec_af() functions are not thread-safe.
>> 
>> so static is appropriate.
>
>"Not thread-safe" doesn't automatically mean that a following call may mess
>with what was returned from a prior call (by the same thread). But since
>it (IMHO) is a poor interface with no description of how to free any
>possibly allocated memory, I agree that static is the only viable option.

The question was about reentrancy.  AFAIK, "reentrant" doesn't mean that
the buffer passed back from a call can't be subsequently modified by the
thread.  I'm not aware of any interface which enforces that behavior.

cgf

Re: [PATCH] cygwin_rexec() returns pointer to deallocated memory

[Christopher Faylor]

On Mon, May 26, 2014 at 05:40:49PM -0400, Christopher Faylor wrote:
>On Mon, May 26, 2014 at 10:39:03PM +0200, Peter Rosin wrote:
>>On 2014-05-26 18:35, Christopher Faylor wrote:
>>> On Mon, May 26, 2014 at 04:27:10PM +0100, David Stacey wrote:
>>>> On 26/05/14 14:36, Peter Rosin wrote:
>>>>> I believe the comment refers to if "static" is the right answer to the
>>>>> problem. Is there a need to handle concurrent calls?
>>>>
>>>> I can't really comment on that. As the code stands, neither of the two 
>>>> functions that we are discussing are reentrant. As long as the author 
>>>> and the user(s) of the routines are both aware of that then it isn't a 
>>>> problem.
>>>>
>>>> I was just trying to fix a coding error that was picked up by Coverity 
>>>> Scan; it wasn't my intention to question the design.
>>> 
>>> But that is the usual problem with Coverity.  Making the simple, obvious
>>> fix to correct a Coverity warning isn't necessarily the right way to
>>> deal with the issue.
>>> 
>>> In this case, the linux man page says:
>>> 
>>>   ATTRIBUTES
>>>      Multithreading (see pthreads(7))
>>> 	 The rexec() and rexec_af() functions are not thread-safe.
>>> 
>>> so static is appropriate.
>>
>>"Not thread-safe" doesn't automatically mean that a following call may mess
>>with what was returned from a prior call (by the same thread). But since
>>it (IMHO) is a poor interface with no description of how to free any
>>possibly allocated memory, I agree that static is the only viable option.
>
>The question was about reentrancy.  AFAIK, "reentrant" doesn't mean that
>the buffer passed back from a call can't be subsequently modified by the
>thread.  I'm not aware of any interface which enforces that behavior.

Btw, the latest version of freebsd can't have this particular problem
since ahostbuf is now gone.  We probably should pull in the latest version
into Cygwin's tree.

cgf

Re: [PATCH] cygwin_rexec() returns pointer to deallocated memory

[Christopher Faylor]

On Mon, May 26, 2014 at 05:46:10PM -0400, Christopher Faylor wrote:
>Btw, the latest version of freebsd can't have this particular problem
>since ahostbuf is now gone.  We probably should pull in the latest version
>into Cygwin's tree.

...and that's apparently because Corinna added the code in question...

cgf

Re: [PATCH] cygwin_rexec() returns pointer to deallocated memory

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

On May 26 19:54, Christopher Faylor wrote:
> On Mon, May 26, 2014 at 05:46:10PM -0400, Christopher Faylor wrote:
> >Btw, the latest version of freebsd can't have this particular problem
> >since ahostbuf is now gone.  We probably should pull in the latest version
> >into Cygwin's tree.
> 
> ...and that's apparently because Corinna added the code in question...

...and I can't remember why I added the buffer at all.  It looks like I
was trying to workaround a problem with the lifetime of the hp buffer
content.  The ahostbuf buffer is in there since I pulled the code in
from FreeBSD in 2006 so I had ... some ... reason.  Which eludes me.

I applied David's patch.


Thanks,
Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]