8.1beta, Subtle bug in COPY in Solaris systems

Started by Sergey E. Koposovover 20 years ago3 messages
#1Sergey E. Koposov
math@sai.msu.ru
1 attachment(s)

Hello,

First, I'll show the warnings seen when compiling postgres on
SunOS 5.6 with gcc 3.2.1

copy.c: In function `GetDecimalFromHex':
copy.c:2660: warning: subscript has type `char'
copy.c: In function `CopyReadAttributesText':
copy.c:2805: warning: subscript has type `char'
copy.c:2813: warning: subscript has type `char'

Actually this warnings are caused by the isdigit function.
On Solaris systems, isdigit is organized as an array lookup, so all the
arguments should be casted to unsigned char.

2660c2660
< if (isdigit(hex))
---

if (isdigit((unsigned char)hex))

2805c2805
< if (isxdigit(hexchar))
---

if (isxdigit((unsigned char)hexchar))

2813c2813
< if (isxdigit(hexchar))
---

if (isxdigit((unsigned char)hexchar))

Actually that problem cause not only warnings but real bugs too,
exploiting that problem. (when the char >128 and is not casted to
unsigned, on solaris there will be a negative indices of arrays)

For example on SunOS (or any Solaris):

test=# CREATE TABLE test0 (xx char(2));
CREATE TABLE
test=# copy test0 from stdin;
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself.

\x3О©╫
\.

test=# select length(xx) from test0;
length
--------
1
(1 row)

But on NOT Solaris:

test=# CREATE TABLE test0 (xx char(2));
CREATE TABLE
test=# copy test0 from stdin;
Enter data to be copied followed by a newline.
End with a backslash and a period on a line by itself.

\x3О©╫
\.

test=# select length(xx) from test0;
length
--------
2
(1 row)

I'm not sure that everybody will see that code properly due to encoding
differences. But the idea is just feed postgres with "\x3" and one
character with the code >128.

Regards,
Sergey

*****************************************************
Sergey E. Koposov
Max-Planck Institut fuer Astronomie
Web: http://lnfm1.sai.msu.ru/~math
E-mail: math@sai.msu.ru

Attachments:

copy.difftext/plain; charset=US-ASCII; name=copy.diffDownload
--- src/backend/commands/copy.c.orig	2005-09-01 15:07:01.000000000 +0200
+++ src/backend/commands/copy.c	2005-09-01 15:08:45.000000000 +0200
@@ -2657,7 +2657,7 @@
 static int
 GetDecimalFromHex(char hex)
 {
-	if (isdigit(hex))
+	if (isdigit((unsigned char)hex))
 		return hex - '0';
 	else
 		return tolower(hex) - 'a' + 10;
@@ -2802,7 +2802,7 @@
 						{
 							char hexchar = *cur_ptr;
 
-							if (isxdigit(hexchar))
+							if (isxdigit((unsigned char)hexchar))
 							{
 								int val = GetDecimalFromHex(hexchar);
 
@@ -2810,7 +2810,7 @@
 								if (cur_ptr < line_end_ptr)
 								{
 									hexchar = *cur_ptr;
-									if (isxdigit(hexchar))
+									if (isxdigit((unsigned char)hexchar))
 									{
 										cur_ptr++;
 										val = (val << 4) + GetDecimalFromHex(hexchar);
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Sergey E. Koposov (#1)
Re: 8.1beta, Subtle bug in COPY in Solaris systems

"Sergey E. Koposov" <math@sai.msu.ru> writes:

2660c2660
< if (isdigit(hex))
---

if (isdigit((unsigned char)hex))

Sigh. We keep fixing these, and they keep creeping back in. I wish
there were a way to get some more-mainstream compiler to warn about
passing chars to the <ctype.h> functions.

Thanks for the report. You only saw the three?

regards, tom lane

#3Sergey E. Koposov
math@sai.msu.ru
In reply to: Tom Lane (#2)
Re: 8.1beta, Subtle bug in COPY in Solaris systems

On Thu, 1 Sep 2005, Tom Lane wrote:

"Sergey E. Koposov" <math@sai.msu.ru> writes:

2660c2660
< if (isdigit(hex))
---

if (isdigit((unsigned char)hex))

Sigh. We keep fixing these, and they keep creeping back in. I wish
there were a way to get some more-mainstream compiler to warn about
passing chars to the <ctype.h> functions.

Thanks for the report. You only saw the three?

In fact, I saw two other warnings, but they should not cause any
problems (at least on my understanding....) :

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -fno-strict-aliasing -DFRONTEND -I. -I../../../src/interfaces/libpq -I../../../src/include -I/systools/include -c -o psqlscan.o psqlscan.c
In file included from ../../../src/include/c.h:53,
from ../../../src/include/postgres_fe.h:21,
from psqlscan.l:40:
../../../src/include/pg_config.h:659:1: warning: "_FILE_OFFSET_BITS" redefined
In file included from /systools/lib/gcc-lib/sparc-sun-solaris2.7/3.2.1/include/stdio.h:36,
from psqlscan.c:13:
/usr/include/sys/feature_tests.h:96:1: warning: this is the location of the previous definition

XXXXXXXXXXXX

gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -fno-strict-aliasing -Wno-error -I./../include -I. -I../../../../src/include -I/systools/include -DMAJOR_VERSION=4 -DMINOR_VERSION=1 -DPATCHLEVEL=1 -c -o preproc.o preproc.c
In file included from preproc.y:6412:
pgc.c: In function `yylex':
pgc.c:1504: warning: label `find_rule' defined but not used
preproc.y: At top level:
pgc.c:3565: warning: `yy_flex_realloc' defined but not used

With Best Regards,
Sergey

*****************************************************
Sergey E. Koposov
Max-Planck Institut fuer Astronomie
Web: http://lnfm1.sai.msu.ru/~math
E-mail: math@sai.msu.ru