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

[MauMau]

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

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

[Thomas Munro]

On Mon, Jun 25, 2018 at 8:16 PM, Tsunakawa, Takayuki
 wrote:
> From: Kyotaro HORIGUCHI [mailto:horiguchi.kyot...@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] might need the same treatment, right?

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

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

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

[Kyotaro HORIGUCHI]

At Mon, 25 Jun 2018 08:16:10 +, "Tsunakawa, Takayuki" 
 wrote in 
<0A3221C70F24FB45833433255569204D1FA241F9@G01JPEXMBYT05>
> From: Kyotaro HORIGUCHI [mailto:horiguchi.kyot...@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

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

[Tsunakawa, Takayuki]

From: Kyotaro HORIGUCHI [mailto:horiguchi.kyot...@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

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

[Kyotaro HORIGUCHI]

Hello. Thank you for commiting this, Thomas.

At Mon, 18 Jun 2018 07:25:13 +, "Tsunakawa, Takayuki" 
 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

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

[Robert Haas]

On Mon, Jun 11, 2018 at 10:04 AM, Tom Lane  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

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

[Tsunakawa, Takayuki]

> On Tue, Jun 12, 2018 at 1:09 PM, Tsunakawa, Takayuki
>  wrote:
> > My colleague is now preparing a patch for that, which adds a function
> ECPGFreeSQLDA() in libecpg.so.  That thread is here:
> >
> https://www.postgresql.org/message-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

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

[Tsunakawa, Takayuki]

> From: Thomas Munro [mailto:thomas.mu...@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  to pgtypes_numeric.h and
> pgtypes_interval.h.  They have functions returning char *.  I
> experimented with removing those and including  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  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:

https://www.postgresql.org/message-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

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

[Tom Lane]

Thomas Munro  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
PostgreSQL ...

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

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

[Thomas Munro]

On Mon, Mar 26, 2018 at 6:07 PM, Kyotaro HORIGUCHI
 wrote:
> At Sun, 25 Mar 2018 22:15:52 +0900, "MauMau"  wrote in 
> 
>> 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  to pgtypes_numeric.h and
pgtypes_interval.h.  They have functions returning char *.  I
experimented with removing those and including  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  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


pgtypes_freemem_v5.patch
Description: Binary data

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

[Kyotaro HORIGUCHI]

At Sun, 25 Mar 2018 22:15:52 +0900, "MauMau"  wrote in 

> 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
diff --git a/doc/src/sgml/ecpg.sgml b/doc/src/sgml/ecpg.sgml
index 98b6840520..3b8a7abaec 100644
--- a/doc/src/sgml/ecpg.sgml
+++ b/doc/src/sgml/ecpg.sgml
@@ -1951,9 +1951,14 @@ EXEC SQL SELECT started, duration INTO :ts1, :iv1 FROM datetbl WHERE d=:date1;
 PGTYPEStimestamp_add_interval(, , );
 out = PGTYPEStimestamp_to_asc();
 printf("Started + duration: %s\n", out);
-free(out);
+PGTYPESchar_free(out);
 ]]>
 
+   Some functions such as PGTYPESnumeric_to_asc return
+   a character string. To free those strings, you need to use
+   PGTYPESchar_free() instead of free().
+   This is necessary for Windows, because memory allocation and release must be
+   done with the functions in the same version of library.
   
 
   
@@ -2029,6 +2034,7 @@ char *PGTYPESnumeric_to_asc(numeric *num, int dscale);
 
The numeric value will be printed with dscale decimal
digits, with rounding applied if necessary.
+   You need to free the string with PGTYPESchar_free().
   
  
 
@@ -2419,6 +2425,7 @@ char *PGTYPESdate_to_asc(date dDate);
 The function receives the date dDate as its only parameter.
 It will output the date in the form 1999-01-18, i.e., in the
 -MM-DD format.
+You need to free the string with PGTYPESchar_free().

   
  
@@ -2841,6 +2848,7 @@ char *PGTYPEStimestamp_to_asc(timestamp tstamp);
 The function receives the timestamp tstamp as
 its only argument and returns an allocated string that contains the
 textual representation of the timestamp.
+You need to free the string with PGTYPESchar_free().

   
  
@@ -3349,6 +3357,7 @@ char *PGTYPESinterval_to_asc(interval *span);
 The function converts the interval variable that span
 points to into a C char*. The output looks like this example:
 @ 1 day 12 hours 59 mins 10 secs.
+You need to free the string with PGTYPESchar_free().

   
  
diff --git a/src/interfaces/ecpg/include/Makefile b/src/interfaces/ecpg/include/Makefile
index e92e56f26f..9c68bf3c47 100644
--- a/src/interfaces/ecpg/include/Makefile
+++ b/src/interfaces/ecpg/include/Makefile
@@ -14,7 +14,7 @@ install: all installdirs install-headers
 
 .PHONY: install-headers
 ecpg_headers = ecpgerrno.h ecpglib.h ecpgtype.h sqlca.h sql3types.h ecpg_informix.h \
-	pgtypes_error.h pgtypes_numeric.h pgtypes_timestamp.h pgtypes_date.h pgtypes_interval.h \
+	pgtypes_error.h pgtypes_numeric.h pgtypes_timestamp.h pgtypes_date.h pgtypes_interval.h pgtypes.h \
 	sqlda.h sqlda-compat.h sqlda-native.h
 informix_headers = datetime.h decimal.h sqltypes.h
 
diff --git a/src/interfaces/ecpg/include/pgtypes.h b/src/interfaces/ecpg/include/pgtypes.h
new file mode 100644
index 00..dbf759b45f
--- /dev/null
+++ b/src/interfaces/ecpg/include/pgtypes.h
@@ -0,0 +1,17 @@
+/* src/interfaces/ecpg/include/pgtypes.h */
+
+#ifndef PGTYPES_H
+#define PGTYPES_H
+
+#ifdef __cplusplus
+extern "C"
+{
+#endif
+
+extern void PGTYPESchar_free(char *ptr);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif			/* PGTYPES_H */
diff --git a/src/interfaces/ecpg/include/pgtypes_interval.h b/src/interfaces/ecpg/include/pgtypes_interval.h
index 5747736fe1..3b17cd1d11 100644
--- a/src/interfaces/ecpg/include/pgtypes_interval.h
+++ b/src/interfaces/ecpg/include/pgtypes_interval.h
@@ -4,6 +4,7 @@
 #define PGTYPES_INTERVAL
 
 #include 
+#include 
 
 #ifndef C_H
 
diff --git a/src/interfaces/ecpg/include/pgtypes_numeric.h b/src/interfaces/ecpg/include/pgtypes_numeric.h
index 56c46ea272..5c763a9eb6 100644
--- a/src/interfaces/ecpg/include/pgtypes_numeric.h
+++ 

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

[MauMau]

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


pgtypes_freemem_v3.patch
Description: Binary data

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

[Kyotaro HORIGUCHI]

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 +, "Tsunakawa, Takayuki" 
 wrote in 
<0A3221C70F24FB45833433255569204D1F8AE06B@G01JPEXMBYT05>
> From: Thomas Munro [mailto:thomas.mu...@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.  
> 
> 
> > +   PGTYPES_free()/ instead of
> > free().
> > 
> > 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

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

[Thomas Munro]

On Fri, Feb 2, 2018 at 3:47 PM, Tsunakawa, Takayuki
 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.

+   PGTYPES_free()/ instead of free().

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

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