SQLDA fix for ECPG

Started by Boszormenyi Zoltanabout 14 years ago7 messages

zb@cybertec.at

about 14 years ago

1 attachment(s)

Hi,

I had a report about ECPG code crashing which involved
a query using a date field. Attached is a one liner fix to make
the date type's offset computed consistently across
sqlda_common_total_size(), sqlda_compat_total_size() and
sqlda_native_total_size().

This must have been a cut and paste bug and is incorrect
in 9.0.x, 9.1.x and GIT HEAD. It would be nice to have it
applied before the next point releases come out.

Best regards,
Zoltďż˝n Bďż˝szďż˝rmďż˝nyi

--
----------------------------------
Zoltďż˝n Bďż˝szďż˝rmďż˝nyi
Cybertec Schďż˝nig & Schďż˝nig GmbH
Grďż˝hrmďż˝hlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

Attachments:

ecpg-sqlda.patchtext/plain; name=ecpg-sqlda.patchDownload

diff -durp postgresql-9.1.1.orig/src/interfaces/ecpg/ecpglib/sqlda.c postgresql-9.1.1/src/interfaces/ecpg/ecpglib/sqlda.c
--- postgresql-9.1.1.orig/src/interfaces/ecpg/ecpglib/sqlda.c	2011-09-22 23:57:57.000000000 +0200
+++ postgresql-9.1.1/src/interfaces/ecpg/ecpglib/sqlda.c	2011-11-11 14:24:17.794761458 +0100
@@ -124,7 +124,7 @@ sqlda_common_total_size(const PGresult *
 				}
 				break;
 			case ECPGt_date:
-				ecpg_sqlda_align_add_size(offset, sizeof(int), sizeof(date), &offset, &next_offset);
+				ecpg_sqlda_align_add_size(offset, sizeof(date), sizeof(date), &offset, &next_offset);
 				break;
 			case ECPGt_timestamp:
 				ecpg_sqlda_align_add_size(offset, sizeof(int), sizeof(timestamp), &offset, &next_offset);

Michael Meskes

meskes@postgresql.org

about 14 years ago

In reply to: Boszormenyi Zoltan (#1)

Re: SQLDA fix for ECPG

This must have been a cut and paste bug and is incorrect
in 9.0.x, 9.1.x and GIT HEAD. It would be nice to have it
applied before the next point releases come out.

Applied, thanks for the patch.

Michael

--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
Jabber: michael.meskes at googlemail dot com
VfL Borussia! Forďż˝a Barďż˝a! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Tom Lane

tgl@sss.pgh.pa.us

about 14 years ago

In reply to: Boszormenyi Zoltan (#1)

Re: SQLDA fix for ECPG

Boszormenyi Zoltan <zb@cybertec.at> writes:

I had a report about ECPG code crashing which involved
a query using a date field. Attached is a one liner fix to make
the date type's offset computed consistently across
sqlda_common_total_size(), sqlda_compat_total_size() and
sqlda_native_total_size().

Is this really the only issue there? I notice discrepancies among those
three routines for some other types too, notably ECPGt_timestamp and
ECPGt_interval.

regards, tom lane

Boszormenyi Zoltan

zb@cybertec.at

about 14 years ago

In reply to: Tom Lane (#3)

1 attachment(s)

Re: SQLDA fix for ECPG

2011-11-13 17:27 keltezďż˝ssel, Tom Lane ďż˝rta:

Boszormenyi Zoltan <zb@cybertec.at> writes:

I had a report about ECPG code crashing which involved
a query using a date field. Attached is a one liner fix to make
the date type's offset computed consistently across
sqlda_common_total_size(), sqlda_compat_total_size() and
sqlda_native_total_size().

Is this really the only issue there? I notice discrepancies among those
three routines for some other types too, notably ECPGt_timestamp and
ECPGt_interval.

regards, tom lane

Yes, you are right. For timestamp and interval, the safe alignment is int64.
Patch is attached.

Best regards,
Zoltďż˝n Bďż˝szďż˝rmďż˝nyi

Attachments:

ecpg-sqlda-part2.patchtext/plain; name=ecpg-sqlda-part2.patchDownload

--- postgresql-9.1.1/src/interfaces/ecpg/ecpglib/sqlda.c.orig	2011-11-14 08:59:15.118711180 +0100
+++ postgresql-9.1.1/src/interfaces/ecpg/ecpglib/sqlda.c	2011-11-14 09:02:53.787803059 +0100
@@ -127,10 +127,10 @@ sqlda_common_total_size(const PGresult *
 				ecpg_sqlda_align_add_size(offset, sizeof(date), sizeof(date), &offset, &next_offset);
 				break;
 			case ECPGt_timestamp:
-				ecpg_sqlda_align_add_size(offset, sizeof(int), sizeof(timestamp), &offset, &next_offset);
+				ecpg_sqlda_align_add_size(offset, sizeof(int64), sizeof(timestamp), &offset, &next_offset);
 				break;
 			case ECPGt_interval:
-				ecpg_sqlda_align_add_size(offset, sizeof(int), sizeof(interval), &offset, &next_offset);
+				ecpg_sqlda_align_add_size(offset, sizeof(int64), sizeof(interval), &offset, &next_offset);
 				break;
 			case ECPGt_char:
 			case ECPGt_unsigned_char:
@@ -359,7 +359,7 @@ ecpg_set_compat_sqlda(int lineno, struct
 				sqlda->sqlvar[i].sqllen = sizeof(date);
 				break;
 			case ECPGt_timestamp:
-				ecpg_sqlda_align_add_size(offset, sizeof(timestamp), sizeof(timestamp), &offset, &next_offset);
+				ecpg_sqlda_align_add_size(offset, sizeof(int64), sizeof(timestamp), &offset, &next_offset);
 				sqlda->sqlvar[i].sqldata = (char *) sqlda + offset;
 				sqlda->sqlvar[i].sqllen = sizeof(timestamp);
 				break;
@@ -545,7 +545,7 @@ ecpg_set_native_sqlda(int lineno, struct
 				sqlda->sqlvar[i].sqllen = sizeof(date);
 				break;
 			case ECPGt_timestamp:
-				ecpg_sqlda_align_add_size(offset, sizeof(timestamp), sizeof(timestamp), &offset, &next_offset);
+				ecpg_sqlda_align_add_size(offset, sizeof(int64), sizeof(timestamp), &offset, &next_offset);
 				sqlda->sqlvar[i].sqldata = (char *) sqlda + offset;
 				sqlda->sqlvar[i].sqllen = sizeof(timestamp);
 				break;

Michael Meskes

meskes@postgresql.org

about 14 years ago

In reply to: Boszormenyi Zoltan (#4)

Re: SQLDA fix for ECPG

On Mon, Nov 14, 2011 at 09:06:30AM +0100, Boszormenyi Zoltan wrote:

Yes, you are right. For timestamp and interval, the safe alignment is int64.
Patch is attached.

Applied, thanks.

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org
Jabber: michael.meskes at googlemail dot com
VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL

Boszormenyi Zoltan

zb@cybertec.at

about 14 years ago

In reply to: Michael Meskes (#5)

1 attachment(s)

Re: SQLDA fix for ECPG

Hi,

2011-11-17 14:53 keltezéssel, Michael Meskes írta:

On Mon, Nov 14, 2011 at 09:06:30AM +0100, Boszormenyi Zoltan wrote:

Yes, you are right. For timestamp and interval, the safe alignment is int64.
Patch is attached.

Applied, thanks.

Michael

thanks.

Hopefully last turn in this topic. For NUMERIC types, the safe minimum
alignment is a pointer because there are 5 int members followed by
two pointer members in this struct. I got a crash from this with a lucky
query and dataset. The DECIMAL struct is safe because the digits[] array
is embedded there.

After fixing this, I got another one at an innocent looking memcpy():

memcpy((char *) sqlda + offset, num->buf, num->ndigits + 1);

It turned out that when the server sends "0.00", PGTYPESnumeric_from_asc()
constructs a numeric structure with:
ndigits == 0
buf == NULL
digits == NULL.
This makes memcpy() crash and burn. Let's also fix this case.

Best regards,
Zoltán Böszörményi

--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/

Attachments:

ecpg-sqlda-part3.patchtext/plain; name=ecpg-sqlda-part3.patchDownload

--- postgresql-9.1.1/src/interfaces/ecpg/ecpglib/sqlda.c.orig	2011-11-19 21:13:25.699169076 +0100
+++ postgresql-9.1.1/src/interfaces/ecpg/ecpglib/sqlda.c	2011-11-19 22:50:13.895653223 +0100
@@ -110,7 +110,7 @@
 				 * int Unfortunately we need to do double work here to compute
 				 * the size of the space needed for the numeric structure.
 				 */
-				ecpg_sqlda_align_add_size(offset, sizeof(int), sizeof(numeric), &offset, &next_offset);
+				ecpg_sqlda_align_add_size(offset, sizeof(NumericDigit *), sizeof(numeric), &offset, &next_offset);
 				if (!PQgetisnull(res, row, i))
 				{
 					char	   *val = PQgetvalue(res, row, i);
@@ -119,7 +119,8 @@
 					num = PGTYPESnumeric_from_asc(val, NULL);
 					if (!num)
 						break;
-					ecpg_sqlda_align_add_size(next_offset, sizeof(int), num->ndigits + 1, &offset, &next_offset);
+					if (num->ndigits)
+						ecpg_sqlda_align_add_size(next_offset, sizeof(int), num->ndigits + 1, &offset, &next_offset);
 					PGTYPESnumeric_free(num);
 				}
 				break;
@@ -323,7 +324,7 @@
 
 					set_data = false;
 
-					ecpg_sqlda_align_add_size(offset, sizeof(int), sizeof(numeric), &offset, &next_offset);
+					ecpg_sqlda_align_add_size(offset, sizeof(NumericDigit *), sizeof(numeric), &offset, &next_offset);
 					sqlda->sqlvar[i].sqldata = (char *) sqlda + offset;
 					sqlda->sqlvar[i].sqllen = sizeof(numeric);
 
@@ -343,11 +344,14 @@
 
 					memcpy(sqlda->sqlvar[i].sqldata, num, sizeof(numeric));
 
-					ecpg_sqlda_align_add_size(next_offset, sizeof(int), num->ndigits + 1, &offset, &next_offset);
-					memcpy((char *) sqlda + offset, num->buf, num->ndigits + 1);
+					if (num->ndigits)
+					{
+						ecpg_sqlda_align_add_size(next_offset, sizeof(int), num->ndigits + 1, &offset, &next_offset);
+						memcpy((char *) sqlda + offset, num->buf, num->ndigits + 1);
 
-					((numeric *) sqlda->sqlvar[i].sqldata)->buf = (NumericDigit *) sqlda + offset;
-					((numeric *) sqlda->sqlvar[i].sqldata)->digits = (NumericDigit *) sqlda + offset + (num->digits - num->buf);
+						((numeric *) sqlda->sqlvar[i].sqldata)->buf = (NumericDigit *) sqlda + offset;
+						((numeric *) sqlda->sqlvar[i].sqldata)->digits = (NumericDigit *) sqlda + offset + (num->digits - num->buf);
+					}
 
 					PGTYPESnumeric_free(num);
 
@@ -509,7 +513,7 @@
 
 					set_data = false;
 
-					ecpg_sqlda_align_add_size(offset, sizeof(int), sizeof(numeric), &offset, &next_offset);
+					ecpg_sqlda_align_add_size(offset, sizeof(NumericDigit *), sizeof(numeric), &offset, &next_offset);
 					sqlda->sqlvar[i].sqldata = (char *) sqlda + offset;
 					sqlda->sqlvar[i].sqllen = sizeof(numeric);
 
@@ -529,11 +533,14 @@
 
 					memcpy(sqlda->sqlvar[i].sqldata, num, sizeof(numeric));
 
-					ecpg_sqlda_align_add_size(next_offset, sizeof(int), num->ndigits + 1, &offset, &next_offset);
-					memcpy((char *) sqlda + offset, num->buf, num->ndigits + 1);
+					if (num->ndigits)
+					{
+						ecpg_sqlda_align_add_size(next_offset, sizeof(int), num->ndigits + 1, &offset, &next_offset);
+						memcpy((char *) sqlda + offset, num->buf, num->ndigits + 1);
 
-					((numeric *) sqlda->sqlvar[i].sqldata)->buf = (NumericDigit *) sqlda + offset;
-					((numeric *) sqlda->sqlvar[i].sqldata)->digits = (NumericDigit *) sqlda + offset + (num->digits - num->buf);
+						((numeric *) sqlda->sqlvar[i].sqldata)->buf = (NumericDigit *) sqlda + offset;
+						((numeric *) sqlda->sqlvar[i].sqldata)->digits = (NumericDigit *) sqlda + offset + (num->digits - num->buf);
+					}
 
 					PGTYPESnumeric_free(num);

Michael Meskes

meskes@postgresql.org

about 14 years ago

In reply to: Boszormenyi Zoltan (#6)

Re: SQLDA fix for ECPG

On Sat, Nov 19, 2011 at 10:56:03PM +0100, Boszormenyi Zoltan wrote:

Hopefully last turn in this topic. For NUMERIC types, the safe minimum
alignment is a pointer because there are 5 int members followed by
two pointer members in this struct. I got a crash from this with a lucky
query and dataset. The DECIMAL struct is safe because the digits[] array
is embedded there.
...

Applied, thanks.