Обсуждение: eclg -C ORACLE breaks data

Hello, we encountered unexpected behavior from an ECPG program
complied with the -C ORACLE option.  The program executes the
following query:

 SELECT 123::numeric(3,0), 't'::char(2)";

Compilation and execution steps:

$ ecpg -C ORACLE ecpgtest.pgc (attached)
$ gcc -g -o ecpgtest ecpgtest.c -L `pg_config --libdir` -I `pg_config --includedir` -lecpg -lpgtypes
$ ./ecpgtest
type: numeric : data: "120"
type: bpchar  : data: "t"

The expected numeric value is "123".


The code below is the direct cause of the unanticipated data
modification.

interfaces/ecpg/ecpglib/data.c:581 (whitespaces are swueezed)
>     if (varcharsize == 0 || varcharsize > size)
>     {
>       /*
>        * compatibility mode, blank pad and null
>        * terminate char array
>        */
>     if (ORACLE_MODE(compat) && (type == ECPGt_char || type == ECPGt_unsigned_char))
>     {
>       memset(str, ' ', varcharsize);
>       memcpy(str, pval, size);
>       str[varcharsize - 1] = '\0';

This results in overwriting str[-1], the last byte of the preceding
numeric in this case, with 0x00, representing the digit '0'. When
callers of ecpg_get_data() explicitly pass zero as varcharsize, they
provide storage that precisely fitting the data. However, it remains
uncertain if this assumption is valid when ecpg_store_result() passes
var->varcharsize which is also zero. Consequently, the current fix
presumes exact-fit storage when varcharsize is zero.

I haven't fully checked, but at least back to 10 have this issue.

Thoughts?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Apr 10, 2023 at 05:35:00PM +0900, Kyotaro Horiguchi wrote:
> This results in overwriting str[-1], the last byte of the preceding
> numeric in this case, with 0x00, representing the digit '0'. When
> callers of ecpg_get_data() explicitly pass zero as varcharsize, they
> provide storage that precisely fitting the data.

Good find, that's clearly wrong.  The test case is interesting.  On
HEAD, the processing of the second field eats up the data of the first
field.

> However, it remains
> uncertain if this assumption is valid when ecpg_store_result() passes
> var->varcharsize which is also zero. Consequently, the current fix
> presumes exact-fit storage when varcharsize is zero.

Based on what I can read in sqlda.c (ecpg_set_compat_sqlda() and
ecpg_set_native_sqlda()), the data length calculated adds an extra
byte to the data length when storing the data references in sqldata.
execute.c and ecpg_store_result() is actually much trickier than that
(see particularly the part where the code does an "allocate memory for
NULL pointers", where varcharsize could also be 0), still I agree that
this assumption should be OK.  The code is as it is for many years,
with its logic to do an estimation of allocation first, and then read
the data at once in the whole area allocated..

This thinko has been introduced by 3b7ab43, so this needs to go down
to v11.  I'll see to that.
--
Michael

(sorry for the wrong subject..)

At Mon, 17 Apr 2023 17:00:59 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Mon, Apr 10, 2023 at 05:35:00PM +0900, Kyotaro Horiguchi wrote:
> > This results in overwriting str[-1], the last byte of the preceding
> > numeric in this case, with 0x00, representing the digit '0'. When
> > callers of ecpg_get_data() explicitly pass zero as varcharsize, they
> > provide storage that precisely fitting the data.
> 
> Good find, that's clearly wrong.  The test case is interesting.  On
> HEAD, the processing of the second field eats up the data of the first
> field.
>
> > However, it remains
> > uncertain if this assumption is valid when ecpg_store_result() passes
> > var->varcharsize which is also zero. Consequently, the current fix
> > presumes exact-fit storage when varcharsize is zero.
> 
> Based on what I can read in sqlda.c (ecpg_set_compat_sqlda() and
> ecpg_set_native_sqlda()), the data length calculated adds an extra
> byte to the data length when storing the data references in sqldata.
> execute.c and ecpg_store_result() is actually much trickier than that
> (see particularly the part where the code does an "allocate memory for
> NULL pointers", where varcharsize could also be 0), still I agree that
> this assumption should be OK.  The code is as it is for many years,
> with its logic to do an estimation of allocation first, and then read
> the data at once in the whole area allocated..
> 
> This thinko has been introduced by 3b7ab43, so this needs to go down
> to v11.  I'll see to that.

Thanks for picking this up.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Apr 17, 2023 at 05:00:59PM +0900, Michael Paquier wrote:
> This thinko has been introduced by 3b7ab43, so this needs to go down
> to v11.  I'll see to that.

So done.  mylodon is feeling unhappy about that, because this has a
C99 declaration:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mylodon&dt=2023-04-18%2002%3A22%3A04
char_array.pgc:73:8: error: variable declaration in for loop is a C99-specific feature [-Werror,-Wc99-extensions]
  for (int i = 0 ; i < sqlda->sqld ; i++)

I'll go fix that in a minute, across all the branches for
consistency.
--
Michael

At Tue, 18 Apr 2023 11:35:16 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Mon, Apr 17, 2023 at 05:00:59PM +0900, Michael Paquier wrote:
> > This thinko has been introduced by 3b7ab43, so this needs to go down
> > to v11.  I'll see to that.
> 
> So done.  mylodon is feeling unhappy about that, because this has a

Thanks!

> C99 declaration:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mylodon&dt=2023-04-18%2002%3A22%3A04
> char_array.pgc:73:8: error: variable declaration in for loop is a C99-specific feature [-Werror,-Wc99-extensions]
>   for (int i = 0 ; i < sqlda->sqld ; i++)
> 
> I'll go fix that in a minute, across all the branches for
> consistency.

Oh, I didn't realize there were differences in the
configurations. Good to know.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Apr 20, 2023 at 12:56:32PM +0900, Kyotaro Horiguchi wrote:
> Oh, I didn't realize there were differences in the
> configurations. Good to know.

C99 declarations are OK in v12~, so with v11 going out of sight in
approximately 6 month, it won't matter soon ;)
--
Michael

At Thu, 20 Apr 2023 13:00:52 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> On Thu, Apr 20, 2023 at 12:56:32PM +0900, Kyotaro Horiguchi wrote:
> > Oh, I didn't realize there were differences in the
> > configurations. Good to know.
> 
> C99 declarations are OK in v12~, so with v11 going out of sight in
> approximately 6 month, it won't matter soon ;)

Ah. the time goes around..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center