Обсуждение: [PATCH] Memory leak, at src/common/exec.c

Hi,
On exec.c, have two memory leaks, and a possible access beyond heap bounds, the patch tries to fix them.
According to documentation at:
https://en.cppreference.com/w/c/experimental/dynamic/strdup
"The returned pointer must be passed to free to avoid a memory leak. "

regards,
Ranier Vilela

On 12/16/19 1:22 PM, Ranier Vilela wrote:
> Hi,
> On exec.c, have two memory leaks, and a possible access beyond heap bounds, the patch tries to fix them.
> According to documentation at:
> https://en.cppreference.com/w/c/experimental/dynamic/strdup
> "The returned pointer must be passed to free to avoid a memory leak."

Please see the man page for putenv.  Are you certain it is safe to
free the string passed to putenv after putenv returns?  I think this
may be implemented differently on various platforms.

Taken from `man putenv`:

"NOTES
        The putenv() function is not required to be reentrant, and the 
one in glibc 2.0 is not, but the glibc 2.1 version is.

        Since  version  2.1.2,  the  glibc  implementation  conforms to 
SUSv2: the pointer string given to putenv() is used.  In particular, 
this string becomes part of the environment; changing it later will
        change the environment.  (Thus, it is an error is to call 
putenv() with an automatic variable as the argument, then return from 
the calling function while string is still  part  of  the  environment.)
        However, glibc versions 2.0 to 2.1.1 differ: a copy of the 
string is used.  On the one hand this causes a memory leak, and on the 
other hand it violates SUSv2.

        The 4.4BSD version, like glibc 2.0, uses a copy.

        SUSv2 removes the const from the prototype, and so does glibc 2.1.3.
"

-- 
Mark Dilger

Mark Dilger <hornschnorter@gmail.com> writes:
> Please see the man page for putenv.  Are you certain it is safe to
> free the string passed to putenv after putenv returns?  I think this
> may be implemented differently on various platforms.

POSIX requires the behavior the glibc man page describes:

    The putenv() function shall use the string argument to set environment
    variable values. The string argument should point to a string of the
    form "name=value". The putenv() function shall make the value of
    the environment variable name equal to value by altering an existing
    variable or creating a new one. In either case, the string pointed to
    by string shall become part of the environment, so altering the string
    shall change the environment.

So yeah, that patch is completely wrong.  It might've survived light
testing with non-debug versions of malloc/free, but under any sort
of load the environment variable would become corrupted.  The reason
for the strdup in our code is exactly to make a long-lived string
that can safely be given to putenv.

            regards, tom lane

According to the documentation at:

https://wiki.sei.cmu.edu/confluence/display/c/POS34-C.+Do+not+call+putenv%28%29+with+a+pointer+to+an+automatic+variable+as+the+argument
"Using setenv() is easier and consequently less error prone than using putenv()."
putenv is problematic and error prone, better replace by setenv.

As a result, set_pglocale_pgservice, is much simpler and more readable.

regards,
Ranier Vilela

Ranier Vilela <ranier_gyn@hotmail.com> writes:
> According to the documentation at:
>
https://wiki.sei.cmu.edu/confluence/display/c/POS34-C.+Do+not+call+putenv%28%29+with+a+pointer+to+an+automatic+variable+as+the+argument
> "Using setenv() is easier and consequently less error prone than using putenv()."
> putenv is problematic and error prone, better replace by setenv.

setenv is also less portable: it does not appear in SUSv2, which is still
our baseline spec for Unix platforms.  We've avoided its use since 2001,
cf. ec7ddc158.

It's also fair to wonder how well this change would fly on Windows,
where we have to implement putenv for ourselves to get things to work
right (cf. src/port/win32env.c, which does not offer support for
setenv).

Please stop inventing reasons to change code that's worked fine for
decades.  We have better things to spend our time on.

            regards, tom lane

According to [1], windows does not support setenv, so for the patch to work [3],  would need to add it.
With the possibility of setenv going further [2], I am submitting in this thread, the patch to add setenv support on
thewindows side, avoiding starting a new trhead. 
It is based on pre-existing functions, and seeks to correctly emulate the functioning of the POSIX setenv, but has not
yetbeen tested. 

If this work is not acceptable then it is finished. And two memory leaks and a possible access beyond heap bounds,
reportedand not fixed. 

regards,
Ranier Vilela

[1] https://www.postgresql.org/message-id/29478.1576537771%40sss.pgh.pa.us
[2] https://www.postgresql.org/message-id/30119.1576538578%40sss.pgh.pa.us
[3]
https://www.postgresql.org/message-id/SN2PR05MB264066382E2CC75E734492C8E3510%40SN2PR05MB2640.namprd05.prod.outlook.com