Обсуждение: [HACKERS] 【ECPG】strncpy function does not set the end character '\0'
[HACKERS] 【ECPG】strncpy function does not set the end character '\0'
От
"postgresql_2016@163.com"
Дата:
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