Try to fix endless loop in ecpg with informix mode

Any comments?

2017-10-26 16:03 GMT+08:00 高增琦 <pgf00a@gmail.com>:

Hi,

I tried some tests with ecpg informix mode.
When trying to store float data into a integer var, I got endless loop.

The reason is:
In informix mode, ecpg can accept
string form of float number when processing query result.
During checking the string form of float number, it seems
that ecpg forgot to skip characters after '.'.
Then outer loop will never stop because it hopes to see '\0'.

The first patch will reproduce the problem in ecpg's regress test.
The second patch tries to fix it in simple way.

--
GaoZengqi
pgf00a@gmail.com
zengqigao@gmail.com

--
GaoZengqi
pgf00a@gmail.com
zengqigao@gmail.com

On Wed, Nov 1, 2017 at 12:22 PM, 高增琦 <pgf00a@gmail.com> wrote:

Any comments?

Hi,

You should register these patches for the next commitfest at
https://commitfest.postgresql.org/15/. As Michael pointed out earlier,
this commitfest will start soon so you should add your patches
quickly.

Regards.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Any comments?

Sorry, I've been working through the backlog of three weeks of
traveling.

I tried some tests with ecpg informix mode.
When trying to store float data into a integer var, I got endless
loop.

The reason is:
In informix mode, ecpg can accept
string form of float number when processing query result.
During checking the string form of float number, it seems
that ecpg forgot to skip characters after '.'.
Then outer loop will never stop because it hopes to see '\0'.

The first patch will reproduce the problem in ecpg's regress test.
The second patch tries to fix it in simple way.

Thanks for spotting and fixing. I changed your patch slightly and made
it check if the rest of the data is indeed digits, or else it would
accept something like "7.hello" as "7".

Committed.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Thanks for commit.

I am afraid the changes may separate "7.a" to "7" and "a", then error out
with "invalid input syntax for type int" for "a".

How about changes as below? (use following the if to decide true or false)

```
-            } while (**scan_length != ' ' && **scan_length != '\0');
-            return false;
+            } while (isdigit(**scan_length));
```

2017-11-01 20:35 GMT+08:00 Michael Meskes <meskes@postgresql.org>:

Any comments?

Sorry, I've been working through the backlog of three weeks of
traveling.

I tried some tests with ecpg informix mode.
When trying to store float data into a integer var, I got endless
loop.

The reason is:
In informix mode, ecpg can accept
string form of float number when processing query result.
During checking the string form of float number, it seems
that ecpg forgot to skip characters after '.'.
Then outer loop will never stop because it hopes to see '\0'.

The first patch will reproduce the problem in ecpg's regress test.
The second patch tries to fix it in simple way.

Thanks for spotting and fixing. I changed your patch slightly and made
it check if the rest of the data is indeed digits, or else it would
accept something like "7.hello" as "7".

Committed.

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

--
GaoZengqi
pgf00a@gmail.com
zengqigao@gmail.com

Diff from the head:
(use the following if to decide true or false)

```
diff --git a/src/interfaces/ecpg/ecpglib/data.c
b/src/interfaces/ecpg/ecpglib/data.c
index 5375934..1621e7b 100644
--- a/src/interfaces/ecpg/ecpglib/data.c
+++ b/src/interfaces/ecpg/ecpglib/data.c
@@ -57,8 +57,7 @@ garbage_left(enum ARRAY_TYPE isarray, char **scan_length,
enum COMPAT_MODE compa
             /* skip invalid characters */
             do {
                 (*scan_length)++;
-            } while (**scan_length != ' ' && **scan_length != '\0' &&
isdigit(**scan_length));
-            return false;
+            } while (isdigit(**scan_length));
         }

if (**scan_length != ' ' && **scan_length != '\0')

```

2017-11-02 11:07 GMT+08:00 高增琦 <pgf00a@gmail.com>:

Thanks for commit.

I am afraid the changes may separate "7.a" to "7" and "a", then error out
with "invalid input syntax for type int" for "a".

How about changes as below? (use following the if to decide true or false)

```
-            } while (**scan_length != ' ' && **scan_length != '\0');
-            return false;
+            } while (isdigit(**scan_length));
```

2017-11-01 20:35 GMT+08:00 Michael Meskes <meskes@postgresql.org>:

Any comments?

Sorry, I've been working through the backlog of three weeks of
traveling.

I tried some tests with ecpg informix mode.
When trying to store float data into a integer var, I got endless
loop.

The reason is:
In informix mode, ecpg can accept
string form of float number when processing query result.
During checking the string form of float number, it seems
that ecpg forgot to skip characters after '.'.
Then outer loop will never stop because it hopes to see '\0'.

The first patch will reproduce the problem in ecpg's regress test.
The second patch tries to fix it in simple way.

Thanks for spotting and fixing. I changed your patch slightly and made
it check if the rest of the data is indeed digits, or else it would
accept something like "7.hello" as "7".

Committed.

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

--
GaoZengqi
pgf00a@gmail.com
zengqigao@gmail.com

--
GaoZengqi
pgf00a@gmail.com
zengqigao@gmail.com

I am afraid the changes may separate "7.a" to "7" and "a", then error
out
with "invalid input syntax for type int" for "a".

Which is correct, is it not?

How about changes as below? (use following the if to decide true or
false)
...
return false;
+ } while (isdigit(**scan_length));

Yes, this is certainly correct and better than what I committed. What
was I thinking yesterday?

I think the same function is used for identifying garbage in floats
which might ask for different logic. Let me check.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Hi,

I found the last commit changed as:

```
            /* skip invalid characters */
            do {
                (*scan_length)++;
-           } while (**scan_length != ' ' && **scan_length != '\0' &&
isdigit(**scan_length));
+           } while (isdigit(**scan_length));
            return false;
        }
```

It will still return false if we got non-digital characters after ".",
then it will error out "invalid input syntax for type int" for "a" . (if
input is "7.a")

Although this error message is not wrong, I think it should be better to
give error message as "invalid input syntax for type int" for "7.a".
This could be done by delete "return false;" after "while(...)", let
the following if to decide which to return.

2017-11-02 15:25 GMT+08:00 Michael Meskes <meskes@postgresql.org>:

I am afraid the changes may separate "7.a" to "7" and "a", then error
out
with "invalid input syntax for type int" for "a".

Which is correct, is it not?

How about changes as below? (use following the if to decide true or
false)
...
return false;
+ } while (isdigit(**scan_length));

Yes, this is certainly correct and better than what I committed. What
was I thinking yesterday?

I think the same function is used for identifying garbage in floats
which might ask for different logic. Let me check.

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

--
GaoZengqi
pgf00a@gmail.com
zengqigao@gmail.com

Although this error message is not wrong, I think it should be better
to
give error message as "invalid input syntax for type int" for "7.a".
This could be done by delete "return false;" after "while(...)", let
the following if to decide which to return.

Ah, now I understand. Sorry, I completely misunderstood your prior
email. Yes, you're right, I will change the code accordingly.

Thanks.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers