Обсуждение: [HACKERS] 【ECPG】strncpy function does not set the end character '\0'

Hi

When we reviewed the ecpg code,we found the array seem not have the end
character('\0')  after using the strncpy function. 

In the function ECPGnoticeReceiver, we use the stncpy function copy the
sqlstate to sqlca->sqlstate. And the  sqlca->sqlstate is defined as the size
of 5, and the copy size is sizeof(sqlca->sqlstate). However, from the
previous strcmp function, the sqlstate size may be 5,such as
ECPG_SQLSTATE_INVALID_CURSOR_NAME. So there may be lack of the end character
for sqlca->sqlstate.

------------------------------------------------------------------------------------------------------

the copy code 
       /* map to SQLCODE for backward compatibility */       if (strcmp(sqlstate, ECPG_SQLSTATE_INVALID_CURSOR_NAME) ==
0)              sqlcode = ECPG_WARNING_UNKNOWN_PORTAL;       else if (strcmp(sqlstate,
ECPG_SQLSTATE_ACTIVE_SQL_TRANSACTION)==
 
0)               sqlcode = ECPG_WARNING_IN_TRANSACTION;       else if (strcmp(sqlstate,
ECPG_SQLSTATE_NO_ACTIVE_SQL_TRANSACTION)
== 0)               sqlcode = ECPG_WARNING_NO_TRANSACTION;       else if (strcmp(sqlstate,
ECPG_SQLSTATE_DUPLICATE_CURSOR)== 0)               sqlcode = ECPG_WARNING_PORTAL_EXISTS;       else
sqlcode= 0;
 
      * strncpy(sqlca->sqlstate, sqlstate, sizeof(sqlca->sqlstate));*       sqlca->sqlcode = sqlcode;
sqlca->sqlwarn[2]= 'W';       sqlca->sqlwarn[0] = 'W';
 

the defined code 

struct sqlca_t
{       char            sqlcaid[8];       long            sqlabc;       long            sqlcode;       struct       {
           int                     sqlerrml;               char            sqlerrmc[SQLERRMC_LEN];       }
        sqlerrm;       char            sqlerrp[8];       long            sqlerrd[6];       /* Element 0: empty
                                  */       /* 1: OID of processed tuple if applicable                      */       /*
2:number of rows processed                          */       /* after an INSERT, UPDATE or                           */
     /* DELETE statement                                     */       /* 3: empty
     */       /* 4: empty                                             */       /* 5: empty
              */       char            sqlwarn[8];       /* Element 0: set to 'W' if at least one other is 'W'   */
 /* 1: if 'W' at least one character string              */       /* value was truncated when it was
 */       /* stored into a host variable.             */
 
       /*        * 2: if 'W' a (hopefully) non-fatal notice occurred        */     /* 3: empty */       /* 4: empty
                                       */       /* 5: empty                                             */       /* 6:
empty                                            */       /* 7: empty                                             */
 
      * char            sqlstate[5];*
};





--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

> When we reviewed the ecpg code,we found the array seem not have the
> end
> character('\0')  after using the strncpy function. 

True.

> In the function ECPGnoticeReceiver, we use the stncpy function copy
> the
> sqlstate to sqlca->sqlstate. And the  sqlca->sqlstate is defined as
> the size
> of 5, and the copy size is sizeof(sqlca->sqlstate). However, from the
> previous strcmp function, the sqlstate size may be 5,such as
> ECPG_SQLSTATE_INVALID_CURSOR_NAME. So there may be lack of the end
> character
> for sqlca->sqlstate.

Why do you think there should be one? My memory might be wrong but I
don't think it's supposed to be a null terminated string.

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

Michael Meskes <meskes@postgresql.org> writes:
>> In the function ECPGnoticeReceiver, we use the stncpy function copy
>> the
>> sqlstate to sqlca->sqlstate. And the  sqlca->sqlstate is defined as
>> the size
>> of 5, and the copy size is sizeof(sqlca->sqlstate). However, from the
>> previous strcmp function, the sqlstate size may be 5,such as
>> ECPG_SQLSTATE_INVALID_CURSOR_NAME. So there may be lack of the end
>> character
>> for sqlca->sqlstate.

> Why do you think there should be one? My memory might be wrong but I
> don't think it's supposed to be a null terminated string.

That field is defined as char[5] in struct sqlca_t, so the intent is
clearly that it not be null terminated.  However, it looks to me like
there'd be at least one alignment-padding byte after it, and that byte
is likely to be 0 in a lot of situations (particularly for statically
allocated sqlca_t's).  So a lot of the time, you could get away with
using strcmp() or other functions that expect null termination.

I'm thinking therefore that there's probably code out there that tries
to do strcmp(sqlca->sqlstate, "22000") or suchlike, and it works often
enough that the authors haven't identified their bug.  The question is
do we want to try to make that be valid code.

Changing the field declaration to char[5+1] would be easy enough, but
I have no idea how many places in ecpglib would need to change to make
sure that the last byte gets set to 0.
        regards, tom lane

> > Why do you think there should be one? My memory might be wrong but
> > I
> > don't think it's supposed to be a null terminated string.
> 
> That field is defined as char[5] in struct sqlca_t, so the intent is
> clearly that it not be null terminated.  However, it looks to me like
> there'd be at least one alignment-padding byte after it, and that
> byte
> is likely to be 0 in a lot of situations (particularly for statically
> allocated sqlca_t's).  So a lot of the time, you could get away with
> using strcmp() or other functions that expect null termination.

With "supposed" I was referring to the standard that defines SQLCA.

> I'm thinking therefore that there's probably code out there that
> tries
> to do strcmp(sqlca->sqlstate, "22000") or suchlike, and it works
> often
> enough that the authors haven't identified their bug.  The question
> is
> do we want to try to make that be valid code.
> 
> Changing the field declaration to char[5+1] would be easy enough, but
> I have no idea how many places in ecpglib would need to change to
> make
> sure that the last byte gets set to 0.

I doubt it'll be a lot. However, it would make us differ, albeit very
slightly, from what others do. I haven't come up with a practical
problem coming from that difference though.

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

Michael Meskes <meskes@postgresql.org> writes:
> With "supposed" I was referring to the standard that defines SQLCA.

Oh.  If there's actually a standard somewhere that says it's not
null-terminated, then code that is expecting it to be so is just
wrong.  No need to change anything in ecpg, IMO.
        regards, tom lane

> Oh.  If there's actually a standard somewhere that says it's not
> null-terminated, then code that is expecting it to be so is just
> wrong.  No need to change anything in ecpg, IMO.

As I said I haven't checked if this detail is actually in there, but I
guess it was because there is a reason why we, and other databases,
define it that way.

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