[BUG][PATCH] ecpg crash with bytea type and cursors

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

[BUG][PATCH] ecpg crash with bytea type and cursors

Jehan-Guillaume de Rorthais
Hi,

A customer reported to us a crash of ecpg when using bytea as variable of
cursors.

Please, find in attachment a patch adding a regression test reproducing the
crash: v1-0001-Add-ecpg-test-on-using-bytea-as-cursor-variable.patch

Here is the backtrace:

  #0  0x0000564aaeeed3f4 in adjust_outofscope_cursor_vars
      (cur=cur@entry=0x564aaff25630) at preproc.y:296
  #1  0x0000564aaef037eb in base_yyparse () at preproc.y:9851
  #2  0x0000564aaeee8945 in main (argc=2, argv=0x7ffeb5ddb108) at ecpg.c:458

According to documentation and source, the bytea type is supposed to be handled
as the varchar type. However, the original patch 050710b3696 miss to add to
adjust_outofscope_cursor_vars() how to handle bytea type as variable. Because of
this, bytea type was considered as an array of value, leading to the crash
because type->u.element was all NULL. The patch adjust existing code to
considers bytea in the same way varchar is handled. See in attachment patch
v1-0002-Fix-ecpg-crash-with-bytea-and-cursor-variables.patch

But there's something else the patch does not address and that might need to be
adjusted. Existing tests were OK because they don't use cursors AND bytea vars
are declared using a macro to hold their size. Eg.:

  bytea send_buf[2][DATA_SIZE]; # ok
  bytea send_buf[2][512]; # crash !

This is because of this test in adjust_outofscope_cursor_vars() (before the
proposed patch):

    else if ((ptr->variable->type->type != ECPGt_varchar
              && ptr->variable->type->type != ECPGt_char
              && ptr->variable->type->type != ECPGt_unsigned_char
              && ptr->variable->type->type != ECPGt_string)
             && atoi(ptr->variable->type->size) > 1)

This test match and related code crash when the size is given as number because
atoi(size) > 1. When using a macro to hold the size, this test doesn't
match and the fallback else{} code doesn't crash, but it doesn't build the same
variable neither (see indirection before ECPGget_var). I'm not sure how
important it is for bytea/varchar type though. I don't know how to build a test
case producing ECPGget_var for them.

Regards,

v1-0001-Add-ecpg-test-on-using-bytea-as-cursor-variable.patch (57K) Download Attachment
v1-0002-Fix-ecpg-crash-with-bytea-and-cursor-variables.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [BUG][PATCH] ecpg crash with bytea type and cursors

Michael Meskes-3
Hi,

On Tue, 2020-06-30 at 15:30 +0200, Jehan-Guillaume de Rorthais wrote:

> ...
> According to documentation and source, the bytea type is supposed to
> be handled
> as the varchar type. However, the original patch 050710b3696 miss to
> add to
> adjust_outofscope_cursor_vars() how to handle bytea type as variable.
> Because of
> this, bytea type was considered as an array of value, leading to the
> crash
> because type->u.element was all NULL. The patch adjust existing code
> to
> considers bytea in the same way varchar is handled. See in attachment
> patch
> v1-0002-Fix-ecpg-crash-with-bytea-and-cursor-variables.patch

Thanks for finding and fixing this. Patch committed.

> But there's something else the patch does not address and that might
> need to be
> adjusted. Existing tests were OK because they don't use cursors AND
> bytea vars
> are declared using a macro to hold their size. Eg.:
>
>   bytea send_buf[2][DATA_SIZE]; # ok
>   bytea send_buf[2][512]; # crash !
>
> This is because of this test in adjust_outofscope_cursor_vars()
> (before the
> proposed patch):
>
>     else if ((ptr->variable->type->type != ECPGt_varchar
>               && ptr->variable->type->type != ECPGt_char
>               && ptr->variable->type->type != ECPGt_unsigned_char
>               && ptr->variable->type->type != ECPGt_string)
>              && atoi(ptr->variable->type->size) > 1)
>
> This test match and related code crash when the size is given as
> number because
> atoi(size) > 1. When using a macro to hold the size, this test
> doesn't
> match and the fallback else{} code doesn't crash, but it doesn't
> build the same
> variable neither (see indirection before ECPGget_var). I'm not sure
> how
> important it is for bytea/varchar type though. I don't know how to
> build a test
> case producing ECPGget_var for them.

I try to find some time to look into this.

Thanks again,

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



Reply | Threaded
Open this post in threaded view
|

Re: [BUG][PATCH] ecpg crash with bytea type and cursors

Jehan-Guillaume de Rorthais
On Tue, 30 Jun 2020 18:38:08 +0200
Michael Meskes <[hidden email]> wrote:

> Hi,
>
> On Tue, 2020-06-30 at 15:30 +0200, Jehan-Guillaume de Rorthais wrote:
>  [...]  
>
> Thanks for finding and fixing this. Patch committed.

Oh, I didn't thought this would be that fast. Thanks!

I don't know if it's still possible to set the reported-by flag, but the
reporter was Gérard Lesbats. I left it as "to-be-defined" in my original patch
while waiting for his agreement.

Regards,


Reply | Threaded
Open this post in threaded view
|

Re: [BUG][PATCH] ecpg crash with bytea type and cursors

Michael Meskes-3
> I don't know if it's still possible to set the reported-by flag, but
> the
> reporter was Gérard Lesbats. I left it as "to-be-defined" in my
> original patch
> while waiting for his agreement.

I didn't know that. I don't think it's possible to change the commit
message, particularly after some other commits have been pushed.

Sorry about that.

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



Reply | Threaded
Open this post in threaded view
|

Re: [BUG][PATCH] ecpg crash with bytea type and cursors

Tom Lane-2
In reply to this post by Michael Meskes-3
Michael Meskes <[hidden email]> writes:
> Thanks for finding and fixing this. Patch committed.

Looks like you missed pushing it into the v13 branch?

                        regards, tom lane


Reply | Threaded
Open this post in threaded view
|

Re: [BUG][PATCH] ecpg crash with bytea type and cursors

Michael Meskes-3
On Tue, 2020-06-30 at 13:19 -0400, Tom Lane wrote:
> Michael Meskes <[hidden email]> writes:
> > Thanks for finding and fixing this. Patch committed.
>
> Looks like you missed pushing it into the v13 branch?

Actually no, this one is having problems with "make check" on my
system, but I do not want to upload without running the regression
tests. This is a local problem, the break happens during "make
install". I'm currently redoing it all. No idea why master and v12
works and v13 not. I will push as soon as I fixed this.

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



Reply | Threaded
Open this post in threaded view
|

Re: [BUG][PATCH] ecpg crash with bytea type and cursors

Michael Meskes-3
> Actually no, this one is having problems with "make check" on my
> system, but I do not want to upload without running the regression
> tests. This is a local problem, the break happens during "make
> install". I'm currently redoing it all. No idea why master and v12
> works and v13 not. I will push as soon as I fixed this.

BTW found it, dynloader.h was not removed during the clean cycle. :)

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



Reply | Threaded
Open this post in threaded view
|

Re: [BUG][PATCH] ecpg crash with bytea type and cursors

Jehan-Guillaume de Rorthais
In reply to this post by Michael Meskes-3
On Tue, 30 Jun 2020 19:18:58 +0200
Michael Meskes <[hidden email]> wrote:

> > I don't know if it's still possible to set the reported-by flag, but
> > the
> > reporter was Gérard Lesbats. I left it as "to-be-defined" in my
> > original patch
> > while waiting for his agreement.  
>
> I didn't know that. I don't think it's possible to change the commit
> message, particularly after some other commits have been pushed.
>
> Sorry about that.

No problem. I should have hold the patch until I have the agreement or warn
explicitly.

Regards,