This is the mail archive of the
cygwin-patches
mailing list for the Cygwin project.
Re: [PATCH] cygwin_rexec() returns pointer to deallocated memory
- From: Peter Rosin <peda at lysator dot liu dot se>
- To: cygwin-patches at cygwin dot com
- Date: Mon, 26 May 2014 15:36:11 +0200
- Subject: Re: [PATCH] cygwin_rexec() returns pointer to deallocated memory
- Authentication-results: sourceware.org; auth=none
- References: <53811668 dot 5010208 at tiscali dot co dot uk> <5382E760 dot 7 at lysator dot liu dot se> <538312E4 dot 1040201 at tiscali dot co dot uk>
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