[bug fix] ECPG: freeing memory for pgtypes crashes on Windows

On Fri, Feb 2, 2018 at 3:47 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:

The fix is to add PGTYPES_free() in libpgtypes.dll, just like libpq has PQfreemem() described here:

+#ifndef PGTYPES_FREE
+#define PGTYPES_FREE
+ extern void PGTYPES_free(void *ptr);
+#endif

It seems quite strange to repeat this in pgtypes_date.h,
pgtypes_interval.h and pgtypes_numeric.h. I guess you might not want
to introduce a new common header file so that his can be back-patched
more easily? Not sure if there is a project policy about that, but it
seems unfortunate to introduce maintenance burden by duplicating this.

+ <function>PGTYPES_free()/<function> instead of <function>free()</function>.

The "/" needs to move inside then closing tag.

--
Thomas Munro
http://www.enterprisedb.com

From: Thomas Munro [mailto:thomas.munro@enterprisedb.com]

+#ifndef PGTYPES_FREE
+#define PGTYPES_FREE
+ extern void PGTYPES_free(void *ptr);
+#endif

It seems quite strange to repeat this in pgtypes_date.h, pgtypes_interval.h
and pgtypes_numeric.h. I guess you might not want to introduce a new common
header file so that his can be back-patched more easily? Not sure if there
is a project policy about that, but it seems unfortunate to introduce
maintenance burden by duplicating this.

Your guess is correct. I wanted to avoid the duplication, but there was no good place to put this without requiring existing applications to change their #include directives.

+ <function>PGTYPES_free()/<function> instead of
<function>free()</function>.

The "/" needs to move inside then closing tag.

Thanks, fixed.

Regards
Takayuki Tsunakawa

Hello.

The objective of this patch looks reasonable and this doesn't
affect ecpg applications except for the problematic case that
happens only on Windows. So the points here are only the
documentation, the new function name and the how we should place
the new defintion.

At Mon, 5 Feb 2018 00:53:47 +0000, "Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com> wrote in <0A3221C70F24FB45833433255569204D1F8AE06B@G01JPEXMBYT05>

From: Thomas Munro [mailto:thomas.munro@enterprisedb.com]

+#ifndef PGTYPES_FREE
+#define PGTYPES_FREE
+ extern void PGTYPES_free(void *ptr);
+#endif

It seems quite strange to repeat this in pgtypes_date.h, pgtypes_interval.h
and pgtypes_numeric.h. I guess you might not want to introduce a new common
header file so that his can be back-patched more easily? Not sure if there
is a project policy about that, but it seems unfortunate to introduce
maintenance burden by duplicating this.

Your guess is correct. I wanted to avoid the duplication, but there was no good place to put this without requiring existing applications to change their #include directives.

+ <function>PGTYPES_free()/<function> instead of
<function>free()</function>.

The "/" needs to move inside then closing tag.

Thanks, fixed.

The types PGTYPES* has corresponding PGTYPES*_free() functions. I
found it a bit strange that the function is named as
PGTYPES_free(), which looks as if it is generic for all PGTYPES*
types. Perhaps PGTYPESchar_free() or PGTYPEStext_free() would be
preferable.

The documentation for PQescapeByteaConn is saying that:

https://www.postgresql.org/docs/10/static/libpq-exec.html#LIBPQ-PQESCAPEBYTEACONN

PQescapeByteaConn returns an escaped version of the from
parameter binary string in memory allocated with malloc(). This
memory should be freed using PQfreemem() when the result is no
longer needed.

The similar description is needed for the four counterparts of
the new free function nor users doesn't have a definite
suggestion on where to use it.

I cannot (or am quite reluctant to^^;) replay the problem,
instead, I looked over the test/* files and the replacement looks
correct.

I agree to Thomas on the opinion that the definition of
PGTYPES_free shouldn't be scattered around. There's some header
files that has only one or two function defenitions. I believe
that a new header file having only this definition is preferable
than the current shape.

# By the way, I think that multiple occurance of function
# prototypes for the same function don't get error as long as
# they are identical. Or does for CL?

Your guess is correct. I wanted to avoid the duplication, but
there was no good place to put this without requiring existing
applications to change their #include directives.

I suppose that it is enough to let the pgtype headers
(pgtypes_(timestamp|numeric|interfval).h) include the new common
header. *I* think that it is better than multiple definitions for
the same function are placed here and there. Applications don't
need to be changed with this.

# Anyway existing applications need to replace free() to
# PGTYPES_free()..

I think this doesn't need more profound review so I'll mark this
Ready For Commit after confirming the amendment.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

From: Kyotaro HORIGUCHI
The objective of this patch looks reasonable and this doesn't
affect ecpg applications except for the problematic case that
happens only on Windows. So the points here are only the
documentation, the new function name and the how we should place
the new defintion.

I think this doesn't need more profound review so I'll mark this
Ready For Commit after confirming the amendment.

I'm sorry for my late reply. Last week I was off for a week.

And thank you for your review. All modifications are done.

Regards
MauMau

At Sun, 25 Mar 2018 22:15:52 +0900, "MauMau" <maumau307@gmail.com> wrote in <B3BEB35436E3471095762969E2FCEDD6@tunaPC>

And thank you for your review. All modifications are done.

Thank you for the new version. I marked this as "Ready for
Committer" with one change.

- Windows requires this since different versions (MT/non-MT and
DEBUG/RELEASE?) of CRT are not compatible on malloc/free, which
is the same reason for PQfreemem().

- It applies on the master HEAD cleanly. Compiled with no
error. (Except for having some warnings with MSB8018 *1 for
some mb modules and seemingly harmless C4818 for many files and
this patch is not to blame.)

- Documentation looks fine.

- The change on regtest looks fine and ran sucessfully on both
Linux and Windows (vcregress ecpgcheck). But it doesn't prove
anything about the different versions of CRT library. (The
same can be said about PQfreemem())

- Style looks fine with one exception that extern "C" is
increasing indentation, so I fixed that in the attached
version.

*1: "The intermediate directory contains files shared from
another project" for pg_archivecleanup, pg_stat_statements,
pg_isolation_regress, latin2_and_win1250, utf8_and_cyrillic,
utf8_and_iso8859 and utf8_and_sjis2004.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Mar 26, 2018 at 6:07 PM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:

At Sun, 25 Mar 2018 22:15:52 +0900, "MauMau" <maumau307@gmail.com> wrote in <B3BEB35436E3471095762969E2FCEDD6@tunaPC>

And thank you for your review. All modifications are done.

Thank you for the new version. I marked this as "Ready for
Committer" with one change.

Hi Tsunakawa-san and Horiguchi-san,

I would like to get this committed soon, but I'm not sure about
backpatching -- see below. Here's a new version with a couple of
minor changes:

1. Small changes to the documentation.

2. I see that you added #include <pgtypes.h> to pgtypes_numeric.h and
pgtypes_interval.h. They have functions returning char *. I
experimented with removing those and including <pgtypes.h> directly in
the .pgc test files, but then I saw why you did that: it changes all
the line numbers in the expected output files making the patch much
larger. No strong objection there. But I figured we should at least
be consistent, so I added #include <pgtypes.h> to pgtypes_timestamp.h
and pgtypes_date.h (they also declare functions that return new
strings).

3. It seemed unnecessary to declare the new function in extern.h
*and* in pgtypes.h. I added #include "pgtypes.h" to common.c instead,
and a comment to introduce the section of that file that defines
functions from pgtypes.h.

4. I found a place in src/interfaces/ecpg/test/sql/sqlda.pgc where
you missed a free() call.

Are these changes OK?

Why is it OK that we do "free(outp_sqlda)" having got that pointer
from a statement like "exec sql fetch 1 from mycur1 into descriptor
outp_sqlda"? Isn't that memory allocated by libecpg.dll?

The files in this area all seem to lack our standard boilerplate,
copyright message, blaming everything on UC Berkeley etc. Your new
header fits the existing pattern, so I can't really complain about
that.

The examples in the documentation call a bunch of _to_asc() functions
and then don't free the result, which is a leak, but that isn't your
patch's fault. (Example: printf("numeric = %s\n",
PGTYPESnumeric_to_asc(num, 0))).

One question I have is whether it is against project policy to
backport a new file and a new user-facing function. It doesn't seem
like a great idea, because it means that client code would need to
depend on a specific patch release. Even if we found an existing
header to declare this function in, you'd still need to do conditional
magic before using it. So... how inconvenient do you think it would
be if we did this for 11+ only? Does anyone see a better way to do an
API evolution here? It's not beautiful but I suppose one work-around
that end-user applications could use while they are stuck on older
releases might be something like this, in their own tree, conditional
on major version:

#define PGTYPESchar_free(x) PGTYPESdate_free((date *)(x))

--
Thomas Munro
http://www.enterprisedb.com

Thomas Munro <thomas.munro@enterprisedb.com> writes:

One question I have is whether it is against project policy to
backport a new file and a new user-facing function.

Given that this has been broken since forever, and there've been
no complaints before now, I do not think the case for back-patching
is strong enough to justify the problems it would cause. Just
put it in v11 and be done.

Also, this bit in the proposed documentation seems quite inappropriate:

(This is a change from earlier releases of
<productname>PostgreSQL</productname> ...

We don't normally put in such comments at all, and if we do, we
specify which version we're talking about. Two years from now
this'd be totally confusing. I'd just make it read

(This is important only on Windows, where ...

regards, tom lane

From: Thomas Munro [mailto:thomas.munro@enterprisedb.com]
I would like to get this committed soon, but I'm not sure about backpatching
-- see below. Here's a new version with a couple of minor changes:

Thank you for taking care of this patch.

1. Small changes to the documentation.

I agree with Tom on this.

2. I see that you added #include <pgtypes.h> to pgtypes_numeric.h and
pgtypes_interval.h. They have functions returning char *. I
experimented with removing those and including <pgtypes.h> directly in
the .pgc test files, but then I saw why you did that: it changes all the
line numbers in the expected output files making the patch much larger.
No strong objection there. But I figured we should at least be consistent,
so I added #include <pgtypes.h> to pgtypes_timestamp.h and pgtypes_date.h
(they also declare functions that return new strings).

The reason I added pgtypes.h only in pgtypes_numeric.h and pgtypes_interval.h is that the opgtypes_date.h includes pgtypes_timestamp.h and pgtypes_timestamp.h in turn includes pgtypes_interval.h. So additional inclusion of pgtypes.h was not necessary. But I'm OK with your patch for consistency.

3. It seemed unnecessary to declare the new function in extern.h
*and* in pgtypes.h. I added #include "pgtypes.h" to common.c instead, and
a comment to introduce the section of that file that defines functions from
pgtypes.h.

Agreed, thanks.

4. I found a place in src/interfaces/ecpg/test/sql/sqlda.pgc where you
missed a free() call.

Are these changes OK?

Why is it OK that we do "free(outp_sqlda)" having got that pointer from
a statement like "exec sql fetch 1 from mycur1 into descriptor outp_sqlda"?
Isn't that memory allocated by libecpg.dll?

My colleague is now preparing a patch for that, which adds a function ECPGFreeSQLDA() in libecpg.so. That thread is here:

/messages/by-id/25C1C6B2E7BE044889E4FE8643A58BA963A42097@G01JPEXMBKW03

One question I have is whether it is against project policy to backport
a new file and a new user-facing function. It doesn't seem like a great
idea, because it means that client code would need to depend on a specific
patch release. Even if we found an existing header to declare this function
in, you'd still need to do conditional magic before using it. So... how
inconvenient do you think it would be if we did this for 11+ only? Does
anyone see a better way to do an API evolution here? It's not beautiful
but I suppose one work-around that end-user applications could use while
they are stuck on older releases might be something like this, in their
own tree, conditional on major version:

#define PGTYPESchar_free(x) PGTYPESdate_free((date *)(x))

I want some remedy for older releases. Our customer worked around this problem by getting a libpq connection in their ECPG application and calling PQfreemem(). That's an ugly kludge, and I don't want other users to follow it.

I don't see a problem with back-patching as-is, because existing users who just call free() or don't call free() won't be affected. I think that most serious applications somehow state their supported minor releases like "this application supports (or is certified against) PostgreSQL 10.5 or later", just like other applications support "RHEL 6.2 or later" or "Windows XP Sp2 or later."

Regards
Takayuki Tsunakawa

On Tue, Jun 12, 2018 at 2:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Thomas Munro <thomas.munro@enterprisedb.com> writes:

One question I have is whether it is against project policy to
backport a new file and a new user-facing function.

Given that this has been broken since forever, and there've been
no complaints before now, I do not think the case for back-patching
is strong enough to justify the problems it would cause. Just
put it in v11 and be done.

Done.

Also, this bit in the proposed documentation seems quite inappropriate:

(This is a change from earlier releases of
<productname>PostgreSQL</productname> ...

We don't normally put in such comments at all, and if we do, we
specify which version we're talking about. Two years from now
this'd be totally confusing. I'd just make it read

(This is important only on Windows, where ...

Done.

On Tue, Jun 12, 2018 at 1:09 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:

From: Thomas Munro [mailto:thomas.munro@enterprisedb.com]
Why is it OK that we do "free(outp_sqlda)" having got that pointer from
a statement like "exec sql fetch 1 from mycur1 into descriptor outp_sqlda"?
Isn't that memory allocated by libecpg.dll?

My colleague is now preparing a patch for that, which adds a function ECPGFreeSQLDA() in libecpg.so. That thread is here:

/messages/by-id/25C1C6B2E7BE044889E4FE8643A58BA963A42097@G01JPEXMBKW03

Thanks. I will follow up on that thread.

I want some remedy for older releases. Our customer worked around this problem by getting a libpq connection in their ECPG application and calling PQfreemem(). That's an ugly kludge, and I don't want other users to follow it.

I don't see a problem with back-patching as-is, because existing users who just call free() or don't call free() won't be affected. I think that most serious applications somehow state their supported minor releases like "this application supports (or is certified against) PostgreSQL 10.5 or later", just like other applications support "RHEL 6.2 or later" or "Windows XP Sp2 or later."

If there is a consensus that we should do that then I'll back-patch,
but so far no one else has spoken up in support.

--
Thomas Munro
http://www.enterprisedb.com

On Tue, Jun 12, 2018 at 1:09 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:

My colleague is now preparing a patch for that, which adds a function

ECPGFreeSQLDA() in libecpg.so. That thread is here:

/messages/by-id/25C1C6B2E7BE044889E4FE8643A58BA9
63A42097@G01JPEXMBKW03

Thanks. I will follow up on that thread.

He's created a separate thread for a new CF entry here:

https://commitfest.postgresql.org/18/1669/

I want some remedy for older releases. Our customer worked around this

problem by getting a libpq connection in their ECPG application and calling
PQfreemem(). That's an ugly kludge, and I don't want other users to follow
it.

I don't see a problem with back-patching as-is, because existing users

who just call free() or don't call free() won't be affected. I think that
most serious applications somehow state their supported minor releases like
"this application supports (or is certified against) PostgreSQL 10.5 or
later", just like other applications support "RHEL 6.2 or later" or "Windows
XP Sp2 or later."

If there is a consensus that we should do that then I'll back-patch,
but so far no one else has spoken up in support.

I'll follow the community decision. But I'm afraid that not enough people will comment on this to call it a consensus, because this topic will not be interesting... FWIW, I thought back-patching would make committers' future burdon smaller thanks to the smaller difference in the code of multiple major versions.

Regards
Takayuki Tsunakawa

On Mon, Jun 11, 2018 at 10:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Given that this has been broken since forever, and there've been
no complaints before now, I do not think the case for back-patching
is strong enough to justify the problems it would cause. Just
put it in v11 and be done.

I'm not sure I understand what problem would be created by
back-patching. It is true that anyone writing code that must work
with any version of PostgreSQL wouldn't able to count on this being
there, but the chances that anyone is writing such software using ECPG
is remote. In other words, nobody's going to add a version number
test to their ECPG code. They're just going to apply the update and
then use the new function.

I don't think this is a very important issue so I'm not prepared to
fight about it, but this looks pretty low-risk to me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hello. Thank you for commiting this, Thomas.

At Mon, 18 Jun 2018 07:25:13 +0000, "Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com> wrote in <0A3221C70F24FB45833433255569204D1FA1BCD9@G01JPEXMBYT05>

I want some remedy for older releases. Our customer worked around this

problem by getting a libpq connection in their ECPG application and calling
PQfreemem(). That's an ugly kludge, and I don't want other users to follow
it.

I don't see a problem with back-patching as-is, because existing users

who just call free() or don't call free() won't be affected. I think that
most serious applications somehow state their supported minor releases like
"this application supports (or is certified against) PostgreSQL 10.5 or
later", just like other applications support "RHEL 6.2 or later" or "Windows
XP Sp2 or later."

If there is a consensus that we should do that then I'll back-patch,
but so far no one else has spoken up in support.

I'll follow the community decision. But I'm afraid that not
enough people will comment on this to call it a consensus,
because this topic will not be interesting...

ECPG x Windows makes very small cardinarity even if it is not
zero. Anyway the vast majority of deverloper here are only
working on Unix like OSes. So only two or three persons can make
consensus on this issue:p

FWIW, I thought back-patching would make committers' future
burdon smaller thanks to the smaller difference in the code of
multiple major versions.

However I also don't see a problem to back-patch it, I don't see
a problem on such difference between versions.

I vote for back-patching this up to 9.5 (quite arbitrary..) but
it is fine for me if the documentation of 9.6 and earlier mention
a restriction like "For Windows environment, the application may
crash when it is using free() to the return values from
PGTYPES*_to_ascii functions. Make sure to use the same version of
CRT libray with the ECPG compiler in the case."

.. Is there any means to find the library version on the
installed environment?

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

From: Kyotaro HORIGUCHI [mailto:horiguchi.kyotaro@lab.ntt.co.jp]

However I also don't see a problem to back-patch it, I don't see
a problem on such difference between versions.

Thank you, Horiguchi-san and Robert.

.. Is there any means to find the library version on the
installed environment?

You can manually see the library version on the [Details] tab in the properties dialog with Windows Explorer. I don't know how to get the version in a program.

Regards
Takayuki Tsunakawa

At Mon, 25 Jun 2018 08:16:10 +0000, "Tsunakawa, Takayuki" <tsunakawa.takay@jp.fujitsu.com> wrote in <0A3221C70F24FB45833433255569204D1FA241F9@G01JPEXMBYT05>

From: Kyotaro HORIGUCHI [mailto:horiguchi.kyotaro@lab.ntt.co.jp]

However I also don't see a problem to back-patch it, I don't see
a problem on such difference between versions.

Thank you, Horiguchi-san and Robert.

.. Is there any means to find the library version on the
installed environment?

You can manually see the library version on the [Details] tab in the properties dialog with Windows Explorer. I don't know how to get the version in a program.

I meant by the "version" not the version-number but the
identification of /MT /MD of CRT libraries. Such description
(mentioned in my previous mail) makes sense if the reader has any
means to see what "version" of the CRT library the target ECPG
library (and/or compiler) uses.

--
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Jun 25, 2018 at 8:16 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:

From: Kyotaro HORIGUCHI [mailto:horiguchi.kyotaro@lab.ntt.co.jp]

However I also don't see a problem to back-patch it, I don't see
a problem on such difference between versions.

Thank you, Horiguchi-san and Robert.

Ok, back-patched.

It seems like the other patch[1]https://commitfest.postgresql.org/18/1669/ might need the same treatment, right?

[1]: https://commitfest.postgresql.org/18/1669/

--
Thomas Munro
http://www.enterprisedb.com

From: Thomas Munro

Ok, back-patched.

Thank you very much!

It seems like the other patch[1] might need the same treatment,

right?

I believe so, because that patch is based on the same cause.

Regards
MauMau